Limit descriptors and enter capability mode in elf2aout(1).
elf2aout(1) is only used during the sparc64 build to convert the boot1
ELF binary to a.out, so it's not in any way critical.
Submitted by: brueffer (earlier version)
Details
Details
CEM: I don't have any sparc binaries to test this on, but I've only added capabilities in this change from brueffer@'s version.
I verified on flame that the a.out binary before and after this patch is identical.
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 5227 Build 5373: CI src build Jenkins Build 5372: arc lint + arc unit
Event Timeline
| elf2aout.c | ||
|---|---|---|
| 31 ↗ | (On Diff #2773) | why the extra space? |
Comment Actions
LGTM (assuming the tabs, etc. aren't actually what Differential shows!)
| elf2aout.c | ||
|---|---|---|
| 31 ↗ | (On Diff #2773) | The comment in D1009 was about keeping consistency with existing style by not deleting a line. In this case, consistency with the existing code would require inconsistency with elfdump. :) I don't have a very strong opinion on the rightness of the blank line (though I generally always err on the side of "more whitespace", against most interpretations of style(9)), but consistency is probably king. |
| elf2aout.c | ||
|---|---|---|
| 31 ↗ | (On Diff #2773) | Actually, there was no blank line in elfdump either :-) https://svnweb.freebsd.org/base/head/usr.bin/elfdump/elfdump.c?r1=270304&r2=274960 I'll remove the blank line here in this case, to keep with the local style. |
Comment Actions
In general, I think this is a very good change; just the one query about CAP_FSTAT on 'fd'.
| usr.bin/elf2aout/elf2aout.c | ||
|---|---|---|
| 97 | Below, for the output descriptor, only CAP_WRITE is required. I'm not sure the CAP_FSTAT here is needed here? Or might CAP_FSTAT be needed below? | |
| usr.bin/elf2aout/elf2aout.c | ||
|---|---|---|
| 116 | I'm actually slightly surprised that err() and friends don't require CAP_FSTAT here, in fact .. but haven't checked. The stdio stack does like to fstat() its descriptors, I thought? | |
Comment Actions
You're right, CAP_FSTAT on fd wasn't needed. I think it was a leftover from elfdump.
I verified with ktrace that CAP_FSTAT isn't needed for the err() and errx() calls either.
| usr.bin/elf2aout/elf2aout.c | ||
|---|---|---|
| 115 | Leaving {stdin, stdout, stderr} in a closed state always makes me nervous, as a later operation in libc might perform I/O. I wonder if it would be better to simply set capability rights on it to 0 and leave fd 0 open? | |
Comment Actions
kib was vehemently opposed, so I left it for now. Maybe something to discuss at BSDCan.
Comment Actions
Well, with all respect to kib, I disagree. We don't need to have a fully-worked-out attack vector before we add security features to our tools, and I think that the ELF format is one of those things that we should treat with some suspicion.
With regards to the "code bloat" comment, I don't remember this diff being particularly large, but it seems to have gone away in the recent Phabricator troubles. Would you mind re-uploading the diff?
Comment Actions
Hi Jon, I'll have time this weekend (Essen devsummit) to have a look at all this again. Thanks for the interest!
Comment Actions
elf2aout is only used during the sparc64 build to convert the boot1 ELF binary to a.out
I was going to ask, does anything still use a.out and can we just drop the program instead. :)
Comment Actions
I don't care too much about the stdin thing, as long as nothing else in the program tries to use stdin after that point. If you don't feel like auditing the whole program for use (and why would you?), I think rwatson's zero-capability suggestion is a decent compromise.
| usr.bin/elf2aout/elf2aout.c | ||
|---|---|---|
| 116 | I expect stdout and stderr need CAP_FSTAT. Maybe even CAP_IOCTL + the isatty(3) ioctl, if you care about output line buffering. | |
Comment Actions
@cem I have zero time to work in this for the next few weeks, if you're interested feel free to pick this up (same goes for the jot(1) review).
Comment Actions
Yes, it's still inevitable for the sparc64 boot1 as the latter has to be either a.out or FCode.
Comment Actions
- Replace default output fd with STDOUT rather than STDIN.
- Replace fclose(stdin) with CAP_NONE() restriction.
- Add FSTAT capability for stdout/err streams.
| usr.bin/elf2aout/elf2aout.c | ||
|---|---|---|
| 91 | This is the output fd. Did this program even work without -o? | |
Comment Actions
Not related to the code...
Can we teach the build to use ld to output a.out? Then again,it sin't that big a hunk of code...
Comment Actions
Can we teach the build to use ld to output a.out?
We can probably make GNU ld do that, but LLD won't.
Comment Actions
It seems like this will (or could conceivably) be built as a build tool, and would need a __FreeBSD_version check and #ifdef HAVE_CAPSICUM
Comment Actions
Why? We only support building CURRENT from stable/11 and CURRENT, both of which have Capsicum. (So does stable/10.)
Comment Actions
No, we support building current from 9.1 and later. Since this is a build tool for sparc, we have to allow that. However, Ed and I have been talking about a -legacy solution which would nop most of the cap stuff when the host system is too old. The last stable branch rule hasn't been true for 15 years: there's enough support that people fix building older stable systems due to large deployments at FreeBSD using companies.