Merge pull request #7240 from smowton/smowton/admin/derecognise-xxe-secure-processing

Note that FEATURE_SECURE_PROCESSING isn't a sufficient defence against XXE

Author: smowton

java/ql/lib/semmle/code/java/security/XmlParsers.qll

@@ -159,15 +159,6 @@ private class ConstantStringExpr extends Expr {
 Expr singleSafeConfig() {
   result.(ConstantStringExpr).getStringValue() =
     "http://apache.org/xml/features/disallow-doctype-decl"
-  or
-  result.(ConstantStringExpr).getStringValue() =
-    "http://javax.xml.XMLConstants/feature/secure-processing"
-  or
-  exists(Field f |
-    result = f.getAnAccess() and
-    f.hasName("FEATURE_SECURE_PROCESSING") and
-    f.getDeclaringType().hasQualifiedName("javax.xml", "XMLConstants")
-  )
 }
 
 /**

java/ql/test/query-tests/security/CWE-611/DocumentBuilderTests.java

@@ -25,14 +25,14 @@ public void enableSecurityFeature(Socket sock) throws Exception {
     DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
     factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
     DocumentBuilder builder = factory.newDocumentBuilder();
-    builder.parse(sock.getInputStream()); //safe
+    builder.parse(sock.getInputStream()); //unsafe -- secure-processing by itself is insufficient
   }
 
   public void enableSecurityFeature2(Socket sock) throws Exception {
     DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
     factory.setFeature("http://javax.xml.XMLConstants/feature/secure-processing", true);
     DocumentBuilder builder = factory.newDocumentBuilder();
-    builder.parse(sock.getInputStream()); //safe
+    builder.parse(sock.getInputStream()); //unsafe -- secure-processing by itself is insufficient
   }
 
   public void enableDTD(Socket sock) throws Exception {

java/ql/test/query-tests/security/CWE-611/XXE.expected

@@ -77,6 +77,8 @@ edges
 | XPathExpressionTests.java:27:37:27:57 | getInputStream(...) : InputStream | XPathExpressionTests.java:27:21:27:58 | new InputSource(...) |
 nodes
 | DocumentBuilderTests.java:14:19:14:39 | getInputStream(...) | semmle.label | getInputStream(...) |
+| DocumentBuilderTests.java:28:19:28:39 | getInputStream(...) | semmle.label | getInputStream(...) |
+| DocumentBuilderTests.java:35:19:35:39 | getInputStream(...) | semmle.label | getInputStream(...) |
 | DocumentBuilderTests.java:42:19:42:39 | getInputStream(...) | semmle.label | getInputStream(...) |
 | DocumentBuilderTests.java:49:19:49:39 | getInputStream(...) | semmle.label | getInputStream(...) |
 | DocumentBuilderTests.java:64:19:64:39 | getInputStream(...) | semmle.label | getInputStream(...) |
@@ -250,6 +252,8 @@ nodes
 subpaths
 #select
 | DocumentBuilderTests.java:14:19:14:39 | getInputStream(...) | DocumentBuilderTests.java:14:19:14:39 | getInputStream(...) | DocumentBuilderTests.java:14:19:14:39 | getInputStream(...) | Unsafe parsing of XML file from $@. | DocumentBuilderTests.java:14:19:14:39 | getInputStream(...) | user input |
+| DocumentBuilderTests.java:28:19:28:39 | getInputStream(...) | DocumentBuilderTests.java:28:19:28:39 | getInputStream(...) | DocumentBuilderTests.java:28:19:28:39 | getInputStream(...) | Unsafe parsing of XML file from $@. | DocumentBuilderTests.java:28:19:28:39 | getInputStream(...) | user input |
+| DocumentBuilderTests.java:35:19:35:39 | getInputStream(...) | DocumentBuilderTests.java:35:19:35:39 | getInputStream(...) | DocumentBuilderTests.java:35:19:35:39 | getInputStream(...) | Unsafe parsing of XML file from $@. | DocumentBuilderTests.java:35:19:35:39 | getInputStream(...) | user input |
 | DocumentBuilderTests.java:42:19:42:39 | getInputStream(...) | DocumentBuilderTests.java:42:19:42:39 | getInputStream(...) | DocumentBuilderTests.java:42:19:42:39 | getInputStream(...) | Unsafe parsing of XML file from $@. | DocumentBuilderTests.java:42:19:42:39 | getInputStream(...) | user input |
 | DocumentBuilderTests.java:49:19:49:39 | getInputStream(...) | DocumentBuilderTests.java:49:19:49:39 | getInputStream(...) | DocumentBuilderTests.java:49:19:49:39 | getInputStream(...) | Unsafe parsing of XML file from $@. | DocumentBuilderTests.java:49:19:49:39 | getInputStream(...) | user input |
 | DocumentBuilderTests.java:64:19:64:39 | getInputStream(...) | DocumentBuilderTests.java:64:19:64:39 | getInputStream(...) | DocumentBuilderTests.java:64:19:64:39 | getInputStream(...) | Unsafe parsing of XML file from $@. | DocumentBuilderTests.java:64:19:64:39 | getInputStream(...) | user input |