G407: Incorrect detection of fixed iv #1211

imirkin · 2024-09-04T14:58:04Z

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?

The text was updated successfully, but these errors were encountered:

ccojocar · 2024-09-04T14:59:38Z

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

ben-krieger · 2024-09-04T18:39:53Z

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 · 2024-09-04T20:43:38Z

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 · 2024-09-04T21:54:37Z

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 · 2024-09-04T22:12:10Z

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 · 2024-09-04T22:21:07Z

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 · 2024-09-05T05:18:16Z

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 · 2024-09-05T11:19:43Z

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 · 2024-09-05T11:39:13Z

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 · 2024-09-05T11:46:08Z

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 · 2024-09-05T11:58:12Z

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 · 2024-09-05T12:06:47Z

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 · 2024-09-05T12:11:00Z

@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 · 2024-09-05T12:25:23Z

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 · 2024-09-05T12:28:23Z

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 · 2024-09-05T14:50:43Z

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

## What kind of change does this PR introduce? Fix to gosec warnings so builds can complete. ## What is the current behavior? The gosec checks are halting builds. ## What is the new behavior? The gosec checks are passing. ## Additional context I didn't see any warnings that led to real vulnerabilities / security issues. That said long term it may be worth adding some defensive bounds checks for a couple of the integer overflow warnings, just to future proof us age the code ages. Given that we allow supabase users to write to the database, not sure we can guarantee a user doesn't provide a bring-your-own-hash singup flow or something like that. Unbound allocations are a prime target for DoS attacks. For the nonce issues, neither is was real issue. Open is not "fixable, see gosec issue [#1211](securego/gosec#1211). For Seal I tried: ``` nonce := make([]byte, cipher.NonceSize()) if _, err := rand.Read(nonce); err != nil { panic(err) } es := EncryptedString{ KeyID: keyID, Algorithm: "aes-gcm-hkdf", Nonce: nonce, } es.Data = cipher.Seal(nil, es.Nonce, data, nil) ``` But it then considers es.Nonce to be stored / hardcoded. The only fix I could get to work was: ```Go nonce := make([]byte, cipher.NonceSize()) if _, err := rand.Read(nonce); err != nil { panic(err) } es := EncryptedString{ KeyID: keyID, Algorithm: "aes-gcm-hkdf", Nonce: nonce, Data: cipher.Seal(nil, nonce, data, nil), } ``` It seems the gosec tool requires using `rand.Read`. I changed the `cipher.NonceSize()` back to `12` (just in case it a numerical constant for a reason) and it started failing again. I think it also checks that cipher.NonceSize() is used as well, just doesn't report that. I ultimately decided to ignore this so there was no changes to crypto functions given the existing code is correct. Co-authored-by: Chris Stockton <[email protected]>

securego/gosec#1211 securego/gosec#1209

qzio · 2024-10-15T19:32:47Z

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()

…e#1770) ## What kind of change does this PR introduce? Fix to gosec warnings so builds can complete. ## What is the current behavior? The gosec checks are halting builds. ## What is the new behavior? The gosec checks are passing. ## Additional context I didn't see any warnings that led to real vulnerabilities / security issues. That said long term it may be worth adding some defensive bounds checks for a couple of the integer overflow warnings, just to future proof us age the code ages. Given that we allow supabase users to write to the database, not sure we can guarantee a user doesn't provide a bring-your-own-hash singup flow or something like that. Unbound allocations are a prime target for DoS attacks. For the nonce issues, neither is was real issue. Open is not "fixable, see gosec issue [supabase#1211](securego/gosec#1211). For Seal I tried: ``` nonce := make([]byte, cipher.NonceSize()) if _, err := rand.Read(nonce); err != nil { panic(err) } es := EncryptedString{ KeyID: keyID, Algorithm: "aes-gcm-hkdf", Nonce: nonce, } es.Data = cipher.Seal(nil, es.Nonce, data, nil) ``` But it then considers es.Nonce to be stored / hardcoded. The only fix I could get to work was: ```Go nonce := make([]byte, cipher.NonceSize()) if _, err := rand.Read(nonce); err != nil { panic(err) } es := EncryptedString{ KeyID: keyID, Algorithm: "aes-gcm-hkdf", Nonce: nonce, Data: cipher.Seal(nil, nonce, data, nil), } ``` It seems the gosec tool requires using `rand.Read`. I changed the `cipher.NonceSize()` back to `12` (just in case it a numerical constant for a reason) and it started failing again. I think it also checks that cipher.NonceSize() is used as well, just doesn't report that. I ultimately decided to ignore this so there was no changes to crypto functions given the existing code is correct. Co-authored-by: Chris Stockton <[email protected]>

