Page MenuHomeFreeBSD

Capsiciumize bspatch
ClosedPublic

Authored by allanjude on Jul 29 2016, 5:14 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Mar 10, 4:02 PM
Unknown Object (File)
Sun, Mar 10, 4:02 PM
Unknown Object (File)
Sun, Mar 10, 4:02 PM
Unknown Object (File)
Sun, Mar 10, 4:02 PM
Unknown Object (File)
Sun, Mar 10, 4:02 PM
Unknown Object (File)
Sun, Mar 10, 4:02 PM
Unknown Object (File)
Sun, Mar 10, 4:02 PM
Unknown Object (File)
Feb 23 2024, 9:11 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 Passed
Unit
No Test Coverage
Build Status
Buildable 4909
Build 4972: 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
51

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

116

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.

195

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
107

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

119

The file should be removed if we hit an error.

122

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
119

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

usr.bin/bsdiff/bspatch/bspatch.c
38

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?

86

#ifdef HAVE_CAPSICUM?

128

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.

152

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

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.

107

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

119

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

122

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

123

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

128

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

141–148

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
128

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
119

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

128

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
128

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

124

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
30

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

119

Overly long line?

emaste edited edge metadata.

LGTM with two more style issues

usr.bin/bsdiff/bspatch/bspatch.c
117

space after ,

121

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.