On Wed, Oct 21, 2020 at 09:58:50PM -0700, Eric Biggers wrote: > On Tue, Oct 20, 2020 at 04:39:53PM -0400, Arvind Sankar wrote: > > The assignments to clear a through h and t1/t2 are optimized out by the > > compiler because they are unused after the assignments. > > > > These variables shouldn't be very sensitive: t1/t2 can be calculated > > from a through h, so they don't reveal any additional information. > > Knowing a through h is equivalent to knowing one 64-byte block's SHA256 > > hash (with non-standard initial value) which, assuming SHA256 is secure, > > doesn't reveal any information about the input. > > > > Signed-off-by: Arvind Sankar <nivedita@xxxxxxxxxxxx> > > I don't entirely buy the second paragraph. It could be the case that the input > is less than or equal to one SHA-256 block (64 bytes), in which case leaking > 'a' through 'h' would reveal the final SHA-256 hash if the input length is > known. And note that callers might consider either the input, the resulting > hash, or both to be sensitive information -- it depends. The "non-standard initial value" was just parenthetical -- my thinking was that revealing the hash, whether the real SHA hash or an intermediate one starting at some other initial value, shouldn't reveal the input; not that you get any additional security from being an intermediate block. But if the hash itself could be sensitive, yeah then a-h are sensitive anyway. > > > --- > > lib/crypto/sha256.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/lib/crypto/sha256.c b/lib/crypto/sha256.c > > index d43bc39ab05e..099cd11f83c1 100644 > > --- a/lib/crypto/sha256.c > > +++ b/lib/crypto/sha256.c > > @@ -202,7 +202,6 @@ static void sha256_transform(u32 *state, const u8 *input) > > state[4] += e; state[5] += f; state[6] += g; state[7] += h; > > > > /* clear any sensitive info... */ > > - a = b = c = d = e = f = g = h = t1 = t2 = 0; > > memzero_explicit(W, 64 * sizeof(u32)); > > } > > Your change itself is fine, though. As you mentioned, these assignments get > optimized out, so they weren't accomplishing anything. > > The fact is, there just isn't any way to guarantee in C code that all sensitive > variables get cleared. > > So we shouldn't (and generally don't) bother trying to clear individual u32's, > ints, etc. like this, but rather only structs and arrays, as clearing those is > more likely to work as intended. > > - Eric Ok, I'll just drop the second paragraph from the commit message then.
