Page MenuHomeFreeBSD

Enter capability mode and limit descriptors in elfdump(1)
ClosedPublic

Authored by brueffer on Oct 25 2014, 5:32 PM.

Diff Detail

Repository
rS FreeBSD src repository
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

brueffer updated this revision to Diff 2114.Oct 25 2014, 5:32 PM
brueffer retitled this revision from to Enter capability mode and limit descriptors in elfdump(1).
brueffer updated this object.
brueffer edited the test plan for this revision. (Show Details)
brueffer added reviewers: pjd, rwatson, eadler, jonathan.
brueffer updated this revision to Diff 2115.Oct 26 2014, 2:58 PM

Added CAP_FSTAT for the output file and trimmed whitespace.

jonathan edited edge metadata.Nov 13 2014, 9:57 PM
This comment was removed by jonathan.

This looks great. I have just a couple of inline comments:

usr.bin/elfdump/elfdump.c
493

Should cap_rights_t rights be declared earlier in main?

552

Should we also limit stdout and stderr?

Thanks for the review, I'm not sure about your two points either.

usr.bin/elfdump/elfdump.c
493

I looked for a style(7) rule for this, but couldn't find one. Any advice appreciated :-)

552

Maybe, should this always be done?

jonathan added inline comments.Nov 14 2014, 7:52 PM
usr.bin/elfdump/elfdump.c
493

My understanding is that we sort in descending order of sizeof(), then ascending alphabetical order. I also couldn't find a hard-and-fast rule in style(9), but I think this is the generally-accepted principle.

Given my relationship with style(9), however, you should take this comment with a rather large grain of salt.

552

It's a good idea to: you wouldn't want a malicious ELF binary to gain control of your terminal session (e.g., be able to send funny ioctls) when you inspect it. Such highjacking should be reserved for when you *run* a malicious binary. :)

pjd edited edge metadata.Nov 16 2014, 5:53 AM

Overall looks good to me. I do agree with Jonathan that we should also limit std descriptors. You can find example in the go_daemon() function in sbin/dhclient/dhclient.c.

usr.bin/elfdump/elfdump.c
31–32

You should always include sys/types.h first. It is good to sort kernel headers, but it is not always possible (it should always be possible for userland headers, though). sys/types.h and sys/param.h are exceptions and should always be included first (but only one of them).

brueffer updated this revision to Diff 2438.Nov 17 2014, 10:17 PM
brueffer edited edge metadata.

Moved the rights variable definition up and added limiting for stdout and stderr.

brueffer updated this revision to Diff 2439.Nov 17 2014, 10:18 PM

Removed erroneously included elf2aout diff.

brueffer updated this revision to Diff 2440.Nov 17 2014, 10:21 PM

Moved types.h inclusion before capsicum.h. Sorry for the spam!

Just a couple more inline comments. Other than these, and with the caveat that I haven't run/tested the proposed patch, I think this looks good.

usr.bin/elfdump/elfdump.c
32

This is an extremely picky point, but should we keep the newline between sys/types.h and the other sys/ headers? That is:

#include <sys/types.h>

#include <sys/capsicum.h>
#include <sys/elf32.h>
/* ... */
553

Maybe STDIN_FILENO as well? Or even better, maybe

close(STDIN_FILENO)

?

brueffer updated this revision to Diff 2447.Nov 18 2014, 10:41 AM

Added a newline after types.h and closed STDIN before entering capability mode.

Well, LGTM, as they say...

Thanks a lot, I'll wait and see if pjd is happy as well.

brueffer accepted this revision.Dec 2 2014, 5:11 PM
brueffer added a reviewer: brueffer.

Committed in r274960.

This revision is now accepted and ready to land.Dec 2 2014, 5:11 PM
brueffer closed this revision.Dec 2 2014, 5:11 PM