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

Comments

@FiloSottile
Copy link
Member

commented Jun 13, 2018

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.

@nodo

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2018

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

@FiloSottile

This comment has been minimized.

Copy link
Member Author

commented Jun 25, 2018

@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

This comment has been minimized.

@FiloSottile

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2018

@nodo

This comment has been minimized.

Copy link
Contributor

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
Projects
None yet
3 participants
You can’t perform that action at this time.