use shared buffer for retrieving random bytes #5444
Conversation
anonrig
force-pushed
the
yagiz/buffered-entropy-source
branch
2 times, most recently
from
0c342d0 to
8d2b2b6
Compare
|
Needs tests for the new utility |
anonrig
force-pushed
the
yagiz/buffered-entropy-source
branch
2 times, most recently
from
0aeedfc to
cd9aa70
Compare
CodSpeed Performance ReportMerging #5444 will not alter performanceComparing Summary
Footnotes
|
anonrig
force-pushed
the
yagiz/buffered-entropy-source
branch
from
cd9aa70 to
d8f1256
Compare
|
This design based on This should use a Then we can replace all the This makes the change light-touch with no need to change any design around |
anonrig
force-pushed
the
yagiz/buffered-entropy-source
branch
from
d8f1256 to
4642d32
Compare
There was a problem hiding this comment.
OK, this now looks pretty good to me but I wonder if we should check in with a security person to be extra-sure we aren't doing something bad here -- random number generation problems can be catastrophic for crypto.
@mschwarzl what do you think?
anonrig
changed the title
anonrig
force-pushed
the
yagiz/buffered-entropy-source
branch
from
4642d32 to
63ec89a
Compare
| size_t toCopy = kj::min(state.data.size(), output.size()); | ||
| output.first(toCopy).copyFrom(state.data.first(toCopy)); | ||
| // Zero out the source buffer after copying to prevent sensitive data from remaining in memory | ||
| OPENSSL_cleanse(state.data.first(toCopy).begin(), toCopy); |
There was a problem hiding this comment.
Using fill(0) here works just as well for this and is simpler.
| OPENSSL_cleanse(state.data.first(toCopy).begin(), toCopy); | |
| state.data.first(toCopy).fill(0); |
There was a problem hiding this comment.
I guess the idea behind OPENSSL_cleanse() is to overwrite memory in a way that the compiler won't try to optimize away if it thinks the value will not be read again -- presumably to reduce the likelihood of the value being readable in memory dumps, or via bad pointer accesses? Unclear if fill() would necessarily do the same.
It's interesting that the value is considered sensitive immediately after a copy of it is given out, but may otherwise have been sitting in the buffer an arbitrary amount of time prior to that. Don't know if that might change the security properties of the code.
There was a problem hiding this comment.
fill(...) uses a simple for loop rather than using memset. The way it's structured the compiler shouldn't be able to optimize it away the same way it can with memset.
One thing that I think we can establish is that this should not be used at all for sensitive data like secret keys. Randomized ids and random uint64's etc are fine.
There was a problem hiding this comment.
Ah, OK... In that case, it might not even be that important to clear the buffer, but I suppose there's little downside to doing so. Probably worth noting in the doc comment (if not the function name) that the function isn't intended for sensitive use cases.
There was a problem hiding this comment.
fill(...) uses a simple for loop rather than using memset. The way it's structured the compiler shouldn't be able to optimize it away the same way it can with memset.
What difference does it make to the compiler whether its a for loop or memset? If the compiler can determine the writes will never be observed, it can optimize it out.
That said it would take some extremely complex logical reasoning for the compiler to be able to optimize out this case, so I wouldn't be too worried about it.
One thing that I think we can establish is that this should not be used at all for sensitive data like secret keys. Randomized ids and random uint64's etc are fine.
Why would we want to say that? The output of this function should work just fine for generating cryptographic secrets.
There was a problem hiding this comment.
Why would we want to say that? ...
Sure it would technically work. While FIPS 140 does not explicitly state that cached entropy sources are forbidden, the requirements are fairly clear about entropy sources needing to be live and actively reseeded. While our 4096-byte cache would, most likely, be drained quickly, it still counts as cached entropy. We'd likely want to have a security team sign-off to rely on it for cryptographic secrets.
There was a problem hiding this comment.
I agree anything FIPS-sensitive likely can't use this.
And I think we should get security team sign-off before landing this at all.
| KJ_ASSERT(RAND_bytes(reinterpret_cast<uint8_t*>(&ret), sizeof(ret)) == 1); | ||
| getEntropy(kj::asBytes(ret)); |
There was a problem hiding this comment.
Maybe worth noting that this probably will have no effect. As far as I can tell, all callers of getRandom64Bit() pass in an a non-none entropySource. We should probably just remove the parameter's Maybe-ness.
(or update callers to pass none in the appropriate places)
|
Point raised from the security team: is forking an issue here? Specifically, if the process is forked after the cache is populated, the cache for the current thread will be duplicated in the fork. We either need to (a) require that the cache is only populated after forking happens or (b) refresh the cache when forking happens. |
FWIW, since this code is most likely invoked in the sandbox, it shouldn't be forking. We avoid forking in a process that already has additional threads, since another thread could be holding locks that would never release in the new process. In the past, this has typically manifested as "interrupt" signals in tcmalloc functions. |
|
This code should be called strictly post-fork, but we should nevertheless add some debug checks to verify that the value returned by We should skip these checks in release builds as |
4 participants
Another take to #5442 which should be a lot faster since we're basically caching now.