Page MenuHomeFreeBSD

Allow absolute paths for O_BENEATH.
ClosedPublic

Authored by kib on Fri, Oct 26, 2:41 PM.

Details

Summary

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

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.Fri, Oct 26, 2:41 PM
jilles requested changes to this revision.Sat, Oct 27, 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.Sat, Oct 27, 10:29 AM
kib updated this revision to Diff 49694.Sat, Oct 27, 4:41 PM

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.Sat, Oct 27, 4:54 PM
kib added inline comments.
lib/libc/sys/open.2
144 ↗(On Diff #49694)

escaping, will fix with the next patch upload.

jilles accepted this revision as: jilles.Sun, Oct 28, 10:22 PM

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 updated this revision to Diff 49747.Mon, Oct 29, 6:22 AM
kib marked 8 inline comments as done.

Grammar fixes.

0mp requested changes to this revision.Mon, Oct 29, 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.Mon, Oct 29, 9:52 AM
kib marked 2 inline comments as done.Mon, Oct 29, 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...

kib added a comment.Mon, Oct 29, 2:19 PM

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.

kib added a comment.Fri, Nov 2, 12:55 AM

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

kib updated this revision to Diff 50009.Sun, Nov 4, 10:03 PM

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

jonathan added inline comments.Mon, Nov 5, 2:21 PM
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.Mon, Nov 5, 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.

emaste added inline comments.Tue, Nov 6, 8:36 PM
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."

kib updated this revision to Diff 50138.Wed, Nov 7, 8:38 PM

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

brooks added a subscriber: brooks.Thu, Nov 8, 6:15 PM
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.

kib added a comment.Thu, Nov 8, 6:20 PM
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 ('/').

brooks added a comment.Thu, Nov 8, 6:40 PM
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.

kib added a comment.Thu, Nov 8, 7:01 PM

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.

emaste added a comment.Thu, Nov 8, 7:08 PM
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.Sun, Nov 11, 12:04 AM
Closed by commit rS340343: Allow absolute paths for O_BENEATH. (authored by kib, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.
kib reopened this revision.Sun, Nov 11, 12:11 AM
This revision was not accepted when it landed; it landed in state Needs Review.Sun, Nov 11, 1:47 AM
This revision was automatically updated to reflect the committed changes.