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
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 4908
Build 4971: arc lint + arc unit

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
262–263

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
50

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

115

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.

196

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

262–263

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
106

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

118

The file should be removed if we hit an error.

121

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
118

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

usr.bin/bsdiff/bspatch/bspatch.c
37

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?

85

#ifdef HAVE_CAPSICUM?

127

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.

151

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
37

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.

106

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

118

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

121

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

122

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

127

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

140–147

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, ...
262–263

Leftover

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
127

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
118

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

127

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.

usr.bin/bsdiff/bspatch/bspatch.c
127

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

195–196

also leftover

allanjude edited edge metadata.

Updated with feedback from emaste and oshobo

usr.bin/bsdiff/bspatch/bspatch.c
30

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

123

engage

allanjude edited edge metadata.

Fix typo, add sys/param.h

usr.bin/bsdiff/bspatch/bspatch.c
126

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
29

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

118

Overly long line?

emaste edited edge metadata.

LGTM with two more style issues

usr.bin/bsdiff/bspatch/bspatch.c
116

space after ,

120

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.