Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 13 commits into from
Aug 30, 2024

Conversation

expp121
Copy link

@expp121 expp121 commented Aug 29, 2024

This rule looks for hard-coded nonce/ initialization vectors or variables which have hard-coded nonces/IV, that are given to multiple encryption algorithms such as: AES CBC,CTR,OFB,CFB,ASCON,CHACHA20

The rule has the following features:

  • Ability to track hard-coded args passed directly to the function
  • Ability to track variables which are assigned to hard-coded values
  • Ability to track when a variable is assigned to the result of a function, it will go and inspect the function whether it has a call to cipher/rand.Read and flag it as vulnerable if it doesn't
  • Ability to backtrack when the encryption algorithm is in another fucntion (example func cool(){ cipher.NewCTR(bloc,nonce)}

closes #1196

@ccojocar
Copy link
Member

@expp121 there are some lint errors, please could you have a look https://github.com/securego/gosec/actions/runs/10630482755/job/29469424412?pr=1197?

@expp121
Copy link
Author

expp121 commented Aug 30, 2024

@expp121 there are some lint errors, please could you have a look https://github.com/securego/gosec/actions/runs/10630482755/job/29469424412?pr=1197?

Yep, I was about to fix them! Thank you for pointing that out!

@codecov-commenter
Copy link

codecov-commenter commented Aug 30, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 82.44275% with 23 lines in your changes missing coverage. Please review.

Project coverage is 67.82%. Comparing base (4ae73c8) to head (d9eaa05).

Files with missing lines Patch % Lines
analyzers/hardcodedNonce.go 85.71% 9 Missing and 9 partials ⚠️
analyzers/util.go 0.00% 5 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1197      +/-   ##
==========================================
+ Coverage   67.35%   67.82%   +0.47%     
==========================================
  Files          74       75       +1     
  Lines        4046     4177     +131     
==========================================
+ Hits         2725     2833     +108     
- Misses       1195     1209      +14     
- Partials      126      135       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@ccojocar ccojocar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contributing this new analyzer. I left some comments, it would be great if you could address them.

analyzers/hardcodedNonce.go Outdated Show resolved Hide resolved
analyzers/hardcodedNonce.go Show resolved Hide resolved
analyzers/hardcodedNonce.go Outdated Show resolved Hide resolved
analyzers/hardcodedNonce.go Outdated Show resolved Hide resolved
analyzers/hardcodedNonce.go Outdated Show resolved Hide resolved
analyzers/hardcodedNonce.go Outdated Show resolved Hide resolved
analyzers/hardcodedNonce.go Outdated Show resolved Hide resolved
analyzers/hardcodedNonce.go Outdated Show resolved Hide resolved
analyzers/hardcodedNonce.go Outdated Show resolved Hide resolved
analyzers/hardcodedNonce.go Outdated Show resolved Hide resolved
@expp121
Copy link
Author

expp121 commented Aug 30, 2024

I wasn't sure what you meant in all of your suggestions, but if there's something else that has to be change, please feel free to inform me!

@expp121 expp121 requested a review from ccojocar August 30, 2024 12:34
@ccojocar
Copy link
Member

I wasn't sure what you meant in all of your suggestions, but if there's something else that has to be change, please feel free to inform me!

Thanks for fixing my review comments. What I meant with (*value) is that you can directly use the value.Method if you pass the value not as pointer because it is already an interface.

I pushed the change in your pull request.

@ccojocar ccojocar changed the title New rule/analyzer added G407 Add new rule G407 to detect hard-coded nonce and initialization vectors in crypto algorithms Aug 30, 2024
Dimitar Banchev and others added 13 commits August 30, 2024 17:26
The rule is supposed to detect for the usage of hardcoded or static nonce/Iv in many encryption algorithms:

* The different modes of AES (mainly tested here)
* It should be able to work with ascon

Currently the rules doesn't check when constant variables are used.

TODO: Improve the rule, to detected for constatant variable usage
* Removed old way of initializing analyzers
* Added the new analyzer to the rest of the default analyzers
* Fixed small bug in the rule
* Removed the test for the new analyzer from the file responsible for testing the rules
* Merged the diffrent examples into 1 variable
* Added tests for the analyzer
* Removed code that was used for testing rules, but it was used to test the analyzer
The value doens't require to be passed as a pointer since is a
interface.

Change-Id: Ia21bceb5f315f4c30bd28425d62f678e9203e93f
Signed-off-by: Cosmin Cojocar <[email protected]>
Change-Id: I7f42c1de4e39dceb8e8144037d5af9223331ff06
Signed-off-by: Cosmin Cojocar <[email protected]>
Change-Id: I49caeb75f1bd7ecdb9b4f99466d96ad81e2e95ac
Signed-off-by: Cosmin Cojocar <[email protected]>
Change-Id: Ifa141b70351136cfe7d0756a83e8166a24b5d538
Signed-off-by: Cosmin Cojocar <[email protected]>
Change-Id: I5b863c0da6cc3d01efa527c60c93fdcbc8c5a53c
Signed-off-by: Cosmin Cojocar <[email protected]>
@ccojocar ccojocar force-pushed the feature-new-rule-hardcoded-iv branch from 238545a to d9eaa05 Compare August 30, 2024 17:28
@ccojocar ccojocar merged commit 0898560 into securego:master Aug 30, 2024
6 checks passed
@Jeeppler
Copy link

Jeeppler commented Sep 2, 2024

@expp121 Thanks for contributing the rule and the research effort required for it 🚀.

@ccojocar Thank you for reviewing and merging it 🎉 👍 .

@ccojocar
Copy link
Member

ccojocar commented Sep 5, 2024

@expp121 Please could you have a look at these two issues #1209 and #1211 which we need to address since otherwise the analyzer is generating too many false positives. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gosec does not detect hard-coded nonces/initialization vectors for multiple encryption algorithms
4 participants