-
Notifications
You must be signed in to change notification settings - Fork 9
refactor(rules): deduplicate hardcoded-secret regex (closes #274) #277
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
Changes from 6 commits
719b001
4da90db
7d25b94
1ef8152
c91f715
f2ed293
85957e0
96b9aee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,7 @@ | ||
| use crate::impl_rule; | ||
| use crate::rules::common::{is_secret_value_long_enough, make_finding, walk_tree}; | ||
| use crate::rules::common::{ | ||
| is_secret_value_long_enough, make_finding, walk_tree, CSHARP_HARDCODED_SECRET_PATTERN, | ||
| }; | ||
| use crate::{Language, Severity}; | ||
| use regex::Regex; | ||
|
|
||
|
|
@@ -443,10 +445,7 @@ impl_rule! { | |
| fn check(_self, source, tree) { | ||
|
|
||
| let mut findings = Vec::new(); | ||
| let secret_pattern = Regex::new( | ||
| r"(?i)(password|secret|api_?key|apikey|token|credential|private_?key|connection_?string|connectionstring)", | ||
| ) | ||
| .unwrap(); | ||
| let secret_pattern = Regex::new(CSHARP_HARDCODED_SECRET_PATTERN).unwrap(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The old inline C# pattern was |
||
|
|
||
| walk_tree(tree.root_node(), source, &mut |node, src| { | ||
| // variable_declarator: string password = "hardcoded" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,8 @@ struct SeedEntry { | |
|
|
||
| const MANIFEST_PQ_CWE: &str = "CWE-327"; | ||
| const MANIFEST_PQ_DESC: &str = "Dependency uses quantum-vulnerable cryptographic algorithm"; | ||
|
Comment on lines
15
to
17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The PR title and description are scoped to deduplicating Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||
| const CARGO_PQ_DESC: &str = | ||
| "Dependency uses quantum-vulnerable cryptographic algorithm (dev-dependencies not distinguished)"; | ||
| const MANIFEST_PQ_DEADLINE: &str = "2033"; | ||
|
|
||
| /// Apply shared PQ fields to a manifest finding. | ||
|
|
@@ -61,6 +63,26 @@ const CARGO_SEEDS: &[SeedEntry] = &[ | |
| crypto_algorithm: Some("ECDSA"), | ||
| confidence: 0.9, | ||
| }, | ||
| SeedEntry { | ||
| name: "k256", | ||
| crypto_algorithm: Some("ECDSA"), | ||
| confidence: 0.9, | ||
| }, | ||
| SeedEntry { | ||
| name: "secp256k1", | ||
| crypto_algorithm: Some("ECDSA"), | ||
| confidence: 0.9, | ||
| }, | ||
| SeedEntry { | ||
| name: "libsecp256k1", | ||
| crypto_algorithm: Some("ECDSA"), | ||
| confidence: 0.9, | ||
| }, | ||
| SeedEntry { | ||
| name: "ed448-goldilocks", | ||
| crypto_algorithm: Some("Ed448"), | ||
| confidence: 0.9, | ||
| }, | ||
| // Tier 2 — confidence 0.6, mixed algorithms | ||
| SeedEntry { | ||
| name: "ring", | ||
|
|
@@ -72,6 +94,11 @@ const CARGO_SEEDS: &[SeedEntry] = &[ | |
| crypto_algorithm: None, | ||
| confidence: 0.6, | ||
| }, | ||
| SeedEntry { | ||
| name: "openssl", | ||
| crypto_algorithm: None, | ||
| confidence: 0.6, | ||
| }, | ||
| SeedEntry { | ||
| name: "aws-lc-rs", | ||
| crypto_algorithm: None, | ||
|
|
@@ -112,11 +139,36 @@ const PIP_PACKAGES: &[SeedEntry] = &[ | |
| crypto_algorithm: Some("RSA"), | ||
| confidence: 0.8, | ||
| }, | ||
| SeedEntry { | ||
| name: "pyjwt", | ||
| crypto_algorithm: None, | ||
| confidence: 0.8, | ||
| }, | ||
| SeedEntry { | ||
| name: "authlib", | ||
| crypto_algorithm: None, | ||
| confidence: 0.8, | ||
| }, | ||
| SeedEntry { | ||
| name: "python-jose", | ||
| crypto_algorithm: None, | ||
| confidence: 0.8, | ||
| }, | ||
| SeedEntry { | ||
| name: "jwcrypto", | ||
| crypto_algorithm: None, | ||
| confidence: 0.8, | ||
| }, | ||
| SeedEntry { | ||
| name: "fabric", | ||
| crypto_algorithm: Some("RSA"), | ||
| crypto_algorithm: None, | ||
| confidence: 0.7, | ||
| }, | ||
| SeedEntry { | ||
| name: "m2crypto", | ||
| crypto_algorithm: None, | ||
| confidence: 0.6, | ||
| }, | ||
| SeedEntry { | ||
| name: "cryptography", | ||
| crypto_algorithm: None, | ||
|
|
@@ -154,7 +206,7 @@ impl Rule for CargoLockPqCrypto { | |
| Some(MANIFEST_PQ_CWE) | ||
| } | ||
| fn description(&self) -> &str { | ||
| MANIFEST_PQ_DESC | ||
| CARGO_PQ_DESC | ||
| } | ||
| fn language(&self) -> Language { | ||
| Language::Manifest | ||
|
|
@@ -241,7 +293,7 @@ impl Rule for CargoLockPqCrypto { | |
| visited.insert(i); | ||
| queue.push_back(i); | ||
|
|
||
| let mut reached_seeds: Vec<&SeedEntry> = Vec::new(); | ||
| let mut reached_seeds: HashMap<&str, &SeedEntry> = HashMap::new(); | ||
|
|
||
| while let Some(node) = queue.pop_front() { | ||
| for &neighbor in &graph[node] { | ||
|
|
@@ -255,7 +307,14 @@ impl Rule for CargoLockPqCrypto { | |
| continue; | ||
| }; | ||
| if let Some(entry) = seed_map.get(neighbor_name) { | ||
| reached_seeds.push(entry); | ||
| reached_seeds | ||
| .entry(entry.name) | ||
| .and_modify(|existing| { | ||
| if entry.confidence > existing.confidence { | ||
| *existing = entry; | ||
| } | ||
| }) | ||
| .or_insert(entry); | ||
| } else { | ||
| queue.push_back(neighbor); | ||
| } | ||
|
|
@@ -268,7 +327,7 @@ impl Rule for CargoLockPqCrypto { | |
|
|
||
| // Pick the highest-confidence seed | ||
| let best = reached_seeds | ||
| .iter() | ||
| .values() | ||
| .max_by(|a, b| a.confidence.total_cmp(&b.confidence)) | ||
| .unwrap(); | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
|
|
@@ -818,4 +877,45 @@ version = \"0.17.0\"\n"; | |
| let tree = dummy_tree(""); | ||
| assert!(RequirementsTxtPqCrypto.check("", &tree).is_empty()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn cargo_k256_seed_flagged() { | ||
| let src = "\ | ||
| [[package]]\n\ | ||
| name = \"wallet\"\n\ | ||
| version = \"0.1.0\"\n\ | ||
| dependencies = [\"k256\"]\n\ | ||
| \n\ | ||
| [[package]]\n\ | ||
| name = \"k256\"\n\ | ||
| version = \"0.13.0\"\n"; | ||
| let tree = dummy_tree(src); | ||
| let findings = CargoLockPqCrypto.check(src, &tree); | ||
| assert_eq!(findings.len(), 1); | ||
| assert_eq!(findings[0].dep_name.as_deref(), Some("wallet")); | ||
| assert_eq!(findings[0].crypto_algorithm.as_deref(), Some("ECDSA")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn pip_jwt_lib_flagged() { | ||
| let src = "pyjwt>=2.0\n"; | ||
| let tree = dummy_tree(src); | ||
| let findings = RequirementsTxtPqCrypto.check(src, &tree); | ||
| assert_eq!(findings.len(), 1); | ||
| assert_eq!(findings[0].dep_name.as_deref(), Some("pyjwt")); | ||
| assert!(findings[0].crypto_algorithm.is_none()); | ||
| assert_eq!(findings[0].confidence, 0.8); | ||
| } | ||
|
|
||
| #[test] | ||
| fn pip_fabric_no_algorithm() { | ||
| let src = "fabric>=3.0\n"; | ||
| let tree = dummy_tree(src); | ||
| let findings = RequirementsTxtPqCrypto.check(src, &tree); | ||
| assert_eq!(findings.len(), 1); | ||
| assert!( | ||
| findings[0].crypto_algorithm.is_none(), | ||
| "fabric should not attribute RSA" | ||
| ); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
connectionstringalternativeconnection_?string(with_?making the underscore optional) already matchesconnectionstring, so the explicitconnectionstringalternative at the end is redundant. This was present in the originalcsharp.rsinline pattern too, but now that it's being lifted into a shared constant it's worth cleaning up.