open(2): Remove O_BENEATH and AT_BENEATH

[kib]

Summary

with the reasoning that the flags did not worked properly, and were not shipped in a release.

O_RESOLVE_BENEATH is kept as useful.

[markj]

Do we have any reason to believe that O_RESOLVE_BENEATH semantics are different from Linux's O_BENEATH?

[emaste]

From Linux openat2 there are a set of resolve flags including

       RESOLVE_BENEATH
              Do not permit the path resolution to succeed if any
              component of the resolution is not a descendant of
              the directory indicated by dirfd.  This causes
              absolute symbolic links (and absolute values of
              pathname) to be rejected.

              Currently, this flag also disables magic-link
              resolution (see below).  However, this may change
              in the future.  Therefore, to ensure that magic
              links are not resolved, the caller should
              explicitly specify RESOLVE_NO_MAGICLINKS.

...
The semantics of RESOLVE_BENEATH were modeled after FreeBSD's
O_BENEATH.

[emaste]

And further, https://marc.info/?l=linux-arch&m=155002737229348

The need for some sort of control over VFS's path resolution (to avoid
malicious paths resulting in inadvertent breakouts) has been a very
long-standing desire of many userspace applications. This patchset is a
revival of Al Viro's old AT_NO_JUMPS[1,2] patchset (which was a variant
of David Drysdale's O_BENEATH patchset[3] which was a spin-off of the
Capsicum project[4]) with a few additions and changes made based on the
previous discussion within [5] as well as others I felt were useful.

eventual commit https://github.com/torvalds/linux/commit/fddb5d430ad9fa91b49b1d34d0202ffe2fa0e179

Short answer (as I understand it): Linux has neither O_BENEATH nor O_RESOLVE_BENEATH; they have another way to specify resolution flags, and there RESOLVE_BENEATH corresponds to our O_RESOLVE BENEATH

[markj]

In D28907#647025, @emaste wrote:

And further, https://marc.info/?l=linux-arch&m=155002737229348

The need for some sort of control over VFS's path resolution (to avoid
malicious paths resulting in inadvertent breakouts) has been a very
long-standing desire of many userspace applications. This patchset is a
revival of Al Viro's old AT_NO_JUMPS[1,2] patchset (which was a variant
of David Drysdale's O_BENEATH patchset[3] which was a spin-off of the
Capsicum project[4]) with a few additions and changes made based on the
previous discussion within [5] as well as others I felt were useful.

eventual commit https://github.com/torvalds/linux/commit/fddb5d430ad9fa91b49b1d34d0202ffe2fa0e179

Short answer (as I understand it): Linux has neither O_BENEATH nor O_RESOLVE_BENEATH; they have another way to specify resolution flags, and there RESOLVE_BENEATH corresponds to our O_RESOLVE BENEATH

Ok, sorry, I confused myself into thinking that they had O_BENEATH. I believe the described semantics are compatible with O_RESOLVE_BENEATH.

I'm ok with this change in principle. The capsicum test suite also needs to be updated.

[arichardson]

I wonder if we should just treat O_BENEATH/AT_BENEATH as a (deprecated) alias for the new O_RESOLVE_BENEATH/AT_RESOLVE_BENEATH? I doubt there's any code that depended on being able to leave the start dir with O_BENEATH?

[kib]

In D28907#647522, @arichardson wrote:

I wonder if we should just treat O_BENEATH/AT_BENEATH as a (deprecated) alias for the new O_RESOLVE_BENEATH/AT_RESOLVE_BENEATH? I doubt there's any code that depended on being able to leave the start dir with O_BENEATH?

I would agree with the proposal, but I want to save bits in open(2) flags. It is too wasteful to leave a bit for legacy purposes, which formally should be never needed if we remove O_BENEATH before 13.0.

[arichardson]

In D28907#647543, @kib wrote:

In D28907#647522, @arichardson wrote:

I wonder if we should just treat O_BENEATH/AT_BENEATH as a (deprecated) alias for the new O_RESOLVE_BENEATH/AT_RESOLVE_BENEATH? I doubt there's any code that depended on being able to leave the start dir with O_BENEATH?

