Skip to content
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

Open
FiloSottile opened this issue Jun 13, 2018 · 7 comments
Labels
Documentation help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@FiloSottile
Copy link
Contributor

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.

@FiloSottile FiloSottile added Documentation help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Jun 13, 2018
@FiloSottile FiloSottile added this to the Unplanned milestone Jun 13, 2018
@nodo
Copy link
Contributor

nodo commented Jun 25, 2018

Hey @FiloSottile ! Are you OK if I help with this issue? Do you have any pointers?

@FiloSottile
Copy link
Contributor Author

@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.

@nodo
Copy link
Contributor

nodo commented Jul 6, 2018

@FiloSottile
Copy link
Contributor Author

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.

@anitgandhi
Copy link
Contributor

I think the same should apply to some of the other interfaces and implementations. For example, cipher.BlockMode, and specifically the CBCEncrypter/Decrypter types, have iv as a shared state, so it's not concurrent safe. This is equivalent to the XTS tweak situation.

I can open a separate issue for this if it makes more sense to do so.

@FiloSottile
Copy link
Contributor Author

FiloSottile commented Jul 12, 2018 via email

@nodo
Copy link
Contributor

nodo commented Jul 22, 2018

@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 aes in pure go is safe for concurrent use? If that is the case I can use the same approach for other cipher.Block implementations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

3 participants