Page MenuHomeFreeBSD

Capsiciumize bspatch
ClosedPublic

Authored by allanjude on Jul 29 2016, 5:14 AM.

Details

Summary

Move all of the fopen() and open() calls to the top of main()

Avoid reusing the 'fd' variable, since it is used read only, then later write only

cap_enter(), and make all except the output FD read/seek only.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

allanjude retitled this revision from to Capsiciumize bspatch.
allanjude updated this object.
allanjude edited the test plan for this revision. (Show Details)
allanjude added reviewers: cperciva, delphij, oshogbo, rwatson, ed.

Capsicum part look's fine for me. Thanks, for that.

I have a lot of issue with the style of file. But maybe we should keep it in the same way?

usr.bin/bsdiff/bspatch/bspatch.c
242 ↗(On Diff #18865)

You changing style of the file:
if ((write(wfd, new, newsize)!=newsize) || (close(wfd)==-1))

I have a lot of issue with the style of file. But maybe we should keep it in the same way?

Well, the file is already a mix and match of style(9) and non-style(9). My suggestion would be to fix up any code to be properly formatted if you're changing it anyway.

Allan: Thanks for working on this!

usr.bin/bsdiff/bspatch/bspatch.c
38 ↗(On Diff #18865)

Could you move this above the other includes? Everything for <sys/*> should go before <*>.

102 ↗(On Diff #18865)

Please add spaces between these O_* flags: O_RDONLY | O_BINARY. Same below.

Maybe just axe the O_BINARY stuff? That's only useful if we wanted to make this work on Windows.

182 ↗(On Diff #18865)

While you're modifying these lines, could you please add missing whitespace and put err() on a separate line?

242 ↗(On Diff #18865)

Which is fine, while you're at it. Could you also remove the extraneous parentheses here?

delphij requested changes to this revision.Jul 29 2016, 5:43 AM
delphij edited edge metadata.
delphij added inline comments.
usr.bin/bsdiff/bspatch/bspatch.c
93 ↗(On Diff #18865)

Why the same file needs to be opened so many times?

105 ↗(On Diff #18865)

The file should be removed if we hit an error.

108 ↗(On Diff #18865)

This means users without Capsicum kernel support would not be able to run this at all. Can't this be changed to fallback mechanisms, either refuse run as root, or chroot to /var/empty then drop all privileges?

This revision now requires changes to proceed.Jul 29 2016, 5:43 AM

People downstream bspatch in other projects. Can we ifdef this based on something?

allanjude edited edge metadata.

Updated after emaste style cleanup

Cleanup includes based on feedback from ed

usr.bin/bsdiff/bspatch/bspatch.c
105 ↗(On Diff #18865)

if there is an error on open, is the file created?

usr.bin/bsdiff/bspatch/bspatch.c
38 ↗(On Diff #19595)

I just wonder do we need to check this?
In my opinion we should include <sys/capsicum.h> and if we would merge it in feature to 10 we could change the header just in that version, right?

84 ↗(On Diff #19595)

#ifdef HAVE_CAPSICUM?

124 ↗(On Diff #19595)

You need to setuid this binary, I think, so you need as well do some setgid/setuid/setgroup.
Otherwise I think you are escalating user privileged.

148 ↗(On Diff #19595)

All that in ifdef.

emaste requested changes to this revision.Aug 23 2016, 7:18 PM
emaste edited edge metadata.
emaste added inline comments.
usr.bin/bsdiff/bspatch/bspatch.c
38 ↗(On Diff #19595)

We're upstream for this file and it may end up being copied into other projects, so there's some argument for keeping a version check.

119 ↗(On Diff #19595)

If we HAVE_CAPSICUM then cap_enter() < 0 with errno != ENOSYS should be fatal.

124 ↗(On Diff #19595)

I'd prefer that we leave this out of the initial change.

137–144 ↗(On Diff #19595)

I think this is cleaner as

if ((cap_rights_limit(fileno(f), &rights_ro) < 0 && errno != ENOSYS) ||
    (cap_rights_limit(fileno(cpf), &rights_ro) < 0 && errno != ENOSYS) ||
    (cap_rights_limit(fileno(dpf), &rights_ro) < 0 && errno != ENOSYS) ||
    (cap_rights_limit(fileno(epf), &rights_ro) < 0 && errno != ENOSYS))
        err(1, ...
270–271 ↗(On Diff #19595)

Leftover

93 ↗(On Diff #18865)

Agree that it's a bit unusual, but it's already like this and I'd rather see a minimal change to capsicumize it.

105 ↗(On Diff #18865)

If there's an error later on (e.g. malloc, BZ2_bzReadOpen etc.) the temp newfile will be left behind.

108 ↗(On Diff #18865)

In my opinion we should look at such a change in a subsequent patch.

This revision now requires changes to proceed.Aug 23 2016, 7:18 PM
oshogbo requested changes to this revision.Aug 23 2016, 7:29 PM
oshogbo edited edge metadata.

In this review we don't clear newfd on error which was pointed out by @delphij.

usr.bin/bsdiff/bspatch/bspatch.c
124 ↗(On Diff #19595)

If you mean idea of chrooting then I agree. Sandboxing which chroot will add you a lot of more work. As well force all other project to suid this binary.

usr.bin/bsdiff/bspatch/bspatch.c
124 ↗(On Diff #19595)

Yes, leave anything to do with chroot out of this change.

We have capsicum in FreeBSD, and I don't want to risk introducing other errors by developing the chroot, suid, etc. support in our tree at the same time. IMO that work would best be developed in a standalone repository, but it could be brought back when done to simplify ongoing maintenance.

105 ↗(On Diff #18865)

That said, I'd be happy with the capsicumization going in now, and addressing the leftover file in a subsequent commit.

usr.bin/bsdiff/bspatch/bspatch.c
124 ↗(On Diff #19595)

the plan with chroot was to just emit the warning and NOT do it if run as root. Definitely do NOT want to suid this binary.

Will leave the chroot part out for now

202 ↗(On Diff #19595)

also leftover

allanjude edited edge metadata.

Updated with feedback from emaste and oshobo

usr.bin/bsdiff/bspatch/bspatch.c
31 ↗(On Diff #19602)

need to add #include <sys/param.h> to pick up __FreeBSD_version

122 ↗(On Diff #19602)

engage

allanjude edited edge metadata.

Fix typo, add sys/param.h

usr.bin/bsdiff/bspatch/bspatch.c
126 ↗(On Diff #19608)

In my opinion we should not inform user about that.
He did not compile capsicum in to the system, it was his decision we should not inform go on every step that he don't have sandbox. He already made his choices.

The same case for platforms that are not build with CAPABILITIEY per default (like arm), there is no point to inform about that.

usr.bin/bsdiff/bspatch/bspatch.c
30 ↗(On Diff #19613)

it could go in the #if defined(__FreeBSD__) block if you like since it's not needed on !FreeBSD

119 ↗(On Diff #19613)

Overly long line?

emaste edited edge metadata.

LGTM with two more style issues

usr.bin/bsdiff/bspatch/bspatch.c
116 ↗(On Diff #19651)

space after ,

120 ↗(On Diff #19651)

space after ,

LGTM in general, thanks for working on this one. Please fix the two style issues pointed out by Ed when committing.

emaste accepted this revision.
emaste accepted this revision.
This revision was automatically updated to reflect the committed changes.