Implement O_BENEATH and AT_BENEATH

[allanjude]

lib/libc/sys/open.2

273 ↗

(On Diff #49099)

Worded slightly better:

.Dv O_BENEATH
returns
.Er ENOTCAPABLE
if the specified path, after resolving all symlinks and ".." references in the specified path,
does not reside in the directory hierarchy of children beneath the starting directory.

[kib]

I accept absolute paths for O_BENEATH. This can be tweaked, I am not sure what is the useful approach there.

[kib]

Reword O_BENEATH explanation.

[0mp]

I've got a couple of suggestions on the wording in the changes to the manual page.

lib/libc/sys/stat.2

104 ↗

(On Diff #49100)

How about:

Do not allow to stat a file which is not a child of the starting directory. See ...

109 ↗

(On Diff #49100)

s/man/manual/

[kib]

Wording for stat(2).

[emaste]

Compare with D2808

[kib]

In D17547#374591, @emaste wrote:

Compare with D2808

Did the comparision. That revision:

1. Does not accept absolute paths for O_BENEATH.
2. Returns EPERM instead of ENOTCAPABLE.
3. Does not use the dotdot tracker since the revision was written before I introduced it.
4. There is no AT_BENEATH.

I can reproduce features 1 and 2, please provide your opinions.

[jilles]

Unconditionally allowing absolute paths with O_BENEATH seems a bit strange, as I expect this to be used for partially untrusted paths. An application can easily check for absolute paths and cause either behaviour (allowing or disallowing).

Not all functions that now accept AT_BENEATH have this mentioned in their man page.

[kib]

In D17547#375474, @jilles wrote:

Unconditionally allowing absolute paths with O_BENEATH seems a bit strange, as I expect this to be used for partially untrusted paths. An application can easily check for absolute paths and cause either behaviour (allowing or disallowing).

This was one of my questions. But, it is not clear to me what change, if any, you are proposing.

Not all functions that now accept AT_BENEATH have this mentioned in their man page.

Yes, I do not want to copy/paste text from fstatat(2) until the patch is finalized.

[jilles]

In D17547#375528, @kib wrote:

In D17547#375474, @jilles wrote:

Unconditionally allowing absolute paths with O_BENEATH seems a bit strange, as I expect this to be used for partially untrusted paths. An application can easily check for absolute paths and cause either behaviour (allowing or disallowing).

This was one of my questions. But, it is not clear to me what change, if any, you are proposing.

I propose to disallow absolute paths with O_BENEATH, resulting in the [ENOTCAPABLE] error.

Not all functions that now accept AT_BENEATH have this mentioned in their man page.

Yes, I do not want to copy/paste text from fstatat(2) until the patch is finalized.

Fair enough.

[kib]

Disallow absolute paths for BENEATH.

[kib]

Patch all manpages.

[emaste]

lib/libc/sys/access.2

124 ↗

(On Diff #49360)

I would rephrase these to avoid the negative - something like "Only allow operation on a file which is a child of the starting directory."

lib/libc/sys/utimensat.2

275 ↗

(On Diff #49360)

or contained a ".." component ?

Also I think I would put the AT_BENEATH flag first (since it's the unusual case),

The AT_BENEATH flag was specified, and path was absolute or contained a ".." component...

[jonathan]

I've made a few minor suggestions about man page wording (which might reasonably apply to all of the man pages in the review). I've also made a minor suggestion about deriving LCF_STRICTRELATIVE from BENEATH rather than including lots of if (... STRICTRELATIVE || ... BENEATH).

On a more substantive point, do we set errno in the same way as Linux when openat(... O_BENEATH) fails? It might be helpful to porters if our behaviour is consistent, i.e., we don't set ENOTCAPABLE when people are expecting something like EINVAL.

lib/libc/sys/access.2

124 ↗

(On Diff #49360)

This might be clearer if it said something like, "Only operate on files and directories within the starting directory".

200 ↗

(On Diff #49360)

I might suggest:

.Dv AT_BENEATH
was specified but
.Fa path
is not strictly relative to the starting directory.
For example,
.Fa path
is absolute or includes a ".." component that escapes the starting directory.

lib/libc/sys/chflags.2

97 ↗

(On Diff #49360)

see comment from access.2

308 ↗

(On Diff #49360)

see comment from access.2

lib/libc/sys/open.2

145 ↗

(On Diff #49360)

maybe "require path to be strictly relative to starting directory"?

sys/kern/vfs_lookup.c

245 ↗

(On Diff #49360)

Would it be sensible to instead use cn_flags & BENEATH as a reason to set NI_LCF_STRICTRELATIVE? That might then avoid some of the duplicate logic...

[kib]

In D17547#376430, @jonathan wrote:

On a more substantive point, do we set errno in the same way as Linux when openat(... O_BENEATH) fails? It might be helpful to porters if our behaviour is consistent, i.e., we don't set ENOTCAPABLE when people are expecting something like EINVAL.

I am sure that we do not, because I have no idea how the Linux does it. An argument for ENOTCAPABLE is that this error code makes the cause for error distinguished.

If you can explain the Linux error codes, it is certainly for the discussion.

sys/kern/vfs_lookup.c

245 ↗

(On Diff #49360)

I am not completely sure what is your suggestion there, but I can provide some explanation why the current patch is structured as it is.

I ended up disabling absolute paths for O_BENEATH, according to jilles' suggestion, for the immediate commit. My plan is to allow absolute paths, assuming that the tail of the path descends into cwd and does not leave it (I hope the idea is clear, I will try to word it more comprehensibly when I implement it). This mode is only relevant for BENEATH and not for STRICTRELATIVE AKA cap mode, for obvious reasons. So I want to test for BENEATH where it is actually BENEATH mode.

[kib]

Reword man pages changes.