G407: Incorrect detection of fixed iv

[imirkin]

The code to generate an OpenSSH-compatible encrypted private key might go something like:

	k, err := BcryptPbkdfKey([]byte(passPhrase), []byte(opts.Salt), int(opts.Rounds), 32+16)
	if err != nil {
		return err
	}
	key, iv := k[:32], k[32:]

	c, err := aes.NewCipher(key)
	if err != nil {
		return err
	}
	ctr := cipher.NewCTR(c, iv)

Where BcryptPbkdfKey is effectively golang.org/x/crypto/ssh/internal/bcrypt_pbkdf. This code generates the following warning:

G407 (CWE-1204): Use of hardcoded IV/nonce for encryption by passing hardcoded slice/array (Confidence: HIGH, Severity: HIGH)

Probably because of the assignment from k?

[ccojocar]

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

[ben-krieger]

Yeah, this analyzer doesn't really make any sense. It requires that all nonces are generated with (crypto/rand).Read. Then how could you ever use (crypto/cipher.AEAD).Open? To open, you need to use the same nonce as was used to seal, not some randomly generated data...

Edit: I just realized this was already captured in #1209.

[imirkin]

Although maybe in this instance the warning is right? The nonce/iv probably should come from a random source? In this particular case, as it happens, there's a good bit of randomness in there too, since opts.Salt comes from rand.Reader. But in the "pure" view, it's not a directly-random nonce, so this usage should just have an ignore since it's just conforming to a format that it can't change.

[ben-krieger]

Although maybe in this instance the warning is right? The nonce/iv probably should come from a random source? In this particular case, as it happens, there's a good bit of randomness in there too, since opts.Salt comes from rand.Reader. But in the "pure" view, it's not a directly-random nonce, so this usage should just have an ignore since it's just conforming to a format that it can't change.

Since bcrypt is generating a symmetric key, gosec can't tell if the nonce should be random or loaded from, say, a database. It should only be random at generation time, and even that could conceivably happen earlier than the first use. For example, even the database engine might generate the random salt and then the Go code will always just be loading it from an external source.

So it's not really possible to tell if the nonce was securely generated unless the variable is never used after initialization and is initialized from the executable's bss section or such...

[imirkin]

It should only be random at generation time, and even that could conceivably happen earlier than the first use. For example, even the database engine might generate the random salt and then the Go code will always just be loading it from an external source.

I was focused on NewCTR being used for encryption that I wasn't thinking that it'd be used for decryption. Of course -- in this case, the function should be removed from the list of checked functions.

But for cases where it's clearly used for encryption (e.g. Seal), checking a random source seems reasonable? Round-tripping via db could count as dubious and therefore worthy of warning.

[ben-krieger]

It should only be random at generation time, and even that could conceivably happen earlier than the first use. For example, even the database engine might generate the random salt and then the Go code will always just be loading it from an external source.

I was focused on NewCTR being used for encryption that I wasn't thinking that it'd be used for decryption. Of course -- in this case, the function should be removed from the list of checked functions.

But for cases where it's clearly used for encryption (e.g. Seal), checking a random source seems reasonable? Round-tripping via db could count as dubious and therefore worthy of warning.

That seems entirely reasonable to me. Maybe we can just improve the error message. If people are loading from storage, then the wording of hard-coded may trip them up and they'll think that it's a false positive.

[Guilder1333]

It should only be random at generation time, and even that could conceivably happen earlier than the first use. For example, even the database engine might generate the random salt and then the Go code will always just be loading it from an external source.

I was focused on NewCTR being used for encryption that I wasn't thinking that it'd be used for decryption. Of course -- in this case, the function should be removed from the list of checked functions.
But for cases where it's clearly used for encryption (e.g. Seal), checking a random source seems reasonable? Round-tripping via db could count as dubious and therefore worthy of warning.

That seems entirely reasonable to me. Maybe we can just improve the error message. If people are loading from storage, then the wording of hard-coded may trip them up and they'll think that it's a false positive.

But it is a false positive, as it also reacts to functions that are required to get IVs from some storage.
As in this issue: #1209 (which we also have in our project right now).

[expp121]

The code to generate an OpenSSH-compatible encrypted private key might go something like:

	k, err := BcryptPbkdfKey([]byte(passPhrase), []byte(opts.Salt), int(opts.Rounds), 32+16)
	if err != nil {
		return err
	}
	key, iv := k[:32], k[32:]

	c, err := aes.NewCipher(key)
	if err != nil {
		return err
	}
	ctr := cipher.NewCTR(c, iv)

Where BcryptPbkdfKey is effectively golang.org/x/crypto/ssh/internal/bcrypt_pbkdf. This code generates the following warning:

G407 (CWE-1204): Use of hardcoded IV/nonce for encryption by passing hardcoded slice/array (Confidence: HIGH, Severity: HIGH)

Probably because of the assignment from k?

I completely agreed with you @imirkin, using BcryptPbkdfKey to generate nonce, should exclude it from being flagged.
That should be added to the rule.

[expp121]

Yeah, this analyzer doesn't really make any sense. It requires that all nonces are generated with (crypto/rand).Read. Then how could you ever use (crypto/cipher.AEAD).Open? To open, you need to use the same nonce as was used to seal, not some randomly generated data...

Edit: I just realized this was already captured in #1209.

It requires that all nonces are generated with (crypto/rand).Read. Right now, YES! Checks for other ways to generate random byte array should be added, I completely agree here. (As already mentioned in the comment above.)

To open, you need to use the same nonce as was used to seal, not some randomly generated data... YES!, but if the underlying ssa.Value object, that represents the argument passed to Seal function is different to the one passed to the Open a.k.a. a different variable is passed to the Open function, obviously it would require it to be random too. But if the same ssa.Value object a.k.a. the same variable is passed to both functions, then there's no problem. Because you still have to make it random for the Seal function.

In a nutshell: if you pass a different variable (different name/different memory location) with the SAME value as the one used in Seal function as nonce argument, it would require it to be random. So the solution is to use the same variable (object/memory address that holds the array) in both places.

Please correct me if I didn't get what you were saying!

[imirkin]

Nobody calls Seal + Open in the same function, other than weird examples... and all the cases being flagged here are ones where only one operation is ever done in the function.

I think it's reasonable to ensure that both encryption and decryption are not done using a fixed nonce, and it's reasonable to ensure that encryption (i.e. Seal) is done using a random nonce.

Sounds like this pass is overly eager in determining that SSA values are identical though.

[expp121]

Nobody calls Seal + Open in the same function, other than weird examples... Yeah... You are correct.

I think it's reasonable to detect that both encryption and decryption are not done using a fixed nonce, but in your case you were using stored nonces, honestly right now I don't have any solution to this problem. Do you have any solution in mind?

and it's reasonable to ensure that encryption (i.e. Seal) is done using a random nonce. - I guess for right now it's better to disable the checks for decryption. What do think ?