Page MenuHomeFreeBSD

Capsicumize uudecode/b64decode
Needs ReviewPublic

Authored by allanjude on Aug 26 2016, 4:28 AM.

Details

Summary

Special handling for -o, open the fd before entering the sandbox, then set the mode on it later (the intended mode is specified in the input)

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 14616
Build 14748: arc lint + arc unit

Event Timeline

allanjude updated this revision to Diff 19711.Aug 26 2016, 4:28 AM
allanjude retitled this revision from to Capsicumize uudecode/b64decode.
allanjude updated this object.
allanjude edited the test plan for this revision. (Show Details)
allanjude added reviewers: emaste, oshogbo.
allanjude added inline comments.Aug 26 2016, 4:29 AM
usr.bin/uudecode/uudecode.c
308

missing space

335

leftover debugging

oshogbo edited edge metadata.Aug 26 2016, 5:22 AM

Some comments.
Later in a day I will review this more ;)

usr.bin/uudecode/uudecode.c
141

This can fail.

160

malloc can fail.

162

strdup as well.

170

This can be done in oneline.

allanjude updated this revision to Diff 19805.Aug 29 2016, 11:33 PM
allanjude edited edge metadata.

Updated with feedback from oshogbo

bapt accepted this revision.Sep 18 2016, 5:43 PM
bapt edited edge metadata.
This revision is now accepted and ready to land.Sep 18 2016, 5:43 PM
cem edited edge metadata.Sep 18 2016, 5:53 PM

Just some high level comments. I think if the diff is smaller, it's easier to review the rest.

usr.bin/uudecode/uudecode.c
85–87

Why have this at all?

We keep a dirfd around which can open outfile anyway — no need to open the file before entering sandbox.

It duplicates the safe-output-file checking logic below, except for potential time-of-check vs time-of-use attacks. IMO, this whole function can be removed.

145–146

I think in general we should preopen an array of fds rather than FILE objects, then fdopen as needed. (The concern is that the overhead of FILE objects vs fds is meaningful.)

One potential cost is the size of the stdio buffering. I guess I would be a little shocked if FILE prefetched file contents or allocated much memory on open. But even so a FILE is bulkier than just an fd.

194–208

I think you could avoid a lot of unnecessary code churn of this patch by setting globals infile, infp, (as is the existing pattern in this code) rather than passing these things as parameters.

allanjude updated this revision to Diff 21338.Oct 13 2016, 2:44 AM
allanjude edited edge metadata.

Use new capsicum_helpers.h

Greatly reduce the diff as suggested by cem

Address a bug where input is provided via stdin

This revision now requires review to proceed.Oct 13 2016, 2:44 AM
allanjude added inline comments.Oct 13 2016, 2:45 AM
usr.bin/uudecode/uudecode.c
170

should be place limits on this directory FD?

336

do we need to limit FDs opened within the sandbox?

cem added inline comments.Oct 13 2016, 3:16 AM
usr.bin/uudecode/uudecode.c
170

Yep. Whatever rights you want openated fds to have, plus CAP_LOOKUP.

336

I think openat from a restricted dirfd inherits dirfd's restrictions. Not 100% sure of this.

Even if so, maybe child fds don't need CAP_LOOKUP.

cem added a comment.Oct 13 2016, 3:18 AM

LGTM. I'd like to see restriction on cwdfd and openated child fds (if they don't inherit restrictions).

usr.bin/uudecode/uudecode.c
170

And rights for fstatat and unlinkat.

This revision is now accepted and ready to land.Jun 12 2017, 7:22 PM
allanjude updated this revision to Diff 38552.Jan 27 2018, 11:01 PM

Update to latest -current for @bkidney_briankidney.ca

This revision now requires review to proceed.Jan 27 2018, 11:01 PM