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: https://github.com/securego/gosec/issues/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 ?

[imirkin]

For all uses, I think it's reasonable to check if the SSA value is not an "immediate" or "const" (not sure which nomenclature is used in Go's SSA). i.e. []byte{1,2,3} or []byte("foo"). I don't think this is ever valid, and when it is, warrants an explicit exception (e.g. I have some data which was generated using a 0 IV always, so I'd have to use a fixed IV to decrypt ... I think it's fine to warn in that case.)

For AEAD.Seal, NewCBCEncrypter, NewCFBEncrypter it's reasonable to check that the IV is filled with random data. Not sure how hard that is. This is also a slightly more dubious check since the random data could reasonably be passed into the encryption function, which would be beyond any SSA analysis. But at least it's a check that could make sense, and it'd be reasonable to recommend against the pattern (i.e. generate the nonce at encryption time and return it separately rather than getting it passed in).

[expp121]

@imirkin thank you for the recommendation, will see what can I do!

Edit: If possible, can you provide me with more examples in which you want something to be flagged and something that you don't want to be flagged. It would be nice if you could do that!

[ccojocar]

Some crytpo algorithms store the nonce with the cipher-text. Maybe we should split the encryption from decryption part first to reduce the noise, and run the check only on the encryption methods where is more critical to have a fresh random nonce.

I think to have a precise judgement of the origin of the data, we need taint analysis to really understand from where the random data is originating (e.g. secure random function, file etc). I think we should only flag when we are sure that the nonse is coming from an hardocded source, and we can find that source in the program.

[imirkin]

Not flagged (note that I'm skimping on error checking, so perhaps it'll be flagged for that...):

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

func Encrypt(data []byte, key [32]byte) []byte {
    block, _ := aes.NewCipher(key[:32])
    gcm, _ := cipher.NewGCM(block)
    nonce := make([]byte, gcm.NonceSize())
    io.ReadFull(rand.Reader, nonce)
    return gcm.Seal(nonce, nonce, data, nil)
}

and similar variants for the other encrypt/decrypt functions.

Both of these should be flagged:

func Decrypt(data []byte, key [32]byte) ([]byte, error) {
    block, _ := aes.NewCipher(key[:32])
    gcm, _ := cipher.NewGCM(block)
    nonce := make([]byte, gcm.NonceSize()) // 0-initialized
    return gcm.Open(nil, nonce, data[gcm.NonceSize():], nil)
}

func Encrypt(data []byte, key [32]byte) []byte {
    block, _ := aes.NewCipher(key[:32])
    gcm, _ := cipher.NewGCM(block)
    nonce := make([]byte, gcm.NonceSize()) // 0-initialized
    return gcm.Seal(nonce, nonce, data, nil)
}

and other variants of making nonce with hard-coded data.

And ideally this would be flagged too:

func Encrypt(data []byte, nonce []byte, key [32]byte) []byte {
    block, _ := aes.NewCipher(key[:32])
    gcm, _ := cipher.NewGCM(block)
    return gcm.Seal(data[:0], nonce, data, nil)
}

since it's trying to encrypt with a non-random nonce.

[imirkin]

@ccojocar Perhaps makes sense to split this into two separate warnings -- one about using a provably non-random IV (i.e. directly initialized with fixed values), and another one about using a non-provably random IV for encryption?

[qzio]

Not flagged (note that I'm skimping on error checking, so perhaps it'll be flagged for that...):

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

func Encrypt(data []byte, key [32]byte) []byte {
    block, _ := aes.NewCipher(key[:32])
    gcm, _ := cipher.NewGCM(block)
    nonce := make([]byte, gcm.NonceSize())
    io.ReadFull(rand.Reader, nonce)
    return gcm.Seal(nonce, nonce, data, nil)
}

The gcm.Open(..) are still flagged.

I had nonce := make([]byte, NonceSize) and this was flagged, but not nonce := make([]byte, gcm.NonceSize()) even though the const NonceSize == gcm.NonceSize()

[ccojocar]

@expp121 Did you have a chance to look at this an try to improve it?

[ccojocar]

I removed all the decryption functions/methods from the check.

It only makes sense to flag an issue when the rule finds an hardcoded value in the code. I think if the value is provided as an input via an option or read from an external source, it is not worth to flag it, since I think will generate too much noise. We need to make sure that the rule is narrowed down only to the real hardcoded values.

[expp121]

@expp121 Did you have a chance to look at this an try to improve it?

Sorry, haven't done any work on that. Might give it a look in the future!