Damien Nozay [:dnozay] Reporter
	Description • 10 years ago

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/35.0.1916.153 Safari/537.36

Steps to reproduce:

- used a python script to send a malicious xml file to my instance of bugzilla.
  (see https://gist.github.com/dnozay/6cabeea56caaf2fdd990)



Actual results:

- cpu usage 100%
- memory usage near 100%
- basically a DoS


Expected results:

- nothing

	Andre Klapper
	Comment 1 • 10 years ago

4.4.1 is ancient. Please try 4.4.4 which is the latest stable release and report back.

	Damien Nozay [:dnozay] Reporter
	Comment 2 • 10 years ago

(In reply to Andre Klapper from comment #1)
> 4.4.1 is ancient. Please try 4.4.4 which is the latest stable release and
> report back.

I don't currently have the time to do that. But I will update this bug when I do.
This should be trivial to test for any active bugzilla developer.

It would not be difficult to try out on b.m.o if xmlrpc is enabled, but it's
not ethical to try and bring down a production server.

	:glob ✱ Assignee
	Comment 3 • 10 years ago

it wasn't mentioned in our relnotes between 4.4.1 and 4.4.4, so upgrading isn't required to test.
i can confirm the DOS works against trunk.

"billion laughs" is an xml library issue, which for us resolves to expat (via XML::Parser).

	Damien Nozay [:dnozay] Reporter
	Comment 4 • 10 years ago

(In reply to Byron Jones ‹:glob› from comment #3)
> it wasn't mentioned in our relnotes between 4.4.1 and 4.4.4, so upgrading
> isn't required to test.
> i can confirm the DOS works against trunk.
> 
> "billion laughs" is an xml library issue, which for us resolves to expat
> (via XML::Parser).

Is xmlrpc enabled for any production bugzilla instances?
Do you need to disable xmlrpc until the issue is taken care of?

	:glob ✱ Assignee
	Comment 5 • 10 years ago

(In reply to Damien Nozay [:dnozay] from comment #4)
> Is xmlrpc enabled for any production bugzilla instances?

no, but it's enabled on most.

> Do you need to disable xmlrpc until the issue is taken care of?

disabling xmlrpc would disable a lot of features within bugzilla.

given the age of this issue, and that it's a DOS attack instead of a security issue, i don't think that disabling xmlrpc is warranted in this case.

	Gervase Markham [:gerv]
	Comment 7 • 9 years ago

From Googling, it's not at all clear whether this is fixed in Expat, or which versions have the fix, and how one would require a certain version of expat when using XML::Parser...

Gerv

	Frédéric Buclin
	Comment 8 • 9 years ago

ML::Parser has a NoExpand attribute to disable entity expansion. In importxml.pl, this is a one-liner, which must be called before parsing the XML file, see my patch. But I'm unable to do something similar in our XMLRPC code. But this can give you some starting point for investigation.

	Gervase Markham [:gerv]
	Comment 9 • 9 years ago

Would setting NoExpand have any unfortunate side-effects? Are we expecting entities of this sort in any of our incoming XML?

Gerv

	Frédéric Buclin
	Comment 10 • 9 years ago

This fixes both importxml.pl and XMLRPC. I didn't find a way to pass NoExpand => 1 to SOAP::Parser->xmlparser, so I had to override it.

	:glob ✱ Assignee
	Comment 11 • 9 years ago

Comment on attachment 8589403 [details] [diff] [review]
patch, v1

Review of attachment 8589403 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/WebService/Server/XMLRPC.pm
@@ +53,5 @@
> +    # No entity definition is expected anyway.
> +    *SOAP::Parser::xmlparser = sub {
> +        require XML::Parser;
> +        return XML::Parser->new(NoExpand => 1, Handlers => { Default => sub {} });
> +    };

replacing a soap::lite internal method with our own isn't an approach that i'm comfortable with.  there's no guarantee that this will work with all SOAP::Lite versions.

this also generates
XMLRPC.pm: Subroutine SOAP::Parser::xmlparser redefined at Bugzilla/WebService/Server/XMLRPC.pm line 57.

	:glob ✱ Assignee
	Comment 12 • 9 years ago

here's an alternative approach, which sets _parser during construction.

it's still messing about with an object's internals, but i feel this is less invasive than overwriting a whole sub.

	Frédéric Buclin
	Comment 13 • 9 years ago

Comment on attachment 8589498 [details] [diff] [review]
1031035_2.patch

I agree this is better. :) r=LpSolit

	Frédéric Buclin
	Comment 14 • 9 years ago

All supported branches? Master only?

	Frédéric Buclin
	Comment 15 • 9 years ago

This is a good candidate for 5.0rc3.

	Reed Loden [:reed]
	Comment 16 • 9 years ago

Please also set NoLWP => 1, as you don't want your XML parser making network calls.

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

	:glob ✱ Assignee
	Comment 17 • 9 years ago

- adds NoLWP

	Frédéric Buclin
	Comment 18 • 9 years ago

Comment on attachment 8590689 [details] [diff] [review]
1031035_3.patch

>+        NoLWP    => 1,  # prevent "external entity" attack

This parameter doesn't help at all, because SOAP::Parser::decode() contains:

    $self->parser->setHandlers(
        Final => sub { shift; $self->final(@_) },
        Start => sub { shift; $self->start(@_) },
        End   => sub { shift; $self->end(@_)   },
        Char  => sub { shift; $self->char(@_)  },
        ExternEnt => sub { shift; die "External entity (pointing to '$_[1]') is not allowed" },
    );


And as explained in the XML::Parser documentation, NoLWP has no effect in this case:

"NoLWP  This option has no effect if the ExternEnt or ExternEntFin handlers are directly set."


With or without NoLWP, you always get:

  Application failed during request deserialization: External entity (pointing to '...') is not allowed at lib/SOAP/Lite.pm line 1763.

	Frédéric Buclin
	Comment 19 • 9 years ago

Comment on attachment 8590689 [details] [diff] [review]
1031035_3.patch

SOAP::Lite has a test to make sure that external entities are not allowed, making NoLWP useless, see http://cpansearch.perl.org/src/PHRED/SOAP-Lite-1.14/t/01-core.t

print "Test of XML::Parser External Entity vulnerability...\n";
  UNIVERSAL::isa(SOAP::Deserializer->parser->parser => 'XML::Parser::Lite') ?
    skip(q!External entity references are not supported in XML::Parser::Lite! => undef) :
    ok(!eval { SOAP::Deserializer->deserialize('<?xml version="1.0"?><!DOCTYPE foo [ <!ENTITY ll SYSTEM "foo.txt"> ]><root>&ll;</root>')->root } and $@ =~ /^External entity/);

	Reed Loden [:reed]
	Comment 20 • 9 years ago

What happens if SOAP::Lite removes that change or breaks it somehow? Then we've opened ourselves up to a security issue. I don't see a reason why *not* to keep NoLWP in, even if it's a NO-OP.

	Damien Nozay [:dnozay] Reporter
	Comment 21 • 9 years ago

(In reply to Reed Loden [:reed] (use needinfo?) from comment #20)
> What happens if SOAP::Lite removes that change or breaks it somehow? Then
> we've opened ourselves up to a security issue. I don't see a reason why
> *not* to keep NoLWP in, even if it's a NO-OP.

For all production deployments, it would be best to freeze the package version.
You could also have negative tests for entity expansion to catch the hole opening up...

	Frédéric Buclin
	Comment 22 • 9 years ago

(In reply to Reed Loden [:reed] (use needinfo?) from comment #20)
> What happens if SOAP::Lite removes that change or breaks it somehow?

I doubt they will intentionally remove a test called "Test of XML::Parser External Entity vulnerability". And if the test fails, install-module.pl will refuse to install the new version of SOAP::Lite anyway. NoLWP would make sense if it allowed us to skip ExternEnt entirely, i.e. simply ignore external entities instead of dying. Anyway, the comment is misleading as it currently doesn't prevent anything.

	:glob ✱ Assignee
	Comment 23 • 9 years ago

let's go with the v2 patch, without the redundant NoLWP param.  i find it extremely unlikely that soap:lite's protection against this issue will be removed, especially since they have a test for it (ie. there's no need to guard against this).

	:glob ✱ Assignee
	Comment 24 • 9 years ago

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   6032799..c325240  master -> master

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   05c0a40..2ca129b  5.0 -> 5.0

	:glob ✱ Assignee
	Comment 25 • 9 years ago

this doesn't work with all SOAP::Lite versions...
in SOAP::Parser..

v0.712 has:

sub parser {
    my $self = shift->new;
    @_
        ? do {
            $self->{'_parser'} = shift;
            return $self;
        }
        : return ($self->{'_parser'} ||= $self->xmlparser);
}

while v1.13 has:

sub parser {
    my $self = shift->new;
    # set the parser if passed
    if (my $parser = shift) {
        $self->{'_parser'} = shift;
        return $self;
    }
    # else return the parser or use XML::Parser::Lite
    return ($self->{'_parser'} ||= $self->xmlparser);
}


note later versions have 3 shifts, and ignore first parameter.

	:glob ✱ Assignee
	Comment 26 • 9 years ago

since patch_2 has already been committed, this patch sits atop it.
it passes the same object twice to cover both implementations of the parser sub.

	Frédéric Buclin
	Comment 27 • 9 years ago

Comment on attachment 8592095 [details] [diff] [review]
1031035_2-fixup.patch

I didn't test it, but it looks good. r=LpSolit

	Teemu Mannermaa (:wicked)
	Comment 28 • 9 years ago

(In reply to Frédéric Buclin from comment #22)
> External Entity vulnerability". And if the test fails, install-module.pl
> will refuse to install the new version of SOAP::Lite anyway. NoLWP would

Not that it changes things much but install-module.pl orders CPAN to skip tests due to performance and thus they have no bearing on whether the installation succeeds or not.

	Frédéric Buclin
	Comment 29 • 9 years ago

(In reply to Teemu Mannermaa (:wicked) from comment #28)
> Not that it changes things much but install-module.pl orders CPAN to skip
> tests due to performance and thus they have no bearing on whether the
> installation succeeds or not.

Hum, right. I had compilation errors in mind (because I saw some dependencies having errors and the whole installation process stopped).

	:glob ✱ Assignee
	Comment 30 • 9 years ago

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   802a5cc..bdd9c47  master -> master

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   b09ffb6..d5c47c9  5.0 -> 5.0

	Frédéric Buclin
	Comment 31 • 9 years ago

glob, wouldn't it make sense to take this patch for 4.4 too?

	Frédéric Buclin
	Comment 32 • 9 years ago

justdave: see comment 31.

	Dave Miller [:justdave]
	Comment 33 • 9 years ago

Yeah, this makes sense for a backport.

	Frédéric Buclin
	Comment 34 • 9 years ago

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   79b334d..8beabdc  4.4 -> 4.4