xmlrpc can be DoS'd with billion laughs attack

RESOLVED FIXED in Bugzilla 4.4

Status

()

defect
--
major
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: dnozay, Assigned: glob)

Tracking

4.4.1
Bugzilla 4.4
Bug Flags:
approval +
approval5.0 +
approval4.4 +

Details

Attachments

(2 attachments, 3 obsolete attachments)

Reporter

Description

5 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

Comment 1

5 years ago
4.4.1 is ancient. Please try 4.4.4 which is the latest stable release and report back.
Reporter

Comment 2

5 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.
Assignee

Comment 3

5 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).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter

Comment 4

5 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?
Severity: normal → critical
Assignee

Comment 5

5 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.
Severity: critical → major
Assignee

Updated

4 years ago
Duplicate of this bug: 1151719
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

4 years ago
Posted patch fix for importxml.pl (obsolete) — Splinter Review
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.
Would setting NoExpand have any unfortunate side-effects? Are we expecting entities of this sort in any of our incoming XML?

Gerv
Posted patch patch, v1 (obsolete) — Splinter Review
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.
Attachment #8589101 - Attachment is obsolete: true
Attachment #8589403 - Flags: review?(glob)
Assignee

Comment 11

4 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.
Attachment #8589403 - Flags: review?(glob) → review-
Assignee

Comment 12

4 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.
Attachment #8589498 - Flags: review?(LpSolit)
Comment on attachment 8589498 [details] [diff] [review]
1031035_2.patch

I agree this is better. :) r=LpSolit
Attachment #8589498 - Flags: review?(LpSolit) → review+
All supported branches? Master only?
Assignee: webservice → glob
Status: NEW → ASSIGNED
Flags: approval?
This is a good candidate for 5.0rc3.
Flags: approval5.0?
Assignee

Updated

4 years ago
Attachment #8589403 - Attachment is obsolete: true
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
Assignee

Comment 17

4 years ago
Posted patch 1031035_3.patch (obsolete) — Splinter Review
- adds NoLWP
Attachment #8589498 - Attachment is obsolete: true
Attachment #8590689 - Flags: review?(LpSolit)
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 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/);
Attachment #8590689 - Flags: review?(LpSolit) → review-

Updated

4 years ago
Attachment #8589498 - Attachment is obsolete: false
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.
Reporter

Comment 21

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

Updated

4 years ago
Attachment #8590689 - Attachment is obsolete: true
Assignee

Comment 23

4 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).
Flags: approval?
Flags: approval5.0?
Flags: approval5.0+
Flags: approval+
Assignee

Comment 24

4 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
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Target Milestone: --- → Bugzilla 5.0
Assignee

Comment 25

4 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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 26

4 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.
Attachment #8592095 - Flags: review?(LpSolit)
Comment on attachment 8592095 [details] [diff] [review]
1031035_2-fixup.patch

I didn't test it, but it looks good. r=LpSolit
Attachment #8592095 - Flags: review?(LpSolit) → review+
(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.
(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).
Assignee

Comment 30

4 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
Assignee

Updated

4 years ago
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
glob, wouldn't it make sense to take this patch for 4.4 too?
Flags: needinfo?(glob)
Assignee

Updated

4 years ago
Flags: needinfo?(glob)
Assignee

Updated

4 years ago
Flags: approval4.4?
justdave: see comment 31.
Flags: needinfo?(justdave)
Yeah, this makes sense for a backport.
Status: RESOLVED → REOPENED
Flags: needinfo?(justdave)
Flags: approval4.4?
Flags: approval4.4+
Resolution: FIXED → ---
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   79b334d..8beabdc  4.4 -> 4.4
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: Bugzilla 5.0 → Bugzilla 4.4
You need to log in before you can comment on or make changes to this bug.