From 2fd5d26b1b27d397f799a4569371e4275e641439 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Tue, 8 Dec 2020 16:37:53 +0000 Subject: [PATCH 1/3] Add FP as a test case --- .../query-tests/security/CWE-611/SAXParserTests.java | 10 +++++++++- java/ql/test/query-tests/security/CWE-611/XXE.expected | 2 ++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/java/ql/test/query-tests/security/CWE-611/SAXParserTests.java b/java/ql/test/query-tests/security/CWE-611/SAXParserTests.java index 882cb79bac8..bafefd637eb 100644 --- a/java/ql/test/query-tests/security/CWE-611/SAXParserTests.java +++ b/java/ql/test/query-tests/security/CWE-611/SAXParserTests.java @@ -2,7 +2,7 @@ import java.net.Socket; import javax.xml.parsers.SAXParser; import javax.xml.parsers.SAXParserFactory; - +import javax.xml.XMLConstants; import org.xml.sax.helpers.DefaultHandler; public class SAXParserTests { @@ -72,4 +72,12 @@ public class SAXParserTests { SAXParser parser = factory.newSAXParser(); parser.parse(sock.getInputStream(), new DefaultHandler()); //unsafe } + + public void safeParser2(Socket sock) throws Exception { + SAXParserFactory factory = SAXParserFactory.newInstance(); + factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + SAXParser parser = factory.newSAXParser(); + parser.parse(sock.getInputStream(), new DefaultHandler()); //safe [FP] + } } diff --git a/java/ql/test/query-tests/security/CWE-611/XXE.expected b/java/ql/test/query-tests/security/CWE-611/XXE.expected index 4cdedb8d0aa..ae499ebf113 100644 --- a/java/ql/test/query-tests/security/CWE-611/XXE.expected +++ b/java/ql/test/query-tests/security/CWE-611/XXE.expected @@ -71,6 +71,7 @@ nodes | SAXParserTests.java:55:18:55:38 | getInputStream(...) | semmle.label | getInputStream(...) | | SAXParserTests.java:64:18:64:38 | getInputStream(...) | semmle.label | getInputStream(...) | | SAXParserTests.java:73:18:73:38 | getInputStream(...) | semmle.label | getInputStream(...) | +| SAXParserTests.java:81:18:81:38 | getInputStream(...) | semmle.label | getInputStream(...) | | SAXReaderTests.java:8:17:8:37 | getInputStream(...) | semmle.label | getInputStream(...) | | SAXReaderTests.java:23:17:23:37 | getInputStream(...) | semmle.label | getInputStream(...) | | SAXReaderTests.java:30:17:30:37 | getInputStream(...) | semmle.label | getInputStream(...) | @@ -213,6 +214,7 @@ nodes | SAXParserTests.java:55:18:55:38 | getInputStream(...) | SAXParserTests.java:55:18:55:38 | getInputStream(...) | SAXParserTests.java:55:18:55:38 | getInputStream(...) | Unsafe parsing of XML file from $@. | SAXParserTests.java:55:18:55:38 | getInputStream(...) | user input | | SAXParserTests.java:64:18:64:38 | getInputStream(...) | SAXParserTests.java:64:18:64:38 | getInputStream(...) | SAXParserTests.java:64:18:64:38 | getInputStream(...) | Unsafe parsing of XML file from $@. | SAXParserTests.java:64:18:64:38 | getInputStream(...) | user input | | SAXParserTests.java:73:18:73:38 | getInputStream(...) | SAXParserTests.java:73:18:73:38 | getInputStream(...) | SAXParserTests.java:73:18:73:38 | getInputStream(...) | Unsafe parsing of XML file from $@. | SAXParserTests.java:73:18:73:38 | getInputStream(...) | user input | +| SAXParserTests.java:81:18:81:38 | getInputStream(...) | SAXParserTests.java:81:18:81:38 | getInputStream(...) | SAXParserTests.java:81:18:81:38 | getInputStream(...) | Unsafe parsing of XML file from $@. | SAXParserTests.java:81:18:81:38 | getInputStream(...) | user input | | SAXReaderTests.java:8:17:8:37 | getInputStream(...) | SAXReaderTests.java:8:17:8:37 | getInputStream(...) | SAXReaderTests.java:8:17:8:37 | getInputStream(...) | Unsafe parsing of XML file from $@. | SAXReaderTests.java:8:17:8:37 | getInputStream(...) | user input | | SAXReaderTests.java:23:17:23:37 | getInputStream(...) | SAXReaderTests.java:23:17:23:37 | getInputStream(...) | SAXReaderTests.java:23:17:23:37 | getInputStream(...) | Unsafe parsing of XML file from $@. | SAXReaderTests.java:23:17:23:37 | getInputStream(...) | user input | | SAXReaderTests.java:30:17:30:37 | getInputStream(...) | SAXReaderTests.java:30:17:30:37 | getInputStream(...) | SAXReaderTests.java:30:17:30:37 | getInputStream(...) | Unsafe parsing of XML file from $@. | SAXReaderTests.java:30:17:30:37 | getInputStream(...) | user input | From 24dc631a8ff3c3a0c107d475458f1b15c781cb51 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Tue, 8 Dec 2020 16:38:42 +0000 Subject: [PATCH 2/3] Java: Fix false positive in XXE query --- java/ql/src/semmle/code/java/security/XmlParsers.qll | 4 ++++ java/ql/test/query-tests/security/CWE-611/SAXParserTests.java | 2 +- java/ql/test/query-tests/security/CWE-611/XXE.expected | 2 -- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/java/ql/src/semmle/code/java/security/XmlParsers.qll b/java/ql/src/semmle/code/java/security/XmlParsers.qll index 7701af08923..685c5754fc9 100644 --- a/java/ql/src/semmle/code/java/security/XmlParsers.qll +++ b/java/ql/src/semmle/code/java/security/XmlParsers.qll @@ -481,6 +481,10 @@ class SAXParserFactoryConfig extends ParserConfig { class SafeSAXParserFactory extends VarAccess { SafeSAXParserFactory() { exists(Variable v | v = this.getVariable() | + exists(SAXParserFactoryConfig config | config.getQualifier() = v.getAnAccess() | + config.enables(singleSafeConfig()) + ) + or exists(SAXParserFactoryConfig config | config.getQualifier() = v.getAnAccess() | config .disables(any(ConstantStringExpr s | diff --git a/java/ql/test/query-tests/security/CWE-611/SAXParserTests.java b/java/ql/test/query-tests/security/CWE-611/SAXParserTests.java index bafefd637eb..f8079dd1bc8 100644 --- a/java/ql/test/query-tests/security/CWE-611/SAXParserTests.java +++ b/java/ql/test/query-tests/security/CWE-611/SAXParserTests.java @@ -78,6 +78,6 @@ public class SAXParserTests { factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); SAXParser parser = factory.newSAXParser(); - parser.parse(sock.getInputStream(), new DefaultHandler()); //safe [FP] + parser.parse(sock.getInputStream(), new DefaultHandler()); //safe } } diff --git a/java/ql/test/query-tests/security/CWE-611/XXE.expected b/java/ql/test/query-tests/security/CWE-611/XXE.expected index ae499ebf113..4cdedb8d0aa 100644 --- a/java/ql/test/query-tests/security/CWE-611/XXE.expected +++ b/java/ql/test/query-tests/security/CWE-611/XXE.expected @@ -71,7 +71,6 @@ nodes | SAXParserTests.java:55:18:55:38 | getInputStream(...) | semmle.label | getInputStream(...) | | SAXParserTests.java:64:18:64:38 | getInputStream(...) | semmle.label | getInputStream(...) | | SAXParserTests.java:73:18:73:38 | getInputStream(...) | semmle.label | getInputStream(...) | -| SAXParserTests.java:81:18:81:38 | getInputStream(...) | semmle.label | getInputStream(...) | | SAXReaderTests.java:8:17:8:37 | getInputStream(...) | semmle.label | getInputStream(...) | | SAXReaderTests.java:23:17:23:37 | getInputStream(...) | semmle.label | getInputStream(...) | | SAXReaderTests.java:30:17:30:37 | getInputStream(...) | semmle.label | getInputStream(...) | @@ -214,7 +213,6 @@ nodes | SAXParserTests.java:55:18:55:38 | getInputStream(...) | SAXParserTests.java:55:18:55:38 | getInputStream(...) | SAXParserTests.java:55:18:55:38 | getInputStream(...) | Unsafe parsing of XML file from $@. | SAXParserTests.java:55:18:55:38 | getInputStream(...) | user input | | SAXParserTests.java:64:18:64:38 | getInputStream(...) | SAXParserTests.java:64:18:64:38 | getInputStream(...) | SAXParserTests.java:64:18:64:38 | getInputStream(...) | Unsafe parsing of XML file from $@. | SAXParserTests.java:64:18:64:38 | getInputStream(...) | user input | | SAXParserTests.java:73:18:73:38 | getInputStream(...) | SAXParserTests.java:73:18:73:38 | getInputStream(...) | SAXParserTests.java:73:18:73:38 | getInputStream(...) | Unsafe parsing of XML file from $@. | SAXParserTests.java:73:18:73:38 | getInputStream(...) | user input | -| SAXParserTests.java:81:18:81:38 | getInputStream(...) | SAXParserTests.java:81:18:81:38 | getInputStream(...) | SAXParserTests.java:81:18:81:38 | getInputStream(...) | Unsafe parsing of XML file from $@. | SAXParserTests.java:81:18:81:38 | getInputStream(...) | user input | | SAXReaderTests.java:8:17:8:37 | getInputStream(...) | SAXReaderTests.java:8:17:8:37 | getInputStream(...) | SAXReaderTests.java:8:17:8:37 | getInputStream(...) | Unsafe parsing of XML file from $@. | SAXReaderTests.java:8:17:8:37 | getInputStream(...) | user input | | SAXReaderTests.java:23:17:23:37 | getInputStream(...) | SAXReaderTests.java:23:17:23:37 | getInputStream(...) | SAXReaderTests.java:23:17:23:37 | getInputStream(...) | Unsafe parsing of XML file from $@. | SAXReaderTests.java:23:17:23:37 | getInputStream(...) | user input | | SAXReaderTests.java:30:17:30:37 | getInputStream(...) | SAXReaderTests.java:30:17:30:37 | getInputStream(...) | SAXReaderTests.java:30:17:30:37 | getInputStream(...) | Unsafe parsing of XML file from $@. | SAXReaderTests.java:30:17:30:37 | getInputStream(...) | user input | From 732542adcb303e7fc7bf22acda64dd6baa884f99 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Wed, 9 Dec 2020 16:41:31 +0000 Subject: [PATCH 3/3] Add change note --- java/change-notes/2020-12-09-xxe-fp-fix.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 java/change-notes/2020-12-09-xxe-fp-fix.md diff --git a/java/change-notes/2020-12-09-xxe-fp-fix.md b/java/change-notes/2020-12-09-xxe-fp-fix.md new file mode 100644 index 00000000000..2ccf4bf369a --- /dev/null +++ b/java/change-notes/2020-12-09-xxe-fp-fix.md @@ -0,0 +1,4 @@ +lgtm,codescanning +* The query "Resolving XML external entity in user-controlled data" (`java/xxe`) has been improved to report fewer false positives when a `SAXParserFactory` is configured safely. + +