Bug 1086699 - libguestfs should parse XML with XML_PARSE_NONET flags
Summary: libguestfs should parse XML with XML_PARSE_NONET flags
Keywords:
Status: CLOSED UPSTREAM
Alias: None
Product: Virtualization Tools
Classification: Community
Component: libguestfs
Version: unspecified
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Pino Toscano
QA Contact:
URL:
Whiteboard:
Depends On: CVE-2014-0179, CVE-2014-5177 CVE-2014-0191
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-04-11 10:53 UTC by Richard W.M. Jones
Modified: 2014-05-06 17:28 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-05-06 17:28:33 UTC
Embargoed:


Attachments (Terms of Use)
xmlParse{File,Memory} -> xmlRead{File,Memory} (1.53 KB, patch)
2014-04-11 14:29 UTC, Pino Toscano
no flags Details | Diff

Description Richard W.M. Jones 2014-04-11 10:53:15 UTC
Description of problem:

libguestfs parses XML files in various places:

 - libvirt capabilities XML
 - libvirt domain XML
 - libosinfo XML database files

In the unlikely case that either libvirt or libosinfo included
external entities then this means we could open network
connections unintentionally.

It's not clear that this is exploitable, but it's best to
avoid it.

For comparison of how to do it correctly, see libvirt
src/util/virxml.c.

Version-Release number of selected component (if applicable):

libguestfs 1.27.3

Additional info:

https://www.owasp.org/index.php/XML_External_Entity_%28XXE%29_Processing

Note that "XML_PARSE_NOENT" is very confusing.  Setting this
flag causes entities to be *parsed*!!

It seems we should set XML_PARSE_NONET (disable network connections).  It's
not clear to me if this is the default or not.

Areas of concern:

$ git grep xmlParse
fish/uri.c:  uri = xmlParseURI (arg);
src/launch-libvirt.c:  doc = xmlParseMemory (capabilities_xml, strlen (capabilities_xml));
src/libvirt-domain.c:  doc = xmlParseMemory (xml, strlen (xml));
src/osinfo.c:  doc = xmlParseFile (pathname);
v2v/xml-c.c:  doc = xmlParseMemory (String_val (xmlv), caml_string_length (xmlv));
v2v/xml.mli:(** xmlParseMemory *)

Comment 1 Pino Toscano 2014-04-11 14:28:10 UTC
(In reply to Richard W.M. Jones from comment #0)
> Areas of concern:
> [...]
> fish/uri.c:  uri = xmlParseURI (arg);

This just parses an URI string, so should not matter.

> v2v/xml-c.c:  doc = xmlParseMemory (String_val (xmlv), caml_string_length
> (xmlv));
> v2v/xml.mli:(** xmlParseMemory *)

These don't exist in master yet.

> src/launch-libvirt.c:  doc = xmlParseMemory (capabilities_xml, strlen
> (capabilities_xml));
> src/libvirt-domain.c:  doc = xmlParseMemory (xml, strlen (xml));
> src/osinfo.c:  doc = xmlParseFile (pathname);

It seems these don't do entities expansion by default, so
https://www.owasp.org/index.php/XML_External_Entity_%28XXE%29_Processing
should not apply.

However, to be sure, I replaced xmlParse{File,Memory} with xmlRead{File,Memory}, so we can explicitly set XML_PARSE_NONET (and other flags may be added, if needed). The result seems unchanged.

Comment 2 Pino Toscano 2014-04-11 14:29:41 UTC
Created attachment 885457 [details]
xmlParse{File,Memory} -> xmlRead{File,Memory}

Comment 3 Richard W.M. Jones 2014-04-11 14:37:36 UTC
ACK.  Let's not push this yet until the libvirt patch has been
made public.

Comment 4 Richard W.M. Jones 2014-04-16 12:30:37 UTC
This bug is also embargoed until the dependent libvirt bug is published.

Comment 5 Richard W.M. Jones 2014-04-30 15:25:23 UTC
The publication date is: Tuesday May 6th at 1200 UTC.

Comment 6 Pino Toscano 2014-05-06 17:28:33 UTC
Patch committed as
https://github.com/libguestfs/libguestfs/commit/845daded5fddc70fc5e822769bc1e2a8cbead7ca
which is in libguestfs >= 1.27.9.


Note You need to log in before you can comment on or make changes to this bug.