New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
crypto: document which cipher.Block implementations are safe for concurrent use #25882
Comments
Hey @FiloSottile ! Are you OK if I help with this issue? Do you have any pointers? |
@nodo Absolutely! It's a matter of going through the implementations (mainly AES and DES) and checking that they don't write to object state in Encrypt / Decrypt. You can also use the race detector to help, but some implementations are in assembly, so it will not work everywhere. |
Hey @FiloSottile, I was checking the packages, would it be enough to verify the files below? |
There are a few other implementations for other platforms in those packages that you'll have to check. By the way, this is about cipher.Block, so you don't have to check the GCM mode, which is a cipher.AEAD implementation. |
I think the same should apply to some of the other interfaces and implementations. For example, I can open a separate issue for this if it makes more sense to do so. |
In general nothing is concurrent safe until explicitly marked. cipher.Block
was confusing because they mostly (all) are and that assumption had made
its way into places. If you think it would be worth explicitly documenting
other implementations, feel free to open another issue, though.
|
@FiloSottile Apologies for the delay, I have spent some time trying to figure out how things are connected together. I have had a look at AES in pure go. It looks to me that https://github.com/golang/go/blob/master/src/crypto/aes/block.go#L40 and https://github.com/golang/go/blob/master/src/crypto/aes/block.go#L85 do not access shared state and are safe for concurrent use. I have also written a quick test to be run with the race detector: https://gist.github.com/nodo/e7beb7dfa1b5495310c27f607d72cdc5 Do you think this is an acceptable proof that |
Many of them are, but since it's undocumented it might change from one architecture to the next.
This came up in https://go-review.googlesource.com/c/crypto/+/118535 because x/crypto/xts effectively assumes they all are.
The text was updated successfully, but these errors were encountered: