Page MenuHomeFreeBSD

Implement O_BENEATH and AT_BENEATH
ClosedPublic

Authored by kib on Oct 13 2018, 5:19 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 19, 1:23 AM
Unknown Object (File)
Tue, Mar 19, 1:23 AM
Unknown Object (File)
Tue, Mar 19, 1:23 AM
Unknown Object (File)
Tue, Mar 19, 1:23 AM
Unknown Object (File)
Tue, Mar 19, 1:23 AM
Unknown Object (File)
Tue, Mar 19, 1:23 AM
Unknown Object (File)
Tue, Mar 19, 1:23 AM
Unknown Object (File)
Tue, Mar 19, 1:23 AM

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 20314

Event Timeline

allanjude added inline comments.
lib/libc/sys/open.2
273

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.

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

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

How about:

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

109

s/man/manual/

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

Wording for stat(2).

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.

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.

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.

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.

Disallow absolute paths for BENEATH.

lib/libc/sys/access.2
124

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

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

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

200

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

see comment from access.2

308

see comment from access.2

lib/libc/sys/open.2
145

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

sys/kern/vfs_lookup.c
245

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.Oct 20 2018, 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

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.

Reword man pages changes.

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