Try to not leave secret data on the stack or heap. #53

gmaxwell commented at 2:08 PM on August 14, 2014: contributor

This makes a basic effort and has not been audited. Doesn't appear to have a measurable performance impact on bench.

It also adds a secp256k1_num_free to secp256k1_ecdsa_pubkey_create.

Try to not leave secret data on the stack or heap.

This makes a basic effort and has not been audited.
Doesn't appear to have a measurable performance impact on bench.

It also adds a secp256k1_num_free to secp256k1_ecdsa_pubkey_create.

2f6c801911

gmaxwell commented at 2:09 PM on August 14, 2014: contributor

If we're going to go through the trouble to do this, I should probably at least audit it with the valgrind instrumentation trick.

voisine commented at 4:15 PM on August 14, 2014: contributor

I'm curious what your opinion is of the openssl_cleanse strategy of attempting to overwrite memory with data in a way that can't be optimized out by a compiler.

gmaxwell commented at 4:45 PM on August 14, 2014: contributor

No amount of cleanse functions no matter how constructed can guarantee you against optimization: The compiler is perfectly permitted to spill your secret data into memory used for something else and then not bother to clean that up.

Any attempt is at best best effort and has to be weighed against the costs. I suppose it would be reasonable to use an assembly cleans loop any place we already have platform assembly— otherwise I'd say it wouldn't be worth the tooling costs. Otherwise just making sure the function is in another compilation unit would be adequate until the next round of compiler intelligence boosts. :)

I did spot-check that these changes weren't being optimized out for me with whatever compiler settings I was using. I think some day we'll be able to get a CI setup working that can detect leaks.

I suppose I should have clarified the attack model I was going for here, I'm mostly attempting to mitigate the effect of some external data leak in external code dumping the stack on a call immediately after a private key operation. Right now the private keys immediately show up in the uninitialized memory of the first array in the next call after a pubkey create or a sign.

sipa commented at 6:18 PM on August 14, 2014: contributor

So the value you're setting cleared group elements to is not a valid element. That's useful for debugginf purposes (it will presumably mean faster failure if you're using such cleared group elements), however maybe you want to set it to the point at infinity to make it harder to distinguish from used values?

sipa commented at 3:27 PM on August 17, 2014: contributor

Any use case in which the message would be considered secret data?

gmaxwell commented at 3:30 PM on August 17, 2014: contributor

Yea, I wasn't sure there, — the message is always a hash too (or you are using DSA wrong and are subject to the usual doom when using DSA wrong :) ), which makes leaks related to the message even less interesting.

in src/ecmult_impl.h:None in 2f6c801911

 198 | +        bits = secp256k1_num_shift(&n, 4);
 199 |          for (int k=0; k<sizeof(secp256k1_ge_t); k++)
 200 |              ((unsigned char*)(&add))[k] = c->prec[j][k][bits];
 201 |          secp256k1_gej_add_ge(r, r, &add);
 202 |      }
 203 | +    bits = 0;

sipa commented at 9:46 PM on August 18, 2014:

Is bits secret data...?

gmaxwell commented at 10:58 PM on August 18, 2014:

At the end of the loop its effectively the least significant few bits of the scalar we're multiplying by. Your call, considering the probability that this assignment just gets optimized out, maybe its not worth the slight uglification of the loop. (Though we could later change these to call a clean function on &bits, to at least avoid the optimization)

An alternative approach to these changes would be to figure out our peak stack usage and have the entrypoints that handle secret data call the interior function, then call a function that zeros that much stack.

sipa merged this on Aug 23, 2014

sipa closed this on Aug 23, 2014

sipa referenced this in commit 87c782f632 on Aug 23, 2014

real-or-random referenced this in commit 1c830b4c9a on May 31, 2019