Page MenuHomeFreeBSD

Capsicum support for elf2aout(1)
AbandonedPublic

Authored by cem on Dec 17 2014, 10:32 AM.

Details

Summary

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)

Test Plan

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

Event Timeline

brueffer updated this revision to Diff 2773.Dec 17 2014, 10:32 AM
brueffer retitled this revision from to Capsicum support for usr.bin/elf2aout.
brueffer updated this object.
brueffer edited the test plan for this revision. (Show Details)
emaste added inline comments.Dec 17 2014, 1:43 PM
elf2aout.c
31 ↗(On Diff #2773)

why the extra space?

brueffer added inline comments.Dec 17 2014, 2:06 PM
elf2aout.c
31 ↗(On Diff #2773)

This came up in the elfdump review (D1009), Jonathan suggested: "This is an extremely picky point, but should we keep the newline between sys/types.h and the other sys/ headers? That is:...".

jonathan edited edge metadata.Dec 17 2014, 2:19 PM

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.

brueffer added inline comments.Dec 17 2014, 2:24 PM
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.

brueffer updated this revision to Diff 2777.Dec 17 2014, 2:33 PM
brueffer edited edge metadata.

Removed a stray blank line.

rwatson edited edge metadata.Jan 5 2015, 9:09 PM

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?

rwatson added inline comments.Jan 5 2015, 9:11 PM
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?

brueffer updated this revision to Diff 3017.Jan 6 2015, 5:34 PM
brueffer edited edge metadata.

Removed an unneeded CAP_FSTAT capability.

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.

rwatson added inline comments.Jan 23 2015, 11:32 PM
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?

eadler resigned from this revision.Mar 26 2015, 3:41 AM
eadler removed a reviewer: eadler.

So... where is this patch now? @brueffer, have you committed it?

kib was vehemently opposed, so I left it for now. Maybe something to discuss at BSDCan.

Can you be more specific about kib's objections?

  1. code bloat
  2. useless in this tool (and e.g,, jot)

I think that was it.

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?

Any update on the stdin thing?

jonathan requested changes to this revision.Jul 23 2015, 2:20 PM
jonathan edited edge metadata.

This seems good to me, except for the stdin business...

This revision now requires changes to proceed.Jul 23 2015, 2:20 PM

Hi Jon, I'll have time this weekend (Essen devsummit) to have a look at all this again. Thanks for the interest!

cem added a comment.Sep 19 2016, 6:13 PM

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

cem added a comment.Sep 19 2016, 6:24 PM

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.

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

cem added a comment.Sep 19 2016, 7:18 PM

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

Sure, will do.

cem commandeered this revision.Sep 19 2016, 7:19 PM
cem added a reviewer: brueffer.

Commandeering for now. Will update today or tomorrow.

marius edited edge metadata.Sep 19 2016, 9:24 PM
In D1331#164562, @cem wrote:

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

Yes, it's still inevitable for the sparc64 boot1 as the latter has to be either a.out or FCode.

cem updated this revision to Diff 20498.Sep 19 2016, 9:32 PM
cem edited edge metadata.
cem marked 2 inline comments as done.
  • 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?

cem retitled this revision from Capsicum support for usr.bin/elf2aout to Capsicum support for elf2aout(1).Sep 19 2016, 9:34 PM
cem updated this object.
cem edited the test plan for this revision. (Show Details)
cem edited edge metadata.
imp added a comment.Sep 20 2016, 3:58 AM

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

emaste edited edge metadata.Sep 20 2016, 4:01 AM

Can we teach the build to use ld to output a.out?

We can probably make GNU ld do that, but LLD won't.

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

cem added a comment.Sep 21 2016, 8:20 PM

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

Why? We only support building CURRENT from stable/11 and CURRENT, both of which have Capsicum. (So does stable/10.)

imp added a comment.EditedSep 21 2016, 8:26 PM
In D1331#165360, @cem wrote:

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

Why? We only support building CURRENT from stable/11 and CURRENT, both of which have Capsicum. (So does stable/10.)

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.

cem abandoned this revision.Sep 21 2016, 9:15 PM

I'm not interested in adding cruft solely to support stable/9 on Sparc.