Removed lxml.etree.XMLParser from xml bomb sinks

Author: Napalys

python/ql/lib/semmle/python/frameworks/Lxml.qll

@@ -129,11 +129,6 @@ module Lxml {
             any(True t)
         )
         or
-        kind.isXmlBomb() and
-        this.getKeywordParameter("huge_tree").getAValueReachingSink().asExpr() = any(True t) and
-        not this.getKeywordParameter("resolve_entities").getAValueReachingSink().asExpr() =
-          any(False t)
-        or
         kind.isDtdRetrieval() and
         this.getKeywordParameter("load_dtd").getAValueReachingSink().asExpr() = any(True t) and
         this.getKeywordParameter("no_network").getAValueReachingSink().asExpr() = any(False t)
@@ -305,9 +300,8 @@ module Lxml {
       // note that there is no `resolve_entities` argument, so it's not possible to turn off XXE :O
       kind.isXxe()
       or
-      kind.isXmlBomb() and
-      this.getKeywordParameter("huge_tree").getAValueReachingSink().asExpr() = any(True t)
-      or
+      // libxml2 has built-in protection against XML bombs via entity reference loop detection,
+      // so lxml is not vulnerable to XML bomb attacks.
       kind.isDtdRetrieval() and
       this.getKeywordParameter("load_dtd").getAValueReachingSink().asExpr() = any(True t) and
       this.getKeywordParameter("no_network").getAValueReachingSink().asExpr() = any(False t)

python/ql/test/library-tests/frameworks/lxml/parsing.py

@@ -50,7 +50,7 @@
 
 # Billion laughs vuln (also XXE)
 parser = lxml.etree.XMLParser(huge_tree=True)
-lxml.etree.fromstring(x, parser=parser) # $ decodeFormat=XML decodeInput=x xmlVuln='XML bomb' xmlVuln='XXE' decodeOutput=lxml.etree.fromstring(..)
+lxml.etree.fromstring(x, parser=parser) # $ decodeFormat=XML decodeInput=x xmlVuln='XXE' decodeOutput=lxml.etree.fromstring(..)
 
 # Safe for both Billion laughs and XXE
 parser = lxml.etree.XMLParser(resolve_entities=False, huge_tree=True)
@@ -63,5 +63,5 @@
 # iterparse configurations ... this doesn't use a parser argument but takes MOST (!) of
 # the normal XMLParser arguments. Specifically, it doesn't allow disabling XXE :O
 
-lxml.etree.iterparse(xml_file, huge_tree=True) # $ decodeFormat=XML decodeInput=xml_file xmlVuln='XML bomb' xmlVuln='XXE' decodeOutput=lxml.etree.iterparse(..) getAPathArgument=xml_file
+lxml.etree.iterparse(xml_file, huge_tree=True) # $ decodeFormat=XML decodeInput=xml_file xmlVuln='XXE' decodeOutput=lxml.etree.iterparse(..) getAPathArgument=xml_file
 lxml.etree.iterparse(xml_file, load_dtd=True, no_network=False) # $ decodeFormat=XML decodeInput=xml_file xmlVuln='DTD retrieval' xmlVuln='XXE' decodeOutput=lxml.etree.iterparse(..) getAPathArgument=xml_file

python/ql/test/query-tests/Security/CWE-776-XmlBomb/XmlBomb.expected

@@ -1,14 +1,4 @@
 edges
-| test.py:1:26:1:32 | ControlFlowNode for ImportMember | test.py:1:26:1:32 | ControlFlowNode for request | provenance |  |
-| test.py:1:26:1:32 | ControlFlowNode for request | test.py:19:19:19:25 | ControlFlowNode for request | provenance |  |
-| test.py:19:5:19:15 | ControlFlowNode for xml_content | test.py:30:34:30:44 | ControlFlowNode for xml_content | provenance |  |
-| test.py:19:19:19:25 | ControlFlowNode for request | test.py:19:5:19:15 | ControlFlowNode for xml_content | provenance | AdditionalTaintStep |
 nodes
-| test.py:1:26:1:32 | ControlFlowNode for ImportMember | semmle.label | ControlFlowNode for ImportMember |
-| test.py:1:26:1:32 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
-| test.py:19:5:19:15 | ControlFlowNode for xml_content | semmle.label | ControlFlowNode for xml_content |
-| test.py:19:19:19:25 | ControlFlowNode for request | semmle.label | ControlFlowNode for request |
-| test.py:30:34:30:44 | ControlFlowNode for xml_content | semmle.label | ControlFlowNode for xml_content |
 subpaths
 #select
-| test.py:30:34:30:44 | ControlFlowNode for xml_content | test.py:1:26:1:32 | ControlFlowNode for ImportMember | test.py:30:34:30:44 | ControlFlowNode for xml_content | XML parsing depends on a $@ without guarding against uncontrolled entity expansion. | test.py:1:26:1:32 | ControlFlowNode for ImportMember | user-provided value |