ccojocar · 2024-11-26T08:35:04Z

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

ccojocar · 2024-11-26T14:56:43Z

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 · 2024-12-10T14:22:13Z

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

ldez mentioned this issue Sep 4, 2024

gosec: disable G407 golangci/golangci-lint#4983

Merged

ccojocar added the bug label Sep 5, 2024

ccojocar mentioned this issue Sep 5, 2024

Add new rule G407 to detect hard-coded nonce and initialization vectors in crypto algorithms #1197

Merged

imirkin mentioned this issue Sep 5, 2024

G407: requires unique nonce for Open? #1209

Closed

cstockton mentioned this issue Sep 6, 2024

chore: fix gosec warnings via ignore annotations in comments supabase/auth#1770

Merged

redaLaanait mentioned this issue Sep 8, 2024

Feat: add support for tokenization ln80/pii#15

Merged

pilinux added a commit to pilinux/crypt that referenced this issue Oct 1, 2024

gosec G407 [false-positive]

536f776

securego/gosec#1211 securego/gosec#1209

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

G407: Incorrect detection of fixed iv #1211

G407: Incorrect detection of fixed iv #1211

imirkin commented Sep 4, 2024

ccojocar commented Sep 4, 2024

ben-krieger commented Sep 4, 2024 •

edited

Loading

imirkin commented Sep 4, 2024

ben-krieger commented Sep 4, 2024

imirkin commented Sep 4, 2024

ben-krieger commented Sep 4, 2024

Guilder1333 commented Sep 5, 2024 •

edited

Loading

expp121 commented Sep 5, 2024

expp121 commented Sep 5, 2024

imirkin commented Sep 5, 2024 •

edited

Loading

expp121 commented Sep 5, 2024

imirkin commented Sep 5, 2024

expp121 commented Sep 5, 2024 •

edited

Loading

ccojocar commented Sep 5, 2024

imirkin commented Sep 5, 2024

imirkin commented Sep 5, 2024 •

edited

Loading

qzio commented Oct 15, 2024 •

edited

Loading

ccojocar commented Nov 26, 2024

ccojocar commented Nov 26, 2024

expp121 commented Dec 10, 2024

G407: Incorrect detection of fixed iv #1211

G407: Incorrect detection of fixed iv #1211

Comments

imirkin commented Sep 4, 2024

ccojocar commented Sep 4, 2024

ben-krieger commented Sep 4, 2024 • edited Loading

imirkin commented Sep 4, 2024

ben-krieger commented Sep 4, 2024

imirkin commented Sep 4, 2024

ben-krieger commented Sep 4, 2024

Guilder1333 commented Sep 5, 2024 • edited Loading

expp121 commented Sep 5, 2024

expp121 commented Sep 5, 2024

imirkin commented Sep 5, 2024 • edited Loading

expp121 commented Sep 5, 2024

imirkin commented Sep 5, 2024

expp121 commented Sep 5, 2024 • edited Loading

ccojocar commented Sep 5, 2024

imirkin commented Sep 5, 2024

imirkin commented Sep 5, 2024 • edited Loading

qzio commented Oct 15, 2024 • edited Loading

ccojocar commented Nov 26, 2024

ccojocar commented Nov 26, 2024

expp121 commented Dec 10, 2024

ben-krieger commented Sep 4, 2024 •

edited

Loading

Guilder1333 commented Sep 5, 2024 •

edited

Loading

imirkin commented Sep 5, 2024 •

edited

Loading

expp121 commented Sep 5, 2024 •

edited

Loading

imirkin commented Sep 5, 2024 •

edited

Loading

qzio commented Oct 15, 2024 •

edited

Loading