Page MenuHomeFreeBSD

Allow absolute paths for O_BENEATH.
ClosedPublic

Authored by kib on Oct 26 2018, 2:41 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 29, 3:06 AM
Unknown Object (File)
Mon, Nov 25, 8:33 PM
Unknown Object (File)
Mon, Nov 25, 2:02 AM
Unknown Object (File)
Sat, Nov 23, 10:37 AM
Unknown Object (File)
Fri, Nov 22, 3:38 AM
Unknown Object (File)
Thu, Nov 21, 1:41 PM
Unknown Object (File)
Thu, Nov 21, 1:41 PM
Unknown Object (File)
Thu, Nov 21, 1:41 PM
Subscribers

Details

Summary

See open(2) change for explanation.
Text for *at(2) pages is postponed.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jilles requested changes to this revision.Oct 27 2018, 10:29 AM
jilles added inline comments.
lib/libc/sys/open.2
287–290 ↗(On Diff #49654)

Note that this means that the sentence "The openat() function is equivalent to the open() function except in the case where the path specifies a relative path." is no longer true if O_BENEATH is specified. So the change to the man pages will need to be more extensive, describing that .Fa fd is always used when O_BENEATH is given.

This revision now requires changes to proceed.Oct 27 2018, 10:29 AM

Man page updates.
Document absolute paths interaction with dirfd for O_BENEATH.
Correct cap mode restrictions for dotdots, and mention sysctls.

kib marked an inline comment as done.Oct 27 2018, 4:54 PM
kib added inline comments.
lib/libc/sys/open.2
144 ↗(On Diff #49694)

escaping, will fix with the next patch upload.

The man page seems to contain the right content now. I just have some English language comments.

lib/libc/sys/open.2
104–112 ↗(On Diff #49694)

Perhaps this is better as

When
.Fn openat
is called with an absolute
.Fa path
without the
.Dv O_BENEATH
flag, it ignores the
.Fa fd
argument
115 ↗(On Diff #49694)

is specified with an absolute

139 ↗(On Diff #49694)

which cause the path resolution to escape the directory hierarchy?

149 ↗(On Diff #49694)

If the

153 ↗(On Diff #49694)

in capability mode

155 ↗(On Diff #49694)

are completely disabled

156 ↗(On Diff #49694)

If the

kib marked 8 inline comments as done.

Grammar fixes.

0mp requested changes to this revision.Oct 29 2018, 9:52 AM
0mp added a subscriber: 0mp.

FWIW, mandoc -Tlint and igor report 5 more messages but they are not really related to this revision.

lib/libc/sys/open.2
31 ↗(On Diff #49747)

Remember to bump the date.

139 ↗(On Diff #49747)

Typo: hierarhy.

151 ↗(On Diff #49747)

I think 2 is a typo here: sysctl(2) does not exist.

This revision now requires changes to proceed.Oct 29 2018, 9:52 AM
kib marked 2 inline comments as done.Oct 29 2018, 10:20 AM
kib added inline comments.
lib/libc/sys/open.2
151 ↗(On Diff #49747)

sysctl is actually sysctl(2), in the sense that it is a syscall, not just some library function. I think it is put into section 3 because other functions are put into the same man page.

Just to be clear: this change only affects the use of O_BENEATH when not in capability mode, right? We wouldn't want to allow absolute path information (e.g., "is FD X somewhere under the path /foo/bar/baz/wibble?") to leak when a process is in capability mode...

Just to be clear: this change only affects the use of O_BENEATH when not in capability mode, right? We wouldn't want to allow absolute path information (e.g., "is FD X somewhere under the path /foo/bar/baz/wibble?") to leak when a process is in capability mode...

Yes. I should have no change on the capability mode operations, BENEATH is mutually exclusive with it. You can see it in namei_handle_root(), first we check that the NI_LCF_STRICTRELATIVE flag is not set (AKA cap mode), then consider BENEATH.

Ping. If nobody comments/objects in 2-3 days, I am going to ask for stress2 run and commit this. Thanks.

fstatat(2) page update. This will be copied/pasted into all other *at(2) pages before commit.

lib/libc/sys/stat.2
104 ↗(On Diff #50009)

"addition" -> "additional"?

156 ↗(On Diff #50009)

I'm not entirely sure what this is saying... isn't the root directory the starting point for the resolution? Are you trying to say something like:

When AT_BENEATH is specified and path is absolute, path must pass through the directory specified by fd

?

kib marked 2 inline comments as done.Nov 5 2018, 2:39 PM
kib added inline comments.
lib/libc/sys/stat.2
104 ↗(On Diff #50009)

Will fix in the next patch upload.

156 ↗(On Diff #50009)

No.

First, please look at the starting point definition in open(2). I tend to agree that the term is confusing since its meaning is taken by intuition. Second, no, it is not enough that the path pass through the directory, its tail after the last entry to the directory must not escape the hierarchy.

lib/libc/sys/open.2
78 ↗(On Diff #50009)

I think we could use a more extensive rewrite of this in light of O_BENEATH but don't have a good suggestion at the moment. We can always iterate on it in HEAD.

lib/libc/sys/stat.2
156 ↗(On Diff #50009)

Yes, the trouble is the intuitive meaning of "starting point."

Fabricate the 'topping directory' term instead of 'starting directory'.

In D17714#382267, @kib wrote:

Fabricate the 'topping directory' term instead of 'starting directory'.

I'd tend to call it a 'root directory' since that's what we call the base of a sub-tree in other contexts.

In D17714#382267, @kib wrote:

Fabricate the 'topping directory' term instead of 'starting directory'.

I'd tend to call it a 'root directory' since that's what we call the base of a sub-tree in other contexts.

Cannot agree, root directory already has the defined meaning ('/').

In D17714#382524, @kib wrote:
In D17714#382267, @kib wrote:

Fabricate the 'topping directory' term instead of 'starting directory'.

I'd tend to call it a 'root directory' since that's what we call the base of a sub-tree in other contexts.

Cannot agree, root directory already has the defined meaning ('/').

I think the modest overloading is better than making up words, but don't feel strongly.

OK, I am concerned with the code rotting, and do not want to hold it up for the man page changes. I am going to ask Peter for test and will commit only the code for now.

In D17714#382558, @kib wrote:

OK, I am concerned with the code rotting, and do not want to hold it up for the man page changes. I am going to ask Peter for test and will commit only the code for now.

I also want to avoid the code rotting. Please commit with the current version of the man page and we can discuss the name for this directory after commit.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 11 2018, 12:04 AM
This revision was automatically updated to reflect the committed changes.
This revision was not accepted when it landed; it landed in state Needs Review.Nov 11 2018, 1:47 AM
This revision was automatically updated to reflect the committed changes.