G407: requires unique nonce for Open?
Opened this issue · 6 comments
gcm.Open
takes a nonce, but it's meant to be the value passed in at Seal
time, not unique. From https://pkg.go.dev/crypto/cipher#NewGCM:
// ... The nonce must be NonceSize()
// bytes long and both it and the additional data must match the
// value passed to Seal.
However with code like
gcm.Open(nil, foo[:gcm.NonceSize()], foo[gcm.NonceSize():], nil)
, we get a warning:
G407 (CWE-1204): Use of hardcoded IV/nonce for encryption by passing hardcoded slice/array (Confidence: HIGH, Severity: HIGH)
Same issue with e.g. cipher.NewCBCDecrypter
, and I suspect others. Unless I'm thoroughly mistaken, decryption should not be using a random nonce, but rather the one that the data was encrypted with.
gcm.Open
takes a nonce, but it's meant to be the value passed in atSeal
time, not unique. From https://pkg.go.dev/crypto/cipher#NewGCM:// ... The nonce must be NonceSize() // bytes long and both it and the additional data must match the // value passed to Seal.However with code like
gcm.Open(nil, foo[:gcm.NonceSize()], foo[gcm.NonceSize():], nil)
, we get a warning:
G407 (CWE-1204): Use of hardcoded IV/nonce for encryption by passing hardcoded slice/array (Confidence: HIGH, Severity: HIGH)
@imirkin Yes, you are right that the nonce for Seal
should be the same as the one for Open
and also for the other encryption algorithms such as cipher.NewCBCDecrypter
.
But right now the rule just checks whether a variable is passed as a nonce argument to these functions
"(crypto/cipher.AEAD).Seal" "(crypto/cipher.AEAD).Open" "crypto/cipher.NewCBCDecrypter" "crypto/cipher.NewCBCEncrypter" "crypto/cipher.NewCFBDecrypter" "crypto/cipher.NewCFBEncrypter" "crypto/cipher.NewCTR" "crypto/cipher.NewOFB"
, and also whether it is hard-coded (meaning it doesn't use cipher/rand to generate the nonce [I am aware that there are other methods to generate a random nonce too, so the rule should be improved a little bit more]), it does not check whether the value passed to Open
is the same/different as the one passed to Seal
.
For example, if you have a hard-coded nonce
variable that is passed both to Open
and Seal
, it would flag both the lines where Open
and Seal
are located.
But if you don't have the same nonce
variable, and it's being randomly generated with cipher/rand
, it should not flag any of those two usages.
Another example would be if I have nonce1
and nonce2
, and nonce1
is hard-coded, nonce2
is random, it should flag only the places where nonce1
was used.
Please tell me if I didn't correctly understand what you were trying to say! And as already mentioned... I am aware that there are other ways to generate random byte array/nonce, but they are not implemented for now! Might to do something about that in the future.
OK. Perhaps "hard-coded" is unclear then? In no way is it hard-coded in my example. But it's not coming from crypto/rand -- it's coming from storage. The flow is
- Encrypt (generate random nonce)
- Store (nonce used to encrypt, along with encrypted data)
- Decrypt (must use same nonce used from storage)
So when I go to decrypt, I use the nonce from storage that was randomly generated 5 years ago... the decrypt function takes these as parameters -- just one, actually, foo in my example, as they are stored concatenated.
Okay, yeah.... if you are loading it from storage, that's a different story :D. It will flag it. You are right for this!
The rule always expects freshly generated nonce. Which as you just said is not the case for you.
What would be your suggestion then?
See my comment in #1211 (comment).
I had originally filed the two issues since I thought they were different, but they may well be the same. Let's not fork the conversation too much -- continue in #1211 since it seems to have more discussion?