I would agree with the proposal, but I want to save bits in open(2) flags. It is too wasteful to leave a bit for legacy purposes, which formally should be never needed if we remove O_BENEATH before 13.0.

Sorry about that, for some reason I though O_BENEATH already existed in 12. I agree that completely removing it makes more sense if it was never available in a real release.

[kib]

In D28907#647547, @arichardson wrote:

In D28907#647543, @kib wrote:

In D28907#647522, @arichardson wrote:

I wonder if we should just treat O_BENEATH/AT_BENEATH as a (deprecated) alias for the new O_RESOLVE_BENEATH/AT_RESOLVE_BENEATH? I doubt there's any code that depended on being able to leave the start dir with O_BENEATH?

I would agree with the proposal, but I want to save bits in open(2) flags. It is too wasteful to leave a bit for legacy purposes, which formally should be never needed if we remove O_BENEATH before 13.0.

Sorry about that, for some reason I though O_BENEATH already existed in 12. I agree that completely removing it makes more sense if it was never available in a real release.

It is in stable/12, but not in 12.2 I believe.

[markj]

lib/libc/sys/open.2

170 ↗

(On Diff #84581)

Is "starting directory" the right term? Resolution starts at the CWD. This might be better phrased as, "path resolution is required to not cross the directory specified by .Ar fd".

328 ↗

(On Diff #84581)

This removes the definition of "topping directory", but it is still referenced in other places.

[kib]

Remove mentions of 'topping directory'. Reformulate O_RESOLVE_BENEATH based on Mark' suggestion, but make it shorter.

[arichardson]

I'm happy to take care of the capsicum-test updates if this gets committed. No objections from me, but I believe someone else should approve this change.

[markj]

I think the bits which remove O_BENEATH/AT_BENEATH handling at the system call layer are ok. You might split that into a separate commit.

lib/libc/sys/chflags.2

341 ↗

(On Diff #84689)

Doesn't this still apply to AT_RESOLVE_BENEATH? At least open.2 still states that ENOTCAPABLE is returned in this scenario.

sys/kern/vfs_lookup.c

243 ↗

(On Diff #84689)

I'm having a hard time understanding this loop. If TAILQ_NEXT(nt, nm_link) is NULL then we will traverse the whole list. Is it intentional?

[kib]

lib/libc/sys/fhlink.2

97–98

fhlink does not take flags.

sys/kern/vfs_lookup.c

243 ↗

(On Diff #84689)

Right, I forgot about strange quirk with nt == NULL for TAILQ_FOREACH_FROM_SAFE().

Intent is to clean everything below the current entry.

[kib]

Fix bug with removal of everything from tracker on a step.

Per-commit split of the patch can be seen at
https://kib.kiev.ua/git/gitweb.cgi?p=deviant3.git;a=shortlog;h=refs/heads/beneath

[markj]

lib/libc/sys/access.2

124 ↗

(On Diff #84821)

While here, it should be "walk" instead of "walks", same as "only operate" in the deleted text. This applies to the other man pages as well.

lib/libc/sys/chmod.2

105 ↗

(On Diff #84821)

s/walks/walk/, "the directory"

sys/kern/vfs_lookup.c

408 ↗

(On Diff #84821)

Shouldn't this be ENOTCAPABLE? The man page does not really clarify since it describes ENOTCAPABLE only in the context of relative paths, but at least the capsicum test suite expects ENOTCAPABLE for O_BENEATH here.

sys/sys/fcntl.h

138 ↗

(On Diff #84821)

It seems weird to include O_UNUSED1 in the namespace. Should this line perhaps be commented out? Ditto for AT_UNUSED1.

[kib]

Grammar in man pages.
Change EINVAL to ENOTCAPABLE for abs path in O_RESOLVE_BENEATH.
Comment out O_UNUSED1/AT_UNUSED1.

[arichardson]

Commit message should probably include PR: 249960?