search

sharplispers / ironclad

A cryptographic toolkit written in Common Lisp
BSD 3-Clause "New" or "Revised" License
166 stars 28 forks source link

OS-PRNG is broken under multi-threading #13

Closed eadmund closed 5 years ago

eadmund commented 5 years ago

Cf. #9, and also https://www.darkchestnut.com/2019/careful-with-ironclad-multi-threaded-applicaions/

I think that probably the best answer is to just use getrandom on Linux & BSDs. We'd have to use the FFI to call it, but I don't think that's really a problem.

If we have to avoid using the FFI, then we have two alternatives. The only reason the stream is visible across streams is because it’s kept open. If we don’t mind opening and re-opening /dev/urandom repeatedly, we could just wrap the read in WITH-OPEN-FILE. It might be better to set PRNG to NIL by default, and when it’s NIL to bind it to a fresh OS-PRNG (and hence to an opened stream) in RANDOM-DATA, RANDOM-BIT, STRONG-RANDOM &c., and that way we’d only open the file once per API call.

Slightly cleverer would be to have a different open stream for each thread. Is it okay for IRONCLAD to depend upon BORDEAUX-THREADS? If so, then I can throw something together for that. That’s still hacky, though.

This is a pretty serious issue, though: IRONCLAD should default safe.

glv2 commented 5 years ago

In fact, almost nothing in the library is thread-safe:

Depending on bordeaux-threads and putting locks everywhere could make everything thread-safe, but I'm not fond of the idea. It would slow everything down and maybe make the library unusable on Common Lisp implementations not supported by bordeaux-threads (if there are any).

I would prefer telling explicitly to the users that the prng/digest/mac/cipher objects are not thread-safe and that they have to put locks in their application where necessary if they need to use one of these objects in several threads at the same time. I put a warning in the documentation concerning PRNGs a few days ago, but I'm going to extend this warning to the other objects.

However, it's true that the OS PRNG can easily be made safer on unix-like systems by removing the source slot of the os-prng class and using with-open-file in the prng-random-data method (I have not yet checked what impact on the performance it would have). And on implementations where we are sure to have FFI available, calling getrandom seems like a good idea.

eadmund commented 5 years ago

I think the biggest difference is that *PRNG* is a global and library-created, not user-code-created.

I think we could use condition compilation to only care about thread-safety on implementations which support threads. BORDEAUX-THREADS:*DEFAULT-SPECIAL-BINDINGS* might do the trick here.

It could be as simple as:

#+thread-support(push '(crypto:*prng* . (crypto:make-prng :os)) bt:*default-special-bindings*)

Where :thread-support is pushed onto *FEATURES* by BORDEAUX-THREADS, and it in turn is conditionally depended on by IRONCLAD only on those implementations which support threads. And it doesn’t require a lock!

glv2 commented 5 years ago

Fixed in f27d6224081b33c786a1816020075477a4446782