Page MenuHomeFreeBSD

Implement O_BENEATH and AT_BENEATH
ClosedPublic

Authored by kib on Oct 13 2018, 5:19 PM.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kib created this revision.Oct 13 2018, 5:19 PM
allanjude added inline comments.
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 added a comment.Oct 13 2018, 5:33 PM

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

kib updated this revision to Diff 49100.Oct 13 2018, 5:36 PM

Reword O_BENEATH explanation.

0mp requested changes to this revision.Oct 13 2018, 7:31 PM
0mp added a subscriber: 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/

This revision now requires changes to proceed.Oct 13 2018, 7:31 PM
kib updated this revision to Diff 49109.Oct 13 2018, 7:51 PM
kib marked 2 inline comments as done.

Wording for stat(2).

kib added a comment.Oct 13 2018, 10:00 PM

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 added a subscriber: jilles.Oct 16 2018, 10:53 PM

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 added a comment.Oct 17 2018, 9:30 AM

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.

0mp accepted this revision as: 0mp.Oct 17 2018, 9:32 AM
In D17547#375528, @kib 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 updated this revision to Diff 49320.Fri, Oct 19, 7:54 PM

Disallow absolute paths for BENEATH.

jilles accepted this revision as: jilles.Sat, Oct 20, 5:13 PM
kib updated this revision to Diff 49360.Sat, Oct 20, 9:12 PM

Patch all manpages.

emaste added inline comments.Sat, Oct 20, 9:37 PM
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...

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 marked 6 inline comments as done.Sat, Oct 20, 10:34 PM

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 updated this revision to Diff 49372.Sat, Oct 20, 10:35 PM

Reword man pages changes.

0mp accepted this revision.Tue, Oct 23, 10:54 AM
This revision is now accepted and ready to land.Tue, Oct 23, 10:54 AM
This revision was automatically updated to reflect the committed changes.