Page MenuHomeFreeBSD
Differential D7358

Capsiciumize bspatch
ClosedPublic
Actions

Authored by allanjude on Jul 29 2016, 5:14 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Dec 10, 2:17 PM
Unknown Object (File)
Mon, Dec 9, 1:15 PM
Unknown Object (File)
Sat, Dec 7, 1:58 PM
Unknown Object (File)
Thu, Dec 5, 3:01 AM
Unknown Object (File)
Thu, Nov 28, 4:32 AM
Unknown Object (File)
Nov 21 2024, 10:56 AM
Unknown Object (File)
Nov 17 2024, 12:55 PM
Unknown Object (File)
Nov 5 2024, 3:46 AM
View All Files
Subscribers
imp
lifanov_mail.lifanov.com

Details

Reviewers
emaste
delphij
cperciva
rwatson
oshogbo
Commits
rS304807: Capsicumize bspatch
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 updated this revision to Diff 18865.Jul 29 2016, 5:14 AM
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.
Herald added a subscriber: imp. · View Herald TranscriptJul 29 2016, 5:14 AM
oshogbo edited edge metadata.EditedJul 29 2016, 5:35 AM

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

ed edited edge metadata.Jul 29 2016, 5:43 AM
In D7358#152989, @oshogbo wrote:

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
lifanov_mail.lifanov.com added a subscriber: lifanov_mail.lifanov.com.Jul 29 2016, 7:10 PM

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

ed removed a reviewer: ed.Aug 14 2016, 12:56 PM
emaste mentioned this in D7603: capsicum bspatch(1).Aug 23 2016, 12:17 PM
emaste mentioned this in D7610: bspatch: apply style(9).Aug 23 2016, 12:36 PM
allanjude updated this revision to Diff 19594.Aug 23 2016, 6:39 PM
allanjude edited edge metadata.

Updated after emaste style cleanup

allanjude added a reviewer: emaste.Aug 23 2016, 6:39 PM
allanjude updated this revision to Diff 19595.Aug 23 2016, 6:40 PM

Cleanup includes based on feedback from ed

allanjude added inline comments.Aug 23 2016, 6:42 PM
usr.bin/bsdiff/bspatch/bspatch.c
119

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

oshogbo added inline comments.Aug 23 2016, 7:01 PM
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.

emaste added inline comments.Aug 23 2016, 7:38 PM
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.

allanjude added inline comments.Aug 24 2016, 1:14 AM
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 updated this revision to Diff 19602.Aug 24 2016, 1:18 AM
allanjude edited edge metadata.

Updated with feedback from emaste and oshobo

emaste added inline comments.Aug 24 2016, 3:13 AM
usr.bin/bsdiff/bspatch/bspatch.c
31

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

124

engage

allanjude updated this revision to Diff 19608.Aug 24 2016, 3:38 AM
allanjude edited edge metadata.

Fix typo, add sys/param.h

oshogbo added inline comments.Aug 24 2016, 5:38 AM
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.

allanjude updated this revision to Diff 19613.Aug 24 2016, 5:54 AM

Address oshogbo feedback

emaste added inline comments.Aug 24 2016, 1:13 PM
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?

allanjude updated this revision to Diff 19651.Aug 24 2016, 10:52 PM

Shorten long line

emaste accepted this revision.Aug 25 2016, 2:11 PM
emaste edited edge metadata.

LGTM with two more style issues

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

space after ,

121

space after ,

delphij added a comment.Aug 25 2016, 2:30 PM

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.Aug 25 2016, 2:44 PM
emaste accepted this revision.
emaste accepted this revision.
Closed by commit rS304807: Capsicumize bspatch (authored by allanjude). · Explain WhyAug 25 2016, 3:08 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
usr.bin/
bsdiff/
bspatch/
64 lines

Diff 19613

View Options

usr.bin/bsdiff/bspatch/bspatch.c

Loading...