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)
Wed, Oct 23, 8:54 PM
Unknown Object (File)
Tue, Oct 22, 5:59 PM
Unknown Object (File)
Thu, Oct 17, 6:35 PM
Unknown Object (File)
Thu, Oct 17, 6:34 PM
Unknown Object (File)
Thu, Oct 17, 6:34 PM
Unknown Object (File)
Thu, Oct 17, 6:30 PM
Unknown Object (File)
Thu, Oct 17, 5:46 PM
Unknown Object (File)
Thu, Oct 17, 5:46 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 Skipped
Unit
Tests Skipped
Build Status
Buildable 20628

Event Timeline

jilles requested changes to this revision.Oct 27 2018, 10:29 AM
jilles added inline comments.
lib/libc/sys/open.2
327โ€“330

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

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

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

is specified with an absolute

139

which cause the path resolution to escape the directory hierarchy?

149

If the

153

in capability mode

155

are completely disabled

156

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

Remember to bump the date.

139

Typo: hierarhy.

151

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

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

"addition" -> "additional"?

156

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

Will fix in the next patch upload.

156

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

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

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
Closed by commit rS340343: Allow absolute paths for O_BENEATH. (authored by kib). ยท Explain Why
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.