Page MenuHomeFreeBSD
Differential D17547

Implement O_BENEATH and AT_BENEATH
ClosedPublic
Actions

Authored by kib on Oct 13 2018, 5:19 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 23, 5:37 PM
Unknown Object (File)
Fri, Nov 22, 11:20 AM
Unknown Object (File)
Wed, Nov 20, 10:17 PM
Unknown Object (File)
Wed, Nov 20, 12:25 AM
Unknown Object (File)
Fri, Nov 15, 3:05 PM
Unknown Object (File)
Thu, Nov 7, 11:24 PM
Unknown Object (File)
Sat, Nov 2, 6:17 PM
Unknown Object (File)
Oct 17 2024, 1:26 PM
View All Files
Subscribers
0mp
allanjude
imp
jilles
jonathan
capsicum

Details

Reviewers
emaste
brooks
0mp
jilles
Group Reviewers
manpages
Commits
rS339748: Implement O_BENEATH and AT_BENEATH.

Diff Detail

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

Event Timeline

kib created this revision.Oct 13 2018, 5:19 PM
Herald added a reviewer: manpages. · View Herald TranscriptOct 13 2018, 5:19 PM
Herald added a subscriber: imp. · View Herald Transcript
Harbormaster completed remote builds in B20177: Diff 49099.Oct 13 2018, 5:19 PM
allanjude added a subscriber: allanjude.Oct 13 2018, 5:24 PM
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.

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.

Harbormaster completed remote builds in B20178: Diff 49100.Oct 13 2018, 5:37 PM
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 updated this revision to Diff 49109.Oct 13 2018, 7:51 PM
kib marked 2 inline comments as done.

Wording for stat(2).

Harbormaster completed remote builds in B20182: Diff 49109.Oct 13 2018, 7:51 PM
emaste added a comment.Oct 13 2018, 9:40 PM

Compare with D2808

kib added a comment.Oct 13 2018, 10:00 PM
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.

emaste added a subscriber: jonathan.Oct 13 2018, 10:21 PM
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
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.

0mp accepted this revision as: 0mp.Oct 17 2018, 9:32 AM
jilles added a comment.Oct 19 2018, 3:23 PM
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 updated this revision to Diff 49320.Oct 19 2018, 7:54 PM

Disallow absolute paths for BENEATH.

Harbormaster completed remote builds in B20296: Diff 49320.Oct 19 2018, 7:54 PM
jilles accepted this revision as: jilles.Oct 20 2018, 5:13 PM
kib updated this revision to Diff 49360.Oct 20 2018, 9:12 PM

Patch all manpages.

Harbormaster completed remote builds in B20310: Diff 49360.Oct 20 2018, 9:12 PM
emaste added inline comments.Oct 20 2018, 9:37 PM
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...

jonathan added a comment.Oct 20 2018, 9:44 PM

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
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

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.Oct 20 2018, 10:35 PM

Reword man pages changes.

Harbormaster completed remote builds in B20314: Diff 49372.Oct 20 2018, 10:35 PM
0mp accepted this revision.Oct 23 2018, 10:54 AM
This revision is now accepted and ready to land.Oct 23 2018, 10:54 AM
Closed by commit rS339748: Implement O_BENEATH and AT_BENEATH. (authored by kib). · Explain WhyOct 25 2018, 10:17 PM
This revision was automatically updated to reflect the committed changes.
emaste mentioned this in D2808: Implement O_BENEATH flag for openat(2)..Nov 16 2018, 8:38 PM

Revision Contents
Changeset List

PathSize
lib/
libc/
sys/
13 lines
14 lines
14 lines
14 lines
15 lines
28 lines
18 lines
14 lines
14 lines
sys/
cddl/
compat/
opensolaris/
sys/
2 lines
compat/
cloudabi/
4 lines
linux/
8 lines
kern/
9 lines
2 lines
83 lines
4 lines
sys/
11 lines
1 line
4 lines
ufs/
ffs/
2 lines

Diff 49372

View Options

lib/libc/sys/access.2

Loading...
View Options

lib/libc/sys/chflags.2

Loading...
View Options

lib/libc/sys/chmod.2

Loading...
View Options

lib/libc/sys/chown.2

Loading...
View Options

lib/libc/sys/link.2

Loading...
View Options

lib/libc/sys/open.2

Loading...
View Options

lib/libc/sys/stat.2

Loading...
View Options

lib/libc/sys/unlink.2

Loading...
View Options

lib/libc/sys/utimensat.2

Loading...
View Options

sys/cddl/compat/opensolaris/sys/vnode.h

Loading...
View Options

sys/compat/cloudabi/cloudabi_file.c

Loading...
View Options

sys/compat/linux/linux_file.c

Loading...
View Options

sys/kern/vfs_lookup.c

Loading...
View Options

sys/kern/vfs_mountroot.c

Loading...
View Options

sys/kern/vfs_syscalls.c

Loading...
View Options

sys/kern/vfs_vnops.c

Loading...
View Options

sys/sys/fcntl.h

Loading...
View Options

sys/sys/namei.h

Loading...
View Options

sys/sys/syscallsubr.h

Loading...
View Options

sys/ufs/ffs/ffs_alloc.c

Loading...