Bug 1086699

Summary: libguestfs should parse XML with XML_PARSE_NONET flags
Product: [Community] Virtualization Tools Reporter: Richard W.M. Jones <rjones>
Component: libguestfsAssignee: Pino Toscano <ptoscano>
Status: CLOSED UPSTREAM QA Contact:
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: unspecifiedCC: acathrow, bengen+rhbz, berrange, mbooth, ptoscano, rjones
Target Milestone: ---   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2014-05-06 17:28:33 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On: 1088290, 1090976    
Bug Blocks:    
Attachments:
Description Flags
xmlParse{File,Memory} -> xmlRead{File,Memory} none

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.