1031035 - xmlrpc can be DoS'd with billion laughs attack

Status: RESOLVED FIXED

(Bugzilla :: WebService, defect)

Description

[Damien Nozay [:dnozay]]

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

Comment 1

[Andre Klapper]

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

Comment 2

[Damien Nozay [:dnozay]]

(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.

Comment 3

[:glob ✱]

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).

Comment 4

[Damien Nozay [:dnozay]]

(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?

Comment 5

[:glob ✱]

(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.

Comment 7

[Gervase Markham [:gerv]]

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

Comment 8

[Frédéric Buclin]

Attachment: fix for importxml.pl

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.

Comment 9

[Gervase Markham [:gerv]]

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

Gerv

Comment 10

[Frédéric Buclin]

Attachment: patch, v1

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.

Comment 11

[:glob ✱]

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.

Comment 12

[:glob ✱]

Attachment: 1031035_2.patch

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.

Comment 13

[Frédéric Buclin]

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

I agree this is better. :) r=LpSolit

Comment 14

[Frédéric Buclin]

All supported branches? Master only?

Comment 15

[Frédéric Buclin]

This is a good candidate for 5.0rc3.

Comment 16

[Reed Loden [:reed]]

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

Comment 17

[:glob ✱]

Attachment: 1031035_3.patch

- adds NoLWP

Comment 18

[Frédéric Buclin]

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.

Comment 19

[Frédéric Buclin]

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/);

Comment 20

[Reed Loden [:reed]]

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.

Comment 21

[Damien Nozay [:dnozay]]

(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...

Comment 22

[Frédéric Buclin]

(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.

Comment 23

[:glob ✱]

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).

Comment 24

[:glob ✱]

To ssh://[e-mail]/bugzilla/bugzilla.git
   6032799..c325240  master -> master

To ssh://[e-mail]/bugzilla/bugzilla.git
   05c0a40..2ca129b  5.0 -> 5.0

Comment 25

[:glob ✱]

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.

Comment 26

[:glob ✱]

Attachment: 1031035_2-fixup.patch

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.

Comment 27

[Frédéric Buclin]

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

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

Comment 28

[Teemu Mannermaa (:wicked)]

(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.

Comment 29

[Frédéric Buclin]

(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).

Comment 30

[:glob ✱]

To ssh://[e-mail]/bugzilla/bugzilla.git
   802a5cc..bdd9c47  master -> master

To ssh://[e-mail]/bugzilla/bugzilla.git
   b09ffb6..d5c47c9  5.0 -> 5.0

Comment 31

[Frédéric Buclin]

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

Comment 32

[Frédéric Buclin]

justdave: see comment 31.

Comment 33

[Dave Miller [:justdave]]

Yeah, this makes sense for a backport.

Comment 34

[Frédéric Buclin]

To ssh://[e-mail]/bugzilla/bugzilla.git
   79b334d..8beabdc  4.4 -> 4.4