xmlrpc can be DoS'd with billion laughs attack

RESOLVED FIXED in Bugzilla 4.4

Status

()

Bugzilla
WebService
--
major
RESOLVED FIXED
3 years ago
2 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

3 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

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

Comment 2

3 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

3 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

3 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

3 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

2 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

2 years ago
Created attachment 8589101 [details] [diff] [review]
fix for importxml.pl

XML::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

Comment 10

2 years ago
Created attachment 8589403 [details] [diff] [review]
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.
Attachment #8589101 - Attachment is obsolete: true
Attachment #8589403 - Flags: review?(glob)
(Assignee)

Comment 11

2 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

2 years ago
Created attachment 8589498 [details] [diff] [review]
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.
Attachment #8589498 - Flags: review?(LpSolit)

Comment 13

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

I agree this is better. :) r=LpSolit
Attachment #8589498 - Flags: review?(LpSolit) → review+

Comment 14

2 years ago
All supported branches? Master only?
Assignee: webservice → glob
Status: NEW → ASSIGNED
Flags: approval?

Comment 15

2 years ago
This is a good candidate for 5.0rc3.
Flags: approval5.0?
(Assignee)

Updated

2 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

2 years ago
Created attachment 8590689 [details] [diff] [review]
1031035_3.patch

- adds NoLWP
Attachment #8589498 - Attachment is obsolete: true
Attachment #8590689 - Flags: review?(LpSolit)

Comment 18

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

Comment 19

2 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/);
Attachment #8590689 - Flags: review?(LpSolit) → review-

Updated

2 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

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

Comment 22

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

Updated

2 years ago
Attachment #8590689 - Attachment is obsolete: true
(Assignee)

Comment 23

2 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

2 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: 2 years ago
Resolution: --- → FIXED

Updated

2 years ago
Target Milestone: --- → Bugzilla 5.0
(Assignee)

Comment 25

2 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

2 years ago
Created attachment 8592095 [details] [diff] [review]
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.
Attachment #8592095 - Flags: review?(LpSolit)

Comment 27

2 years ago
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.

Comment 29

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

Comment 30

2 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

2 years ago
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED

Comment 31

2 years ago
glob, wouldn't it make sense to take this patch for 4.4 too?
Flags: needinfo?(glob)
(Assignee)

Updated

2 years ago
Flags: needinfo?(glob)
(Assignee)

Updated

2 years ago
Flags: approval4.4?

Comment 32

2 years ago
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 → ---

Comment 34

2 years ago
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   79b334d..8beabdc  4.4 -> 4.4
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 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.