G407: requires unique nonce for Open?

[imirkin]

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)

[ccojocar]

@expp121 please could you have a look at this? Thanks

[imirkin]

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.

[expp121]

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)

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

[imirkin]

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

1. Encrypt (generate random nonce)
2. Store (nonce used to encrypt, along with encrypted data)
3. 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.

[expp121]

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?

[imirkin]

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?

[qzio]

I'm curious if there's a test case or example on how to get past the check for gcm.Open.

func Decrypt(data []byte, key [32]byte) ([]byte, error) {
        block, err := aes.NewCipher(key[:32])
        if err != nil {
                return nil, err
        }
        gcm, err := cipher.NewGCM(block)
        if err != nil {
                return nil, err
        }
        return gcm.Open(nil, data[:gcm.NonceSize()], data[gcm.NonceSize():], nil)
}

is flagged.
usage: https://go.dev/play/p/1UFFWJjjqEZ

[imirkin]

The check is currently broken, I think it should probably be disabled until such a time as it can be reimplemented.

[ccojocar]

I agree that we should disable the rule until it gets improved.