From 28472ae12f565229b0262f06fac722b8504687d3 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 30 Jan 2025 15:00:46 +0100 Subject: [PATCH 1/4] Test: Don't expect 'Source' tag when source and alert are on same line Previously the Source tag was required if the source and alert did not have the exact same location. This relaxes the restriction to being on the same line. Note that in order to be "on the same line" both start and end lines have to match. It's still possible for a given line to expect both Alert and Source tags, in case the source pairs up with another alert on a different line. --- .../util/test/InlineExpectationsTest.qll | 29 +++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/shared/util/codeql/util/test/InlineExpectationsTest.qll b/shared/util/codeql/util/test/InlineExpectationsTest.qll index 8dde42b51678..2e60a3edd707 100644 --- a/shared/util/codeql/util/test/InlineExpectationsTest.qll +++ b/shared/util/codeql/util/test/InlineExpectationsTest.qll @@ -645,6 +645,29 @@ module TestPostProcessing { private import InlineExpectationsTest as InlineExpectationsTest private import InlineExpectationsTest::Make + bindingset[loc] + private predicate parseLocation( + string loc, string file, int startLine, int startColumn, int endLine, int endColumn + ) { + exists(string regexp | + regexp = "(.*):(-?\\d+):(-?\\d+):(-?\\d+):(-?\\d+)" and + file = loc.regexpCapture(regexp, 1) and + startLine = loc.regexpCapture(regexp, 2).toInt() and + startColumn = loc.regexpCapture(regexp, 3).toInt() and + endLine = loc.regexpCapture(regexp, 4).toInt() and + endColumn = loc.regexpCapture(regexp, 5).toInt() + ) + } + + /** Holds if the given location strings refer to the same lines, but possibly with different column numbers. */ + bindingset[loc1, loc2] + private predicate sameLineInfo(string loc1, string loc2) { + exists(string file, int line1, int line2 | + parseLocation(loc1, file, line1, _, line2, _) and + parseLocation(loc2, file, line1, _, line2, _) + ) + } + /** * Gets the tag to be used for the path-problem source at result row `row`. * @@ -653,8 +676,10 @@ module TestPostProcessing { */ private string getSourceTag(int row) { getQueryKind() = "path-problem" and - exists(string loc | queryResults(mainResultSet(), row, 2, loc) | - if queryResults(mainResultSet(), row, 0, loc) then result = "Alert" else result = "Source" + exists(string sourceLoc, string selectLoc | + queryResults(mainResultSet(), row, 0, selectLoc) and + queryResults(mainResultSet(), row, 2, sourceLoc) and + if sameLineInfo(selectLoc, sourceLoc) then result = "Alert" else result = "Source" ) } From 78a7f2670aa2d2d759074e8ec2547ff924227822 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 31 Jan 2025 14:34:05 +0100 Subject: [PATCH 2/4] JS: Update a JS test case --- javascript/ql/test/query-tests/Security/CWE-611/libxml.noent.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/javascript/ql/test/query-tests/Security/CWE-611/libxml.noent.js b/javascript/ql/test/query-tests/Security/CWE-611/libxml.noent.js index 40cb0148b573..4596dc7be398 100644 --- a/javascript/ql/test/query-tests/Security/CWE-611/libxml.noent.js +++ b/javascript/ql/test/query-tests/Security/CWE-611/libxml.noent.js @@ -13,7 +13,7 @@ express().post('/some/path', function (req, res) { // NOT OK: unguarded entity expansion libxmljs.parseXmlString(req.param("some-xml"), { noent: true }) // $ Alert // NOT OK: unguarded entity expansion - libxmljs.parseXmlString(req.files.products.data.toString('utf8'), { noent: true })// $ Source=files $ Alert=files + libxmljs.parseXmlString(req.files.products.data.toString('utf8'), { noent: true })// $ Alert // OK - no entity expansion libxmljs.parseXmlString(req.files.products.data.toString('utf8'), { noent: false }) From fc1d36f867f576bb276d51edd2d826b5169f5761 Mon Sep 17 00:00:00 2001 From: Asger F Date: Fri, 31 Jan 2025 14:36:56 +0100 Subject: [PATCH 3/4] Rust: update a Rust test case --- .../security/CWE-312/test_logging.rs | 88 +++++++++---------- 1 file changed, 44 insertions(+), 44 deletions(-) diff --git a/rust/ql/test/query-tests/security/CWE-312/test_logging.rs b/rust/ql/test/query-tests/security/CWE-312/test_logging.rs index ab8013689906..970a9caf0ee5 100644 --- a/rust/ql/test/query-tests/security/CWE-312/test_logging.rs +++ b/rust/ql/test/query-tests/security/CWE-312/test_logging.rs @@ -39,51 +39,51 @@ impl std::fmt::Display for MyStruct2 { fn test_log(harmless: String, password: String, encrypted_password: String) { // logging macros - debug!("message = {}", password); // $ Source Alert[rust/cleartext-logging] - error!("message = {}", password); // $ Source Alert[rust/cleartext-logging] - info!("message = {}", password); // $ Source Alert[rust/cleartext-logging] - trace!("message = {}", password); // $ Source Alert[rust/cleartext-logging] - warn!("message = {}", password); // $ Source Alert[rust/cleartext-logging] - log!(Level::Error, "message = {}", password); // $ Source Alert[rust/cleartext-logging] + debug!("message = {}", password); // $ Alert[rust/cleartext-logging] + error!("message = {}", password); // $ Alert[rust/cleartext-logging] + info!("message = {}", password); // $ Alert[rust/cleartext-logging] + trace!("message = {}", password); // $ Alert[rust/cleartext-logging] + warn!("message = {}", password); // $ Alert[rust/cleartext-logging] + log!(Level::Error, "message = {}", password); // $ Alert[rust/cleartext-logging] // debug! macro, various formatting debug!("message"); debug!("message = {}", harmless); - debug!("message = {}", password); // $ Source Alert[rust/cleartext-logging] + debug!("message = {}", password); // $ Alert[rust/cleartext-logging] debug!("message = {}", encrypted_password); - debug!("message = {} {}", harmless, password); // $ Source Alert[rust/cleartext-logging] + debug!("message = {} {}", harmless, password); // $ Alert[rust/cleartext-logging] debug!("message = {harmless}"); - debug!("message = {harmless} {}", password); // $ Source Alert[rust/cleartext-logging] - debug!("message = {password}"); // $ Source Alert[rust/cleartext-logging] - debug!("message = {password:?}"); // $ Source Alert[rust/cleartext-logging] + debug!("message = {harmless} {}", password); // $ Alert[rust/cleartext-logging] + debug!("message = {password}"); // $ Alert[rust/cleartext-logging] + debug!("message = {password:?}"); // $ Alert[rust/cleartext-logging] debug!(target: "target", "message = {}", harmless); - debug!(target: "target", "message = {}", password); // $ Source Alert[rust/cleartext-logging] - debug!(target: &password, "message = {}", harmless); // $ Source Alert[rust/cleartext-logging] + debug!(target: "target", "message = {}", password); // $ Alert[rust/cleartext-logging] + debug!(target: &password, "message = {}", harmless); // $ Alert[rust/cleartext-logging] // log! macro, various formatting log!(Level::Error, "message = {}", harmless); - log!(Level::Error, "message = {}", password); // $ Source Alert[rust/cleartext-logging] + log!(Level::Error, "message = {}", password); // $ Alert[rust/cleartext-logging] log!(target: "target", Level::Error, "message = {}", harmless); - log!(target: "target", Level::Error, "message = {}", password); // $ Source Alert[rust/cleartext-logging] - log!(target: &password, Level::Error, "message = {}", harmless); // $ Source Alert[rust/cleartext-logging] + log!(target: "target", Level::Error, "message = {}", password); // $ Alert[rust/cleartext-logging] + log!(target: &password, Level::Error, "message = {}", harmless); // $ Alert[rust/cleartext-logging] // structured logging error!(value = 1; "message = {}", harmless); - error!(value = 1; "message = {}", password); // $ Source Alert[rust/cleartext-logging] + error!(value = 1; "message = {}", password); // $ Alert[rust/cleartext-logging] error!(target: "target", value = 1; "message"); - error!(target: "target", value = 1; "message = {}", password); // $ Source Alert[rust/cleartext-logging] - error!(target: &password, value = 1; "message"); // $ Source Alert[rust/cleartext-logging] - error!(value = 1; "message = {}", password); // $ Source Alert[rust/cleartext-logging] + error!(target: "target", value = 1; "message = {}", password); // $ Alert[rust/cleartext-logging] + error!(target: &password, value = 1; "message"); // $ Alert[rust/cleartext-logging] + error!(value = 1; "message = {}", password); // $ Alert[rust/cleartext-logging] error!(value = password.as_str(); "message"); // $ MISSING: Alert[rust/cleartext-logging] error!(value:? = password.as_str(); "message"); // $ MISSING: Alert[rust/cleartext-logging] let value1 = 1; error!(value1; "message = {}", harmless); - error!(value1; "message = {}", password); // $ Source Alert[rust/cleartext-logging] + error!(value1; "message = {}", password); // $ Alert[rust/cleartext-logging] error!(target: "target", value1; "message"); - error!(target: "target", value1; "message = {}", password); // $ Source Alert[rust/cleartext-logging] - error!(target: &password, value1; "message"); // $ Source Alert[rust/cleartext-logging] - error!(value1; "message = {}", password); // $ Source Alert[rust/cleartext-logging] + error!(target: "target", value1; "message = {}", password); // $ Alert[rust/cleartext-logging] + error!(target: &password, value1; "message"); // $ Alert[rust/cleartext-logging] + error!(value1; "message = {}", password); // $ Alert[rust/cleartext-logging] let value2 = password.as_str(); error!(value2; "message"); // $ MISSING: Alert[rust/cleartext-logging] @@ -115,7 +115,7 @@ fn test_log(harmless: String, password: String, encrypted_password: String) { } // logging with a call - trace!("message = {}", get_password()); // $ Source Alert[rust/cleartext-logging] + trace!("message = {}", get_password()); // $ Alert[rust/cleartext-logging] let str1 = "123456".to_string(); trace!("message = {}", &str1); // $ MISSING: Alert[rust/cleartext-logging] @@ -149,36 +149,36 @@ fn test_log(harmless: String, password: String, encrypted_password: String) { } fn test_std(password: String, i: i32, opt_i: Option) { - print!("message = {}\n", password); // $ Source Alert[rust/cleartext-logging] - println!("message = {}", password); // $ Source Alert[rust/cleartext-logging] - eprint!("message = {}\n", password); // $ Source Alert[rust/cleartext-logging] - eprintln!("message = {}", password); // $ Source Alert[rust/cleartext-logging] + print!("message = {}\n", password); // $ Alert[rust/cleartext-logging] + println!("message = {}", password); // $ Alert[rust/cleartext-logging] + eprint!("message = {}\n", password); // $ Alert[rust/cleartext-logging] + eprintln!("message = {}", password); // $ Alert[rust/cleartext-logging] match i { - 1 => { panic!("message = {}", password); } // $ Source Alert[rust/cleartext-logging] - 2 => { todo!("message = {}", password); } // $ Source Alert[rust/cleartext-logging] - 3 => { unimplemented!("message = {}", password); } // $ Source Alert[rust/cleartext-logging] - 4 => { unreachable!("message = {}", password); } // $ Source Alert[rust/cleartext-logging] - 5 => { assert!(false, "message = {}", password); } // $ Source Alert[rust/cleartext-logging] - 6 => { assert_eq!(1, 2, "message = {}", password); } // $ Source Alert[rust/cleartext-logging] - 7 => { assert_ne!(1, 1, "message = {}", password); } // $ Source Alert[rust/cleartext-logging] - 8 => { debug_assert!(false, "message = {}", password); } // $ Source Alert[rust/cleartext-logging] - 9 => { debug_assert_eq!(1, 2, "message = {}", password); } // $ Source Alert[rust/cleartext-logging] - 10 => { debug_assert_ne!(1, 1, "message = {}", password); } // $ Source Alert[rust/cleartext-logging] - 11 => { _ = opt_i.expect(format!("message = {}", password).as_str()); } // $ Source Alert[rust/cleartext-logging] + 1 => { panic!("message = {}", password); } // $ Alert[rust/cleartext-logging] + 2 => { todo!("message = {}", password); } // $ Alert[rust/cleartext-logging] + 3 => { unimplemented!("message = {}", password); } // $ Alert[rust/cleartext-logging] + 4 => { unreachable!("message = {}", password); } // $ Alert[rust/cleartext-logging] + 5 => { assert!(false, "message = {}", password); } // $ Alert[rust/cleartext-logging] + 6 => { assert_eq!(1, 2, "message = {}", password); } // $ Alert[rust/cleartext-logging] + 7 => { assert_ne!(1, 1, "message = {}", password); } // $ Alert[rust/cleartext-logging] + 8 => { debug_assert!(false, "message = {}", password); } // $ Alert[rust/cleartext-logging] + 9 => { debug_assert_eq!(1, 2, "message = {}", password); } // $ Alert[rust/cleartext-logging] + 10 => { debug_assert_ne!(1, 1, "message = {}", password); } // $ Alert[rust/cleartext-logging] + 11 => { _ = opt_i.expect(format!("message = {}", password).as_str()); } // $ Alert[rust/cleartext-logging] _ => {} } std::io::stdout().lock().write_fmt(format_args!("message = {}\n", password)); // $ MISSING: Alert[rust/cleartext-logging] std::io::stderr().lock().write_fmt(format_args!("message = {}\n", password)); // $ MISSING: Alert[rust/cleartext-logging] - std::io::stdout().lock().write(format!("message = {}\n", password).as_bytes()); // $ Source Alert[rust/cleartext-logging] - std::io::stdout().lock().write_all(format!("message = {}\n", password).as_bytes()); // $ Source Alert[rust/cleartext-logging] + std::io::stdout().lock().write(format!("message = {}\n", password).as_bytes()); // $ Alert[rust/cleartext-logging] + std::io::stdout().lock().write_all(format!("message = {}\n", password).as_bytes()); // $ Alert[rust/cleartext-logging] let mut out = std::io::stdout().lock(); - out.write(format!("message = {}\n", password).as_bytes()); // $ Source Alert[rust/cleartext-logging] + out.write(format!("message = {}\n", password).as_bytes()); // $ Alert[rust/cleartext-logging] let mut err = std::io::stderr().lock(); - err.write(format!("message = {}\n", password).as_bytes()); // $ Source Alert[rust/cleartext-logging] + err.write(format!("message = {}\n", password).as_bytes()); // $ Alert[rust/cleartext-logging] } fn main() { From fc04ad1ef01b62c8c4e65425242a9e436e0195ae Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Tue, 4 Feb 2025 09:34:33 +0100 Subject: [PATCH 4/4] Test: Remove location parsing --- .../util/test/InlineExpectationsTest.qll | 67 +++++++------------ 1 file changed, 25 insertions(+), 42 deletions(-) diff --git a/shared/util/codeql/util/test/InlineExpectationsTest.qll b/shared/util/codeql/util/test/InlineExpectationsTest.qll index 2e60a3edd707..a3143c4848e4 100644 --- a/shared/util/codeql/util/test/InlineExpectationsTest.qll +++ b/shared/util/codeql/util/test/InlineExpectationsTest.qll @@ -645,29 +645,21 @@ module TestPostProcessing { private import InlineExpectationsTest as InlineExpectationsTest private import InlineExpectationsTest::Make - bindingset[loc] - private predicate parseLocation( - string loc, string file, int startLine, int startColumn, int endLine, int endColumn - ) { - exists(string regexp | - regexp = "(.*):(-?\\d+):(-?\\d+):(-?\\d+):(-?\\d+)" and - file = loc.regexpCapture(regexp, 1) and - startLine = loc.regexpCapture(regexp, 2).toInt() and - startColumn = loc.regexpCapture(regexp, 3).toInt() and - endLine = loc.regexpCapture(regexp, 4).toInt() and - endColumn = loc.regexpCapture(regexp, 5).toInt() - ) - } - - /** Holds if the given location strings refer to the same lines, but possibly with different column numbers. */ + /** Holds if the given locations refer to the same lines, but possibly with different column numbers. */ bindingset[loc1, loc2] - private predicate sameLineInfo(string loc1, string loc2) { + pragma[inline_late] + private predicate sameLineInfo(Input::Location loc1, Input::Location loc2) { exists(string file, int line1, int line2 | - parseLocation(loc1, file, line1, _, line2, _) and - parseLocation(loc2, file, line1, _, line2, _) + loc1.hasLocationInfo(file, line1, _, line2, _) and + loc2.hasLocationInfo(file, line1, _, line2, _) ) } + pragma[nomagic] + private predicate mainQueryResult(int row, int column, Input::Location loc) { + queryResults(mainResultSet(), row, column, Input2::getRelativeUrl(loc)) + } + /** * Gets the tag to be used for the path-problem source at result row `row`. * @@ -676,9 +668,9 @@ module TestPostProcessing { */ private string getSourceTag(int row) { getQueryKind() = "path-problem" and - exists(string sourceLoc, string selectLoc | - queryResults(mainResultSet(), row, 0, selectLoc) and - queryResults(mainResultSet(), row, 2, sourceLoc) and + exists(Input::Location sourceLoc, Input::Location selectLoc | + mainQueryResult(row, 0, selectLoc) and + mainQueryResult(row, 2, sourceLoc) and if sameLineInfo(selectLoc, sourceLoc) then result = "Alert" else result = "Source" ) } @@ -744,13 +736,10 @@ module TestPostProcessing { int row, Input::Location location, string element, string tag, string value ) { getQueryKind() = "path-problem" and - exists(string loc | - queryResults(mainResultSet(), row, 2, loc) and - queryResults(mainResultSet(), row, 3, element) and - tag = getSourceTag(row) and - value = "" and - Input2::getRelativeUrl(location) = loc - ) + mainQueryResult(row, 2, location) and + queryResults(mainResultSet(), row, 3, element) and + tag = getSourceTag(row) and + value = "" } predicate hasActualResult(Input::Location location, string element, string tag, string value) { @@ -784,24 +773,18 @@ module TestPostProcessing { int row, Input::Location location, string element, string tag ) { getQueryKind() = "path-problem" and - exists(string loc | - queryResults(mainResultSet(), row, 4, loc) and - queryResults(mainResultSet(), row, 5, element) and - tag = getSinkTag(row) and - Input2::getRelativeUrl(location) = loc - ) + mainQueryResult(row, 4, location) and + queryResults(mainResultSet(), row, 5, element) and + tag = getSinkTag(row) } private predicate hasAlert(int row, Input::Location location, string element, string tag) { getQueryKind() = ["problem", "path-problem"] and - exists(string loc | - queryResults(mainResultSet(), row, 0, loc) and - queryResults(mainResultSet(), row, 2, element) and - tag = "Alert" and - Input2::getRelativeUrl(location) = loc and - not hasPathProblemSource(row, location, _, _, _) and - not hasPathProblemSink(row, location, _, _) - ) + mainQueryResult(row, 0, location) and + queryResults(mainResultSet(), row, 2, element) and + tag = "Alert" and + not hasPathProblemSource(row, location, _, _, _) and + not hasPathProblemSink(row, location, _, _) } /**