Details

Reviewers

emaste
allanjude
wblock
ed
rwatson

Group Reviewers

manpages

Commits

rS306537: open.2: Document Capsicum behavior

Summary

Document open(2) and openat(2) behavior in Capsicum capability mode.

Test Plan

igor clean

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 5379
Build 5577: CI src build	Jenkins
Build 5576: arc lint + arc unit

Event Timeline

cem updated this revision to Diff 20473.Sep 19 2016, 3:39 AM

cem retitled this revision from to Capsicumify open.2.

cem updated this object.

cem edited the test plan for this revision. (Show Details)

cem added reviewers: allanjude, emaste, wblock, rwatson.

Herald added a reviewer: manpages. · View Herald TranscriptSep 19 2016, 3:39 AM

Herald added a subscriber: imp. · View Herald Transcript

allanjude added inline comments.Sep 19 2016, 3:42 AM

lib/libc/sys/open.2
105	might be good to mention that it is a DIRECTORY descriptor

cem added inline comments.Sep 19 2016, 3:58 AM

lib/libc/sys/open.2
105	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	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	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	Ah, got it. Then disregard my comment. :-)

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

lib/libc/sys/open.2
105	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

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	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	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	I wonder if an explicit .Xr to capsicum(4) might be appropriate here?
112	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.