Page MenuHomeFreeBSD

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

Authored by brueffer on Oct 25 2014, 5:32 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 14 2024, 8:18 AM
Unknown Object (File)
Dec 28 2023, 1:45 PM
Unknown Object (File)
Dec 19 2023, 11:29 PM
Unknown Object (File)
Dec 14 2023, 9:48 AM
Unknown Object (File)
Dec 14 2023, 9:47 AM
Unknown Object (File)
Nov 29 2023, 4:03 PM
Unknown Object (File)
Nov 13 2023, 10:19 AM
Unknown Object (File)
Nov 10 2023, 8:29 AM
Subscribers
None

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

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.

Added CAP_FSTAT for the output file and trimmed whitespace.

This comment was removed by jonathan.

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

usr.bin/elfdump/elfdump.c
492

Should cap_rights_t rights be declared earlier in main?

551

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
492

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

551

Maybe, should this always be done?

usr.bin/elfdump/elfdump.c
492

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.

551

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. :)

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 edited edge metadata.

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

Removed erroneously included elf2aout diff.

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
33

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>
/* ... */
552

Maybe STDIN_FILENO as well? Or even better, maybe

close(STDIN_FILENO)

?

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

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

brueffer added a reviewer: brueffer.

Committed in r274960.

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