From 63a04c056a43ac92e453d1e89a20428e23b8d0b8 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Sat, 23 Mar 2024 21:30:33 +0000 Subject: [PATCH 1/5] Add test with `tokenImage` as used in JavaCC --- .../CWE-532/TokenSequenceParserTest.java | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 java/ql/test/query-tests/security/CWE-532/TokenSequenceParserTest.java diff --git a/java/ql/test/query-tests/security/CWE-532/TokenSequenceParserTest.java b/java/ql/test/query-tests/security/CWE-532/TokenSequenceParserTest.java new file mode 100644 index 00000000000..89814865a9f --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-532/TokenSequenceParserTest.java @@ -0,0 +1,17 @@ +import org.apache.logging.log4j.Logger; + +interface TokenSequenceParserConstants { + /** Literal token values. */ + String[] tokenImage = { + "", + }; +} + +public class TokenSequenceParserTest implements TokenSequenceParserConstants { + void test(String password) { + Logger logger = null; + + logger.info("When parsing found this: " + tokenImage[0]); // Safe + } + +} From 4832dc51edfd122cf82eb424b1b8c4e543f63171 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Sat, 23 Mar 2024 21:33:02 +0000 Subject: [PATCH 2/5] Whitelist variable name `tokenImage` --- .../ql/lib/semmle/code/java/security/SensitiveLoggingQuery.qll | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/java/ql/lib/semmle/code/java/security/SensitiveLoggingQuery.qll b/java/ql/lib/semmle/code/java/security/SensitiveLoggingQuery.qll index c599756a81c..70ef0b39405 100644 --- a/java/ql/lib/semmle/code/java/security/SensitiveLoggingQuery.qll +++ b/java/ql/lib/semmle/code/java/security/SensitiveLoggingQuery.qll @@ -12,7 +12,8 @@ class VariableWithSensitiveName extends Variable { VariableWithSensitiveName() { exists(string name | name = this.getName() | name.regexpMatch(getCommonSensitiveInfoRegex()) and - not name.regexpMatch("(?i).*null.*") + not name.regexpMatch("(?i).*null.*") and + not name.matches("tokenImage") // appears in parser code generated by JavaCC ) } } From f4b3bae88b51eff81e2f58ccbbf3b3e6fd2f0e2c Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Sat, 23 Mar 2024 23:48:16 +0000 Subject: [PATCH 3/5] Add test for ParseException use of `tokenImage` --- .../security/CWE-532/TokenSequenceParserTest.java | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/java/ql/test/query-tests/security/CWE-532/TokenSequenceParserTest.java b/java/ql/test/query-tests/security/CWE-532/TokenSequenceParserTest.java index 89814865a9f..0a24c32f26a 100644 --- a/java/ql/test/query-tests/security/CWE-532/TokenSequenceParserTest.java +++ b/java/ql/test/query-tests/security/CWE-532/TokenSequenceParserTest.java @@ -15,3 +15,13 @@ public class TokenSequenceParserTest implements TokenSequenceParserConstants { } } + +class ParseExceptionTest extends Exception { + String[] tokenImage; + + void test() { + Logger logger = null; + + logger.info("When parsing found this: " + tokenImage[0]); // Safe + } +} From 821f3991930c4d3566a48310093db870ed0e821d Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Sat, 23 Mar 2024 23:51:52 +0000 Subject: [PATCH 4/5] Add change note --- .../2024-03-24-sensitive-log-whitelist-tokenimage.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 java/ql/src/change-notes/2024-03-24-sensitive-log-whitelist-tokenimage.md diff --git a/java/ql/src/change-notes/2024-03-24-sensitive-log-whitelist-tokenimage.md b/java/ql/src/change-notes/2024-03-24-sensitive-log-whitelist-tokenimage.md new file mode 100644 index 00000000000..d62c2dfbb47 --- /dev/null +++ b/java/ql/src/change-notes/2024-03-24-sensitive-log-whitelist-tokenimage.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Variables named `tokenImage` are no longer sources for the `java/sensitive-log` query. This is because this variable name is used in parsing code generated by JavaCC, so it causes a larger number of false positive alerts. From ac6c4add1480fc0ec35d2bf49231f1996226b604 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan <62447351+owen-mc@users.noreply.github.com> Date: Sun, 24 Mar 2024 20:11:15 +0000 Subject: [PATCH 5/5] Apply suggestions from code review Co-authored-by: Chris Smowton --- java/ql/lib/semmle/code/java/security/SensitiveLoggingQuery.qll | 2 +- .../2024-03-24-sensitive-log-whitelist-tokenimage.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/SensitiveLoggingQuery.qll b/java/ql/lib/semmle/code/java/security/SensitiveLoggingQuery.qll index 70ef0b39405..d46d35ab0cc 100644 --- a/java/ql/lib/semmle/code/java/security/SensitiveLoggingQuery.qll +++ b/java/ql/lib/semmle/code/java/security/SensitiveLoggingQuery.qll @@ -13,7 +13,7 @@ class VariableWithSensitiveName extends Variable { exists(string name | name = this.getName() | name.regexpMatch(getCommonSensitiveInfoRegex()) and not name.regexpMatch("(?i).*null.*") and - not name.matches("tokenImage") // appears in parser code generated by JavaCC + name != "tokenImage" // appears in parser code generated by JavaCC ) } } diff --git a/java/ql/src/change-notes/2024-03-24-sensitive-log-whitelist-tokenimage.md b/java/ql/src/change-notes/2024-03-24-sensitive-log-whitelist-tokenimage.md index d62c2dfbb47..017e5abd7ee 100644 --- a/java/ql/src/change-notes/2024-03-24-sensitive-log-whitelist-tokenimage.md +++ b/java/ql/src/change-notes/2024-03-24-sensitive-log-whitelist-tokenimage.md @@ -1,4 +1,4 @@ --- category: minorAnalysis --- -* Variables named `tokenImage` are no longer sources for the `java/sensitive-log` query. This is because this variable name is used in parsing code generated by JavaCC, so it causes a larger number of false positive alerts. +* Variables named `tokenImage` are no longer sources for the `java/sensitive-log` query. This is because this variable name is used in parsing code generated by JavaCC, so it causes a large number of false positive alerts.