Closed
Bug 1031035
Opened 10 years ago
Closed 8 years ago
xmlrpc can be DoS'd with billion laughs attack
Categories
(Bugzilla :: WebService, defect)
Tracking
()
RESOLVED
FIXED
Bugzilla 4.4
People
(Reporter: dnozay, Assigned: glob)
References
Details
Attachments
(2 files, 3 obsolete files)
1.22 KB,
patch
|
LpSolit
:
review+
|
Details | Diff | Splinter Review |
655 bytes,
patch
|
LpSolit
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
4.4.1 is ancient. Please try 4.4.4 which is the latest stable release and report back.
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.
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•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?
Severity: normal → critical
(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
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
![]() |
||
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.
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
![]() |
||
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.
Attachment #8589101 -
Attachment is obsolete: true
Attachment #8589403 -
Flags: review?(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.
Attachment #8589403 -
Flags: review?(glob) → review-
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.
Attachment #8589498 -
Flags: review?(LpSolit)
![]() |
||
Comment 13•9 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•9 years ago
|
||
All supported branches? Master only?
Assignee: webservice → glob
Status: NEW → ASSIGNED
Flags: approval?
Attachment #8589403 -
Attachment is obsolete: true
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
Assignee | ||
Comment 17•9 years ago
|
||
- adds NoLWP
Attachment #8589498 -
Attachment is obsolete: true
Attachment #8590689 -
Flags: review?(LpSolit)
![]() |
||
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.
![]() |
||
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>≪</root>')->root } and $@ =~ /^External entity/);
Attachment #8590689 -
Flags: review?(LpSolit) → review-
![]() |
||
Updated•9 years ago
|
Attachment #8589498 -
Attachment is obsolete: false
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.
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...
![]() |
||
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.
Attachment #8590689 -
Attachment is obsolete: true
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).
Flags: approval?
Flags: approval5.0?
Flags: approval5.0+
Flags: approval+
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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
![]() |
||
Updated•9 years ago
|
Target Milestone: --- → Bugzilla 5.0
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #8592095 -
Flags: review?(LpSolit)
![]() |
||
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
Attachment #8592095 -
Flags: review?(LpSolit) → review+
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.
![]() |
||
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).
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
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
![]() |
||
Comment 31•9 years ago
|
||
glob, wouldn't it make sense to take this patch for 4.4 too?
Flags: needinfo?(glob)
Comment 33•8 years ago
|
||
Yeah, this makes sense for a backport.
Status: RESOLVED → REOPENED
Flags: needinfo?(justdave)
Flags: approval4.4?
Flags: approval4.4+
Resolution: FIXED → ---
![]() |
||
Comment 34•8 years ago
|
||
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git 79b334d..8beabdc 4.4 -> 4.4
Status: REOPENED → RESOLVED
Closed: 9 years ago → 8 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.
Description
•