eli: Switch bzero() to explicit_bzero() for sensitive data
ClosedPublic
Actions

Authored by allanjude on Feb 26 2017, 5:40 PM.

Details

Reviewers

def
delphij
pjd
oshogbo

Group Reviewers

secteam

Commits

rS316312: sys/geom/eli: Switch bzero() to explicit_bzero() for sensitive data

Summary

In GELI, anywhere we are zeroing out possibly sensitive data, like
the metadata struct, the metadata sector (both contain the encrypted
master key), the user key, or the master key, use explicit_bzero.

Didn't touch the bzero() used to initialize structs.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

allanjude updated this revision to Diff 25713.Feb 26 2017, 5:40 PM

allanjude retitled this revision from to sys/geom/eli: Switch bzero() to explicit_bzero() for sensitive data.

allanjude updated this object.

allanjude edited the test plan for this revision. (Show Details)

allanjude added reviewers: pjd, delphij, oshogbo, def.

Herald added a subscriber: imp. · View Herald TranscriptFeb 26 2017, 5:40 PM

Look's good for me.

This revision is now accepted and ready to land.Feb 26 2017, 5:55 PM

delphij added a reviewer: secteam.Feb 27 2017, 5:17 AM

I think this is good in principle (I have commented inline for the ones that doesn't really matter, but the changes would not hurt either), so no objection here. Please consider waiting for a few more days to see if Pawel have any objections.

sys/geom/eli/g_eli_ctl.c
107 ↗	(On Diff #25713)	(No action needed) I think this doesn't really matter since the keys is invalid.
114 ↗	(On Diff #25713)	(No action needed) Note that this pattern have repeated multiple times, I'd probably abstract the cleanup into one place and have goto cleanup instead, but it's Ok as-is.
122 ↗	(On Diff #25713)	(No action needed) This won't hurt but have little effect either, because the key is wrong.
126 ↗	(On Diff #25713)	(No action needed) Ditto.
133 ↗	(On Diff #25713)	(Trivial issue) If I was you I'd move this block to line 100 and forget about zeroing. It's Okay to leave it as-is, though.
324 ↗	(On Diff #25713)	(No action needed) I think this is just initialization, it doesn't hurt to have it but the explicit zeroing doesn't help anything either, because it's not going to be optimized away. @pjd why do we need to do these zeros in the first place? (I think it's already done in line 242?)
330 ↗	(On Diff #25713)	Ditto.
552 ↗	(On Diff #25713)	(No action required) I don't think this really matters, as the key material are all in encrypted form, but again, it won't hurt either.
631 ↗	(On Diff #25713)	(Trivial) I'd move this to line 588 and remove the bzero part, as we don't really need it to make the decision.
654 ↗	(On Diff #25713)	(No action requested) This is similar to earlier change in g_eli_ctl_configure(). It won't hurt but I think it does not really matter either.
932 ↗	(On Diff #25713)	(No action requested) Similar to earlier code. Since we don't have keys, it shouldn't matter.
938 ↗	(On Diff #25713)	Maybe move this to earlier and remove the zero's?
940 ↗	(On Diff #25713)	(Unrelated) This is sorta weird way of saying "No 'key' argument", I found this a few times in this file.
1002 ↗	(On Diff #25713)	(No action requested) There is no point of doing explicit_ here, as the purpose is merely initializing the memory block that would immediately be used, and the bzero can't be optimized away.