Details

lib/libc/sys/open.2
105 ↗	(On Diff #20473)	That's covered on line 79-80. In this paragraph, I just want to highlight the difference in behavior in the capability mode sandbox.

ed requested changes to this revision.Sep 19 2016, 5:23 AM

ed added a reviewer: ed.

ed added a subscriber: ed.

ed added inline comments.

lib/libc/sys/open.2
108 ↗	(On Diff #20473)	I'm not sure this is true. I thought the actual rules were: Paths must be relative. Absolute paths are rejected immediately. During resolution, the path must stay below the directory at all times. "a/../b/../c/d/../.." is okay. "a/../../b" is not.

This revision now requires changes to proceed.Sep 19 2016, 5:23 AM

cem added inline comments.Sep 19 2016, 5:27 AM

lib/libc/sys/open.2
108 ↗	(On Diff #20473)	https://github.com/freebsd/freebsd/blob/master/sys/kern/vfs_lookup.c#L619-L644 I don't think ".." is allowed at all.

ed added inline comments.Sep 20 2016, 7:08 AM

lib/libc/sys/open.2
108 ↗	(On Diff #20473)	Ah, got it. Then disregard my comment. :-)

wblock added inline comments.Sep 23 2016, 4:03 PM

lib/libc/sys/open.2
105 ↗	(On Diff #20473)	It's hard to tell what the quote marks mean. They kind of look sarcastic, and might give the opposite of the intended meaning. But the intended meaning is not clear. Maybe remove the quotes and be more specific about the term, or use .Xr to refer to a definition.

cem added inline comments.Sep 23 2016, 4:30 PM

lib/libc/sys/open.2

105 ↗

(On Diff #20473)

This is a technical document; they're not sarcasm quotes.

They refer to a specific clause with identical quotation in the source code. There's no definition elsewhere in .Xr.

201 #ifdef CAPABILITY_MODE
202         /*
203          * In capability mode, lookups must be "strictly relative" (i.e.
204          * not an absolute path, and not containing '..' components) to
205          * a real file descriptor, not the pseudo-descriptor AT_FDCWD.
206          */

cem added inline comments.Sep 23 2016, 4:31 PM

lib/libc/sys/open.2
105 ↗	(On Diff #20473)	Maybe just removing the quotes is a good solution?

wblock added inline comments.Sep 30 2016, 2:35 PM

lib/libc/sys/open.2
105 ↗	(On Diff #20473)	And possibly add a reference to the source file: must be strictly relative to a real file descriptor .Fa fd , as defined in .Pa sys/kern/vfs_lookup.c .

In general, this seems like a good idea. A bit of wordsmithing does help, and reviewing an updated commit candidate before it goes into the tree wouldn't hurt if you can tolerate another RTT with reviewers :-).

lib/libc/sys/open.2
98 ↗	(On Diff #20473)	I wonder if an explicit .Xr to capsicum(4) might be appropriate here?
112 ↗	(On Diff #20473)	I'm not sure we need to mention bogus values? People who pass bogus values to openat(2) deserve failure modes. Just to confirm the above: no use of "..", either in the string passed, nor in symbolic links, is permitted in paths looked up in capability mode.