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 4907
Build 4970: 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
260–261

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
48

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

113

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.

194

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

260–261

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
104

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

116

The file should be removed if we hit an error.

119

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
116

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?

83

#ifdef HAVE_CAPSICUM?

125

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.

149

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.

104

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

116

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

119

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

120

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

125

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

138–145

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, ...
260–261

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
125

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
116

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

125

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
125

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

193–194

also leftover

allanjude edited edge metadata.

Updated with feedback from emaste and oshobo

usr.bin/bsdiff/bspatch/bspatch.c
31

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

122

engage

allanjude edited edge metadata.

Fix typo, add sys/param.h

usr.bin/bsdiff/bspatch/bspatch.c
123

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

116

Overly long line?

emaste edited edge metadata.

LGTM with two more style issues

usr.bin/bsdiff/bspatch/bspatch.c
114

space after ,

118

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.