From c010d659c3187352814fbc059efd11b33a6da095 Mon Sep 17 00:00:00 2001 From: Maksudul Haque Date: Thu, 5 Jan 2023 22:44:12 +0600 Subject: [PATCH 1/2] [`flake8-bandit`] Add Rule for `S506` (unsafe use of yaml load) --- README.md | 1 + resources/test/fixtures/flake8_bandit/S506.py | 29 +++++++++++ ruff.schema.json | 3 ++ src/checkers/ast.rs | 11 ++++ src/flake8_bandit/checks/mod.rs | 2 + src/flake8_bandit/checks/unsafe_yaml_load.rs | 51 +++++++++++++++++++ src/flake8_bandit/mod.rs | 1 + ...f__flake8_bandit__tests__S506_S506.py.snap | 25 +++++++++ src/registry.rs | 11 ++++ src/registry_gen.rs | 11 ++++ 10 files changed, 145 insertions(+) create mode 100644 resources/test/fixtures/flake8_bandit/S506.py create mode 100644 src/flake8_bandit/checks/unsafe_yaml_load.rs create mode 100644 src/flake8_bandit/snapshots/ruff__flake8_bandit__tests__S506_S506.py.snap diff --git a/README.md b/README.md index 94f59937c21f8..85f00f96c5d47 100644 --- a/README.md +++ b/README.md @@ -771,6 +771,7 @@ For more, see [flake8-bandit](https://pypi.org/project/flake8-bandit/4.1.1/) on | S106 | HardcodedPasswordFuncArg | Possible hardcoded password: `"..."` | | | S107 | HardcodedPasswordDefault | Possible hardcoded password: `"..."` | | | S108 | HardcodedTempFile | Probable insecure usage of temp file/directory: `"..."` | | +| S506 | UnsafeYAMLLoad | Probable insecure usage of `yaml.load`: `"..."` | | ### flake8-blind-except (BLE) diff --git a/resources/test/fixtures/flake8_bandit/S506.py b/resources/test/fixtures/flake8_bandit/S506.py new file mode 100644 index 0000000000000..7712a8a7ca812 --- /dev/null +++ b/resources/test/fixtures/flake8_bandit/S506.py @@ -0,0 +1,29 @@ +import json +import yaml +from yaml import CSafeLoader +from yaml import SafeLoader +from yaml import SafeLoader as NewSafeLoader + +def test_yaml_load(): + ystr = yaml.dump({'a': 1, 'b': 2, 'c': 3}) + y = yaml.load(ystr) + yaml.dump(y) + try: + y = yaml.load(ystr, Loader=yaml.CSafeLoader) + except AttributeError: + # CSafeLoader only exists if you build yaml with LibYAML + y = yaml.load(ystr, Loader=yaml.SafeLoader) + + +def test_json_load(): + # no issue should be found + j = json.load("{}") + +yaml.load("{}", Loader=yaml.Loader) + +# no issue should be found +yaml.load("{}", SafeLoader) +yaml.load("{}", yaml.SafeLoader) +yaml.load("{}", CSafeLoader) +yaml.load("{}", yaml.CSafeLoader) +yaml.load("{}", NewSafeLoader) diff --git a/ruff.schema.json b/ruff.schema.json index 2b97da8dc75bb..01beb7171c64c 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -927,6 +927,9 @@ "S106", "S107", "S108", + "S5", + "S50", + "S506", "SIM", "SIM1", "SIM10", diff --git a/src/checkers/ast.rs b/src/checkers/ast.rs index 4eae376a12671..04ab28bc8db3e 100644 --- a/src/checkers/ast.rs +++ b/src/checkers/ast.rs @@ -1896,6 +1896,17 @@ where self.add_check(check); } } + if self.settings.enabled.contains(&CheckCode::S506) { + if let Some(check) = flake8_bandit::checks::unsafe_yaml_load( + func, + args, + keywords, + &self.from_imports, + &self.import_aliases, + ) { + self.add_check(check); + } + } if self.settings.enabled.contains(&CheckCode::S106) { self.add_checks( flake8_bandit::checks::hardcoded_password_func_arg(keywords).into_iter(), diff --git a/src/flake8_bandit/checks/mod.rs b/src/flake8_bandit/checks/mod.rs index 6a1bb66f2256f..5cd2c453a5f6b 100644 --- a/src/flake8_bandit/checks/mod.rs +++ b/src/flake8_bandit/checks/mod.rs @@ -8,6 +8,7 @@ pub use hardcoded_password_string::{ assign_hardcoded_password_string, compare_to_hardcoded_password_string, }; pub use hardcoded_tmp_directory::hardcoded_tmp_directory; +pub use unsafe_yaml_load::unsafe_yaml_load; mod assert_used; mod bad_file_permissions; @@ -17,3 +18,4 @@ mod hardcoded_password_default; mod hardcoded_password_func_arg; mod hardcoded_password_string; mod hardcoded_tmp_directory; +mod unsafe_yaml_load; diff --git a/src/flake8_bandit/checks/unsafe_yaml_load.rs b/src/flake8_bandit/checks/unsafe_yaml_load.rs new file mode 100644 index 0000000000000..d613bad1fc7f1 --- /dev/null +++ b/src/flake8_bandit/checks/unsafe_yaml_load.rs @@ -0,0 +1,51 @@ +use rustc_hash::{FxHashMap, FxHashSet}; +use rustpython_ast::{Expr, ExprKind, Keyword}; + +use crate::ast::helpers::{match_module_member, SimpleCallArgs}; +use crate::ast::types::Range; +use crate::registry::{Check, CheckKind}; + +/// S506 +pub fn unsafe_yaml_load( + func: &Expr, + args: &Vec, + keywords: &Vec, + from_imports: &FxHashMap<&str, FxHashSet<&str>>, + import_aliases: &FxHashMap<&str, &str>, +) -> Option { + if match_module_member(func, "yaml", "load", from_imports, import_aliases) { + let call_args = SimpleCallArgs::new(args, keywords); + + if let Some(loader_arg) = call_args.get_argument("Loader", Some(1)) { + if !match_module_member( + loader_arg, + "yaml", + "SafeLoader", + from_imports, + import_aliases, + ) && !match_module_member( + loader_arg, + "yaml", + "CSafeLoader", + from_imports, + import_aliases, + ) { + let loader_name = match &loader_arg.node { + ExprKind::Attribute { attr, .. } => Some(attr), + ExprKind::Name { id, .. } => Some(id), + _ => None, + }?; + return Some(Check::new( + CheckKind::UnsafeYAMLLoad(loader_name.clone()), + Range::from_located(loader_arg), + )); + } + } else { + return Some(Check::new( + CheckKind::UnsafeYAMLLoad("No Loader used".to_string()), + Range::from_located(func), + )); + } + } + None +} diff --git a/src/flake8_bandit/mod.rs b/src/flake8_bandit/mod.rs index f7dc93c6ae209..9287c78e112be 100644 --- a/src/flake8_bandit/mod.rs +++ b/src/flake8_bandit/mod.rs @@ -21,6 +21,7 @@ mod tests { #[test_case(CheckCode::S106, Path::new("S106.py"); "S106")] #[test_case(CheckCode::S107, Path::new("S107.py"); "S107")] #[test_case(CheckCode::S108, Path::new("S108.py"); "S108")] + #[test_case(CheckCode::S506, Path::new("S506.py"); "S506")] fn checks(check_code: CheckCode, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", check_code.as_ref(), path.to_string_lossy()); let checks = test_path( diff --git a/src/flake8_bandit/snapshots/ruff__flake8_bandit__tests__S506_S506.py.snap b/src/flake8_bandit/snapshots/ruff__flake8_bandit__tests__S506_S506.py.snap new file mode 100644 index 0000000000000..f6979319c4b54 --- /dev/null +++ b/src/flake8_bandit/snapshots/ruff__flake8_bandit__tests__S506_S506.py.snap @@ -0,0 +1,25 @@ +--- +source: src/flake8_bandit/mod.rs +expression: checks +--- +- kind: + UnsafeYAMLLoad: No Loader used + location: + row: 9 + column: 8 + end_location: + row: 9 + column: 17 + fix: ~ + parent: ~ +- kind: + UnsafeYAMLLoad: Loader + location: + row: 22 + column: 23 + end_location: + row: 22 + column: 34 + fix: ~ + parent: ~ + diff --git a/src/registry.rs b/src/registry.rs index c651c6186de91..59d4ff4796520 100644 --- a/src/registry.rs +++ b/src/registry.rs @@ -333,6 +333,7 @@ pub enum CheckCode { S106, S107, S108, + S506, // flake8-boolean-trap FBT001, FBT002, @@ -1072,6 +1073,7 @@ pub enum CheckKind { HardcodedPasswordFuncArg(String), HardcodedPasswordDefault(String), HardcodedTempFile(String), + UnsafeYAMLLoad(String), // mccabe FunctionIsTooComplex(String, usize), // flake8-boolean-trap @@ -1534,6 +1536,7 @@ impl CheckCode { CheckCode::S106 => CheckKind::HardcodedPasswordFuncArg("...".to_string()), CheckCode::S107 => CheckKind::HardcodedPasswordDefault("...".to_string()), CheckCode::S108 => CheckKind::HardcodedTempFile("...".to_string()), + CheckCode::S506 => CheckKind::UnsafeYAMLLoad("...".to_string()), // mccabe CheckCode::C901 => CheckKind::FunctionIsTooComplex("...".to_string(), 10), // flake8-boolean-trap @@ -1936,6 +1939,7 @@ impl CheckCode { CheckCode::S106 => CheckCategory::Flake8Bandit, CheckCode::S107 => CheckCategory::Flake8Bandit, CheckCode::S108 => CheckCategory::Flake8Bandit, + CheckCode::S506 => CheckCategory::Flake8Bandit, // flake8-simplify CheckCode::SIM102 => CheckCategory::Flake8Simplify, CheckCode::SIM105 => CheckCategory::Flake8Simplify, @@ -2311,6 +2315,7 @@ impl CheckKind { CheckKind::HardcodedPasswordFuncArg(..) => &CheckCode::S106, CheckKind::HardcodedPasswordDefault(..) => &CheckCode::S107, CheckKind::HardcodedTempFile(..) => &CheckCode::S108, + CheckKind::UnsafeYAMLLoad(..) => &CheckCode::S506, // mccabe CheckKind::FunctionIsTooComplex(..) => &CheckCode::C901, // flake8-boolean-trap @@ -3265,6 +3270,12 @@ impl CheckKind { string.escape_debug() ) } + CheckKind::UnsafeYAMLLoad(string) => { + format!( + "Probable insecure usage of `yaml.load`: `\"{}\"`", + string.escape_debug() + ) + } // flake8-blind-except CheckKind::BlindExcept(name) => format!("Do not catch blind exception: `{name}`"), // mccabe diff --git a/src/registry_gen.rs b/src/registry_gen.rs index 77f1803884b3a..5357f6540edf8 100644 --- a/src/registry_gen.rs +++ b/src/registry_gen.rs @@ -515,6 +515,9 @@ pub enum CheckCodePrefix { S106, S107, S108, + S5, + S50, + S506, SIM, SIM1, SIM10, @@ -921,6 +924,7 @@ impl CheckCodePrefix { CheckCode::S106, CheckCode::S107, CheckCode::S108, + CheckCode::S506, CheckCode::FBT001, CheckCode::FBT002, CheckCode::FBT003, @@ -2609,6 +2613,7 @@ impl CheckCodePrefix { CheckCode::S106, CheckCode::S107, CheckCode::S108, + CheckCode::S506, ], CheckCodePrefix::S1 => vec![ CheckCode::S101, @@ -2638,6 +2643,9 @@ impl CheckCodePrefix { CheckCodePrefix::S106 => vec![CheckCode::S106], CheckCodePrefix::S107 => vec![CheckCode::S107], CheckCodePrefix::S108 => vec![CheckCode::S108], + CheckCodePrefix::S5 => vec![CheckCode::S506], + CheckCodePrefix::S50 => vec![CheckCode::S506], + CheckCodePrefix::S506 => vec![CheckCode::S506], CheckCodePrefix::SIM => vec![ CheckCode::SIM102, CheckCode::SIM105, @@ -3642,6 +3650,9 @@ impl CheckCodePrefix { CheckCodePrefix::S106 => SuffixLength::Three, CheckCodePrefix::S107 => SuffixLength::Three, CheckCodePrefix::S108 => SuffixLength::Three, + CheckCodePrefix::S5 => SuffixLength::One, + CheckCodePrefix::S50 => SuffixLength::Two, + CheckCodePrefix::S506 => SuffixLength::Three, CheckCodePrefix::SIM => SuffixLength::Zero, CheckCodePrefix::SIM1 => SuffixLength::One, CheckCodePrefix::SIM10 => SuffixLength::Two, From bc284e02e55893f49f19f3a80c66b7fd0e5b8f0c Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 5 Jan 2023 13:33:27 -0500 Subject: [PATCH 2/2] Tweak payload --- README.md | 11 +++---- ruff.schema.json | 3 ++ src/flake8_bandit/checks/unsafe_yaml_load.rs | 16 +++++----- ...f__flake8_bandit__tests__S506_S506.py.snap | 2 +- src/registry.rs | 29 +++++++++---------- 5 files changed, 32 insertions(+), 29 deletions(-) diff --git a/README.md b/README.md index 2365d9aca091f..8c6cb9f3e0b50 100644 --- a/README.md +++ b/README.md @@ -767,11 +767,12 @@ For more, see [flake8-bandit](https://pypi.org/project/flake8-bandit/4.1.1/) on | S102 | ExecUsed | Use of `exec` detected | | | S103 | BadFilePermissions | `os.chmod` setting a permissive mask `0o777` on file or directory | | | S104 | HardcodedBindAllInterfaces | Possible binding to all interfaces | | -| S105 | HardcodedPasswordString | Possible hardcoded password: `"..."` | | -| S106 | HardcodedPasswordFuncArg | Possible hardcoded password: `"..."` | | -| S107 | HardcodedPasswordDefault | Possible hardcoded password: `"..."` | | -| S108 | HardcodedTempFile | Probable insecure usage of temp file/directory: `"..."` | | -| S324 | HashlibInsecureHashFunction | Probable use of insecure hash functions in hashlib: `"..."` | | +| S105 | HardcodedPasswordString | Possible hardcoded password: "..." | | +| S106 | HardcodedPasswordFuncArg | Possible hardcoded password: "..." | | +| S107 | HardcodedPasswordDefault | Possible hardcoded password: "..." | | +| S108 | HardcodedTempFile | Probable insecure usage of temporary file or directory: "..." | | +| S324 | HashlibInsecureHashFunction | Probable use of insecure hash functions in `hashlib`: "..." | | +| S506 | UnsafeYAMLLoad | Probable use of unsafe `yaml.load`. Allows instantiation of arbitrary objects. Consider `yaml.safe_load`. | | ### flake8-blind-except (BLE) diff --git a/ruff.schema.json b/ruff.schema.json index f3de63daa7dce..261511533067b 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -930,6 +930,9 @@ "S3", "S32", "S324", + "S5", + "S50", + "S506", "SIM", "SIM1", "SIM10", diff --git a/src/flake8_bandit/checks/unsafe_yaml_load.rs b/src/flake8_bandit/checks/unsafe_yaml_load.rs index a924b10dc2d84..9c3e6e51f05e6 100644 --- a/src/flake8_bandit/checks/unsafe_yaml_load.rs +++ b/src/flake8_bandit/checks/unsafe_yaml_load.rs @@ -8,8 +8,8 @@ use crate::registry::{Check, CheckKind}; /// S506 pub fn unsafe_yaml_load( func: &Expr, - args: &Vec, - keywords: &Vec, + args: &[Expr], + keywords: &[Keyword], from_imports: &FxHashMap<&str, FxHashSet<&str>>, import_aliases: &FxHashMap<&str, &str>, ) -> Option { @@ -29,19 +29,19 @@ pub fn unsafe_yaml_load( from_imports, import_aliases, ) { - let loader_name = match &loader_arg.node { - ExprKind::Attribute { attr, .. } => Some(attr), - ExprKind::Name { id, .. } => Some(id), + let loader = match &loader_arg.node { + ExprKind::Attribute { attr, .. } => Some(attr.to_string()), + ExprKind::Name { id, .. } => Some(id.to_string()), _ => None, - }?; + }; return Some(Check::new( - CheckKind::UnsafeYAMLLoad(loader_name.clone()), + CheckKind::UnsafeYAMLLoad(loader), Range::from_located(loader_arg), )); } } else { return Some(Check::new( - CheckKind::UnsafeYAMLLoad("No Loader used".to_string()), + CheckKind::UnsafeYAMLLoad(None), Range::from_located(func), )); } diff --git a/src/flake8_bandit/snapshots/ruff__flake8_bandit__tests__S506_S506.py.snap b/src/flake8_bandit/snapshots/ruff__flake8_bandit__tests__S506_S506.py.snap index 007a6f0d6ed81..386f5346b5808 100644 --- a/src/flake8_bandit/snapshots/ruff__flake8_bandit__tests__S506_S506.py.snap +++ b/src/flake8_bandit/snapshots/ruff__flake8_bandit__tests__S506_S506.py.snap @@ -3,7 +3,7 @@ source: src/flake8_bandit/mod.rs expression: checks --- - kind: - UnsafeYAMLLoad: No Loader used + UnsafeYAMLLoad: ~ location: row: 10 column: 8 diff --git a/src/registry.rs b/src/registry.rs index d4495b6db1388..8549a39502b7b 100644 --- a/src/registry.rs +++ b/src/registry.rs @@ -1075,11 +1075,8 @@ pub enum CheckKind { HardcodedPasswordFuncArg(String), HardcodedPasswordDefault(String), HardcodedTempFile(String), - - UnsafeYAMLLoad(String), - HashlibInsecureHashFunction(String), - + UnsafeYAMLLoad(Option), // mccabe FunctionIsTooComplex(String, usize), // flake8-boolean-trap @@ -1543,7 +1540,7 @@ impl CheckCode { CheckCode::S107 => CheckKind::HardcodedPasswordDefault("...".to_string()), CheckCode::S108 => CheckKind::HardcodedTempFile("...".to_string()), CheckCode::S324 => CheckKind::HashlibInsecureHashFunction("...".to_string()), - CheckCode::S506 => CheckKind::UnsafeYAMLLoad("...".to_string()), + CheckCode::S506 => CheckKind::UnsafeYAMLLoad(None), // mccabe CheckCode::C901 => CheckKind::FunctionIsTooComplex("...".to_string(), 10), // flake8-boolean-trap @@ -3268,10 +3265,7 @@ impl CheckKind { CheckKind::HardcodedPasswordString(string) | CheckKind::HardcodedPasswordFuncArg(string) | CheckKind::HardcodedPasswordDefault(string) => { - format!( - "Possible hardcoded password: `\"{}\"`", - string.escape_debug() - ) + format!("Possible hardcoded password: \"{}\"", string.escape_debug()) } CheckKind::HardcodedTempFile(string) => { format!( @@ -3285,12 +3279,17 @@ impl CheckKind { string.escape_debug() ) } - CheckKind::UnsafeYAMLLoad(string) => { - format!( - "Probable insecure usage of `yaml.load`: \"{}\"", - string.escape_debug() - ) - } + CheckKind::UnsafeYAMLLoad(loader) => match loader { + Some(name) => { + format!( + "Probable use of unsafe loader `{name}` with `yaml.load`. Allows \ + instantiation of arbitrary objects. Consider `yaml.safe_load`." + ) + } + None => "Probable use of unsafe `yaml.load`. Allows instantiation of arbitrary \ + objects. Consider `yaml.safe_load`." + .to_string(), + }, // flake8-blind-except CheckKind::BlindExcept(name) => format!("Do not catch blind exception: `{name}`"), // mccabe