725663 - (CVE-2012-0453) [SECURITY] CSRF vulnerability in the XML-RPC API when using mod_perl

Status: RESOLVED FIXED

(Bugzilla :: WebService, defect)

Description

[Mario Gomes]

Attachment: PoC.html

User Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/535.19 (KHTML, like Gecko) Chrome/18.0.1025.7 Safari/535.19

Steps to reproduce:

Hello,

There is a CSRF vulnerability in Bugzilla XML-RPC. The vulnerability allows an attacker to make alterations to a bug, groups, etc.. The vulnerability is due to non-implementation of security tokens, thus allowing you to make HTTP requests, bugs and making alterations in groups.

Reproduce:
1. Log in  https://landfill.bugzilla.org/bugzilla-tip.
2. Open PoC.html
3. See the XML response.
4. Back to https://landfill.bugzilla.org/bugzilla-tip
4. See you logouted.

Note: My PoC only make the user logged be logout.

Cheers,
Mario.

Comment 1

[Mario Gomes]

A patch for this flaw would check the "Content-Type" correctly, allows only request only if "Content-Type: text/xml".

Comment 2

[:glob ✱]

confirming.

Comment 3

[Gervase Markham [:gerv]]

I'm not sure that the proposed fix in comment #1 will work; I would not like to bet on the web platform not already having, or not evolving, a way of sending POSTs of arbitrary MIME types.

Does our entire XML-RPC API not use tokens? <sigh>

Gerv

Comment 4

[:glob ✱]

this only impacts mod_perl installations, mod_cgi throws the following error:

  Content-Type must be 'text/xml,' 'multipart/*,' 'application/soap+xml,'
  'or 'application/dime' instead of 'text/plain'

Comment 5

[David Lawrence [:dkl]]

(In reply to Gervase Markham [:gerv] from comment #3)
> I'm not sure that the proposed fix in comment #1 will work; I would not like
> to bet on the web platform not already having, or not evolving, a way of
> sending POSTs of arbitrary MIME types.
> 
> Does our entire XML-RPC API not use tokens? <sigh>
> 
> Gerv

Also confirmed.

No tokens used in XMLRPC API. or JSONRPC API for that matter as that would require the client to get the bug contents before making an update similar to the web UI. We have just always
just require the user to login when making any changes either passing a valid cookie or username/password combination.

dkl

Comment 6

[Gervase Markham [:gerv]]

Note in passing that BzAPI does not have this problem:
https://wiki.mozilla.org/Bugzilla:REST_API#Authentication

However, it probably takes advantage of this issue in order to interact with Bugzilla on the back end!

What are the risks here? AIUI, direct data theft is not one of them, because of the same origin policy - an attacking page, unless it's on bugzilla.mozilla.org (or mozilla.org?) can't get at any returned data. But bugs can be defaced, or perhaps have group bits removed (!).

CCing mcoates: can the infrasec team give us an impact assessment, and details of best practices? Is this as bad as it seems?

Gerv

Comment 7

[Gervase Markham [:gerv]]

For info, here's a blog post describing this technique:
http://pentestmonkey.net/blog/csrf-xml-post-request

We could get the XML-RPC User.login operation to return a session key in the returned hash, and then require that on every request afterwards. This would, of course, break every single client...

Gerv

Comment 8

[Frédéric Buclin]

I'm a bit tired by these sec bugs. Why is the security of our API so badly implemented? And what's so special in mod_perl vs mod_cgi that only mod_perl is affected? Who throws the error about the Content-Type mentioned in comment 4?

Comment 9

[Mario Gomes]

Clickjacking in xmprpc.cgi + A old bug in Firefox(view-source:view-source:). May allow the exploitation of this Info-stealing easier(see 72580).

(In reply to Gervase Markham [:gerv] from comment #6)
> What are the risks here? AIUI, direct data theft is not one of them, because
> of the same origin policy - an attacking page, unless it's on
> bugzilla.mozilla.org (or mozilla.org?) can't get at any returned data. But
> bugs can be defaced, or perhaps have group bits removed (!).

Comment 10

[Max Kanat-Alexander]

(In reply to Frédéric Buclin from comment #8)
> I'm a bit tired by these sec bugs. Why is the security of our API so badly
> implemented?

  The JSON-RPC bug and this bug are most likely the result of me (or the other WebServices reviewers) not being aware that browsers could post in text/plain, and also us not being aware that browsers were sending XmlHttpRequests even though they were denying reading the responses.

> And what's so special in mod_perl vs mod_cgi that only mod_perl
> is affected? 

  I'm guessing this is some implementation complexity in either CGI.pm or SOAP::Lite.

Comment 11

[:glob ✱]

Attachment: patch v1

i think for branches we'll have to whitelist the content-type, as introducing token based auth would cause much breakage.

note firefox has a whitelist of content-types that it will allow the user to set (http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#2683) so for firefox at least this should be sufficient.  i haven't had a chance yet to experiment with other browsers.

Comment 12

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #11)
> note firefox has a whitelist of content-types that it will allow the user to
> set

Yeah, this is the same list as mentioned in bug 718319 comment 17. We blacklisted them to force the browser to wait for an answer from the server, which it will never get. Probably we could do the same here, for branches.

Comment 13

[Michael Coates [:mcoates] (acct no longer active)]

Adding mgoodwin to provide feedback per comment 6

Comment 15

[Mario Gomes]

I think that this patch is better(remove case sensitive problem, and also accept application/json).

sub deserialize {
    my $self = shift;
    my @types = ("text/xml","application/xml"); # XML Content types(see http://en.wikipedia.org/wiki/XML).
	foreach(@types){$content_type = $_;
   # Only allow text/xml to protect against CSRF attacks
    if (lc($ENV{'CONTENT_TYPE'}) ne $content_type) { # Remove case sensitive problem in MIME name.

        ThrowUserError('xmlrpc_illegal_content_type',
                       { content_type => $ENV{'CONTENT_TYPE'} });
    }
	}
...

Comment 16

[Mario Gomes]

I mean application/xml...

Comment 17

[David Lawrence [:dkl]]

Comment on attachment 596704 [details] [diff] [review]
patch v1

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

looks good but a few changes needed.

::: Bugzilla/WebService/Server/XMLRPC.pm
@@ +77,5 @@
>  sub deserialize {
>      my $self = shift;
> +
> +    # Only allow text/xml to protect against CSRF attacks
> +    if ($ENV{'CONTENT_TYPE'} ne 'text/xml') {

Also need to allow application/xml.

Make this a constant in Bugzilla/WebService/Constants.pm. There is already a 
constant for JSON-RPC so you may need to rename the old one and create new XMLRPC_CONTENT_TYPE_WHITELIST. 

use constant XMLRPC_CONTENT_TYPE_WHITELIST => qw(
    text/xml
    application/xml
);

@@ +79,5 @@
> +
> +    # Only allow text/xml to protect against CSRF attacks
> +    if ($ENV{'CONTENT_TYPE'} ne 'text/xml') {
> +        ThrowUserError('xmlrpc_illegal_content_type',
> +                       { content_type => $ENV{'CONTENT_TYPE'} });

{ content_type => $ENV{'CONTENT_TYPE'},
  content_type_list => XMLRPC_CONTENT_TYPE_WHITELIST }

::: template/en/default/global/user-error.html.tmpl
@@ +1707,5 @@
>      for details.)
>  
> +  [% ELSIF error == "xmlrpc_illegal_content_type" %]
> +    When using XML-RPC, you cannot send data as
> +    [%+ content_type FILTER html %]. Only text/xml is allowed.

[% ELSIF error == "xmlrpc_illegal_content_type" %]
    When using XML-RPC, you cannot send data as 
    [% content_type FILTER html %]. The allowed 
    content types are [% content_type_list.join(', ') FILTER html %].

Comment 18

[Frédéric Buclin]

(In reply to David Lawrence [:dkl] from comment #17)
> { content_type => $ENV{'CONTENT_TYPE'},
>   content_type_list => XMLRPC_CONTENT_TYPE_WHITELIST }

No need to pass constants to error messages. Constants are already accessible to templates, via [% constants.FOO %].

Comment 19

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #18)
> No need to pass constants to error messages. Constants are already
> accessible to templates, via [% constants.FOO %].

Not constants from Bugzilla/WebService/Constants.pm from what I can tell. We could definitely
patch Template.pm to do so but I would worry about naming conflicts.

dkl

Comment 20

[Frédéric Buclin]

(In reply to David Lawrence [:dkl] from comment #19)
> Not constants from Bugzilla/WebService/Constants.pm from what I can tell.

Ah right, I didn't check in which file the constant was put into. :)


> patch Template.pm to do so but I would worry about naming conflicts.

There should be no name conflicts between Bugzilla/Constants.pm and Bugzilla/WebService/Constants.pm as most Bugzilla/WebService/*.pm modules load both files. So no worry about that. If a constant name would be duplicated, this would be a bug.

Comment 21

[David Lawrence [:dkl]]

Attachment: Patch to allows only certain content-types for XMLRPC 4.0.x and 4.2 (v1)

Added to globs original patch. This fixes the issue in my testing.

dkl

Comment 22

[Frédéric Buclin]

Comment on attachment 597994 [details] [diff] [review]
Patch to allows only certain content-types for XMLRPC 4.0.x and 4.2 (v1)

>=== modified file 'Bugzilla/WebService/Server/XMLRPC.pm'

>+    if (!grep($_ eq $ENV{'CONTENT_TYPE'}, XMLRPC_CONTENT_TYPE_WHITELIST)) {

In bug 718319, I wrote that this variable could also contain the charset, which is why I had to use a regexp. Are we 100% sure no charset is passed here? I think a similar regexp as the one I used in bug 718319 would be safer.



>=== modified file 'template/en/default/global/user-error.html.tmpl'

>+  [% ELSIF error == "xmlrpc_illegal_content_type" %]
>+    When using XML-RPC, you cannot send data as
>+    [%+ content_type FILTER html %]. Only text/xml 
>+    and application/xml are allowed.

For trunk, you can use [% constants.XMLRPC_CONTENT_TYPE_WHITELIST.join(', ') FILTER html %] to enumerate allowed MIME types. Only branches need a hardcoded text here.


Otherwise your patch looks good.

Comment 23

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #22)
> Comment on attachment 597994 [details] [diff] [review]
> Patch to allows only certain content-types for XMLRPC (v2)
> 
> >=== modified file 'Bugzilla/WebService/Server/XMLRPC.pm'
> 
> >+    if (!grep($_ eq $ENV{'CONTENT_TYPE'}, XMLRPC_CONTENT_TYPE_WHITELIST)) {
> 
> In bug 718319, I wrote that this variable could also contain the charset,
> which is why I had to use a regexp. Are we 100% sure no charset is passed
> here? I think a similar regexp as the one I used in bug 718319 would be
> safer.

From what I can find, charset is generally not used for 'enctype' and is instead 
specified in 'accept-charset'. In my testing, the browser will fall back to the default when it doesn't recognize the 'enctype' value. One example I used enctype="text/plain;charset=utf8". The browser converted the submitted 'enctype' to application/x-www-form-urlencoded.

So I feel we can just leave it as a list of accepted content-types and not use a regex.
JSONRPC is different in that the content-type is specified in the HTTP header which allows for adding charset values.

> >=== modified file 'template/en/default/global/user-error.html.tmpl'
> 
> >+  [% ELSIF error == "xmlrpc_illegal_content_type" %]
> >+    When using XML-RPC, you cannot send data as
> >+    [%+ content_type FILTER html %]. Only text/xml 
> >+    and application/xml are allowed.
> 
> For trunk, you can use [% constants.XMLRPC_CONTENT_TYPE_WHITELIST.join(', ')
> FILTER html %] to enumerate allowed MIME types. Only branches need a
> hardcoded text here.
> 
> 
> Otherwise your patch looks good.

My mistake. The patch was meant for 4.2 and I did not specify in my description. I will upload the appropriate patch for trunk and the other branches.

dkl

Comment 24

[David Lawrence [:dkl]]

Interestingly, 3.4 is not affected by this as far as I can tell from my testing. Reason is that XMLRPC.pm uses XMLRPC::Transport::HTTP::CGI even when running under mod_perl. So text/plain is rejected automatically by SOAP::Lite.

This is the diff of 3.4 XMLRPC.pm to trunk XMLRPC.pm:

-our @ISA = qw(XMLRPC::Transport::HTTP::CGI Bugzilla::WebService::Server);
+if ($ENV{MOD_PERL}) {
+    our @ISA = qw(XMLRPC::Transport::HTTP::Apache Bugzilla::WebService::Server);
+} else {
+    our @ISA = qw(XMLRPC::Transport::HTTP::CGI Bugzilla::WebService::Server);
+}

dkl

Comment 25

[David Lawrence [:dkl]]

Attachment: Patch that allows only certain content-types for XMLRPC trunk (v1)

Patch for trunk that uses constants.XMLRPC_CONTENT_TYPE_WHITELIST in user-error.html.tmpl.

dkl

Comment 26

[Frédéric Buclin]

(In reply to David Lawrence [:dkl] from comment #24)
> Interestingly, 3.4 is not affected by this as far as I can tell from my
> testing. Reason is that XMLRPC.pm uses XMLRPC::Transport::HTTP::CGI even
> when running under mod_perl. So text/plain is rejected automatically by
> SOAP::Lite.

Does it mean that 3.4 is not affected *at all* by this issue, i.e. even when setting enctype to something else? Your answer is important because this would mean that we wouldn't need to release 3.4.15.

Comment 27

[Frédéric Buclin]

If the problem comes from XMLRPC::Transport::HTTP::Apache, then 3.6 isn't affected either as bug 600810 landed in 4.0.2 and 4.1.3 only. Retargetting to 4.0 unless someone can create a PoC against 3.6 or 3.4.

Comment 28

[Mario Gomes]

Is There a Landfill with mod_cgi?

Comment 29

[Frédéric Buclin]

(In reply to Mario Gomes from comment #28)
> Is There a Landfill with mod_cgi?

only bugzilla-tip on landfill uses mod_perl. All other installations use mod_cgi:
https://landfill.bugzilla.org/bugzilla-tip/testagent.cgi       (mod_perl)
https://landfill.bugzilla.org/bugzilla-4.2-branch/testagent.cgi (mod_cgi)
https://landfill.bugzilla.org/bugzilla-4.0-branch/testagent.cgi (mod_cgi)
https://landfill.bugzilla.org/bugzilla-3.6-branch/testagent.cgi (mod_cgi)
https://landfill.bugzilla.org/bugzilla-3.4-branch/testagent.cgi (mod_cgi)

Comment 30

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #27)
> If the problem comes from XMLRPC::Transport::HTTP::Apache, then 3.6 isn't
> affected either as bug 600810 landed in 4.0.2 and 4.1.3 only. Retargetting
> to 4.0 unless someone can create a PoC against 3.6 or 3.4.

The problem does come from the use of XMLRPC::Transport::HTTP::Apache which subclasses SOAP::Transport::HTTP::Apache. The latter uses Apache2::RequestUtil->request for obtaining the HTTP request headers. From what I can tell the request object does not have content_type proper let which causes the SOAP code to bypass it's internal test and allows the operation to continue. If Bugzilla uses XMLRPC::Transport::HTTP::CGI, it uses a standard HTTP::Request object and the content-type value is set properly. The SOAP code then sees the content-type and generates a fault response since it is not one of it's allowed content-types.

So given that, only Bugzilla releases that use XMLRPC::Transport::HTTP::Apache are affected which is currently 4.0.2 and up.

This narrows the release size a bit.

dkl

Comment 31

[Frédéric Buclin]

OK, thanks for your investigation.

Comment 32

[Frédéric Buclin]

Comment on attachment 598138 [details] [diff] [review]
Patch that allows only certain content-types for XMLRPC trunk (v1)

>+        ThrowUserError('xmlrpc_illegal_content_type',
>+                       { content_type => $ENV{'CONTENT_TYPE'} });

You must add an entry into WebServices/Constants.pm into WS_ERROR_CODE to uniquely identify this error. I suggest we add error id 32612 for 'xmlrpc_illegal_content_type'. I realize that I forgot to do the same when adding 'json_rpc_illegal_content_type'. Could you please give it id 32613?

r=LpSolit with error ids added. Please update your patch and carry forward my r+.

Comment 33

[Frédéric Buclin]

Comment on attachment 597994 [details] [diff] [review]
Patch to allows only certain content-types for XMLRPC 4.0.x and 4.2 (v1)

r=LpSolit, but please add missing error ids, see comment 32.

Comment 34

[Mario Gomes]

Could this vulnerability be elegible for a bounty?

Comment 35

[David Lawrence [:dkl]]

Attachment: Patch that allows only certain content-types for XMLRPC 4.0.x and 4.2 (v2)

Added error ids. Moving r+ forward.

Comment 36

[David Lawrence [:dkl]]

Attachment: Patch that allows only certain content-types for XMLRPC trunk (v2)

Added error ids. Moving forward r+.

Comment 37

[Frédéric Buclin]

a=LpSolit for immediate checkin.

Comment 38

[David Lawrence [:dkl]]

4.0:
Committing to: bzr+ssh://[e-mail]/bugzilla/4.0
modified Bugzilla/WebService/Server/XMLRPC.pm
modified template/en/default/global/user-error.html.tmpl
modified Bugzilla/WebService/Constants.pm
Committed revision 7695. 

4.2:
Committing to: bzr+ssh://[e-mail]/bugzilla/4.2
modified Bugzilla/WebService/Server/XMLRPC.pm
modified template/en/default/global/user-error.html.tmpl
modified Bugzilla/WebService/Constants.pm
Committed revision 8031.

trunk:
Committing to: bzr+ssh://[e-mail]/bugzilla/trunk
modified Bugzilla/WebService/Server/XMLRPC.pm
modified template/en/default/global/user-error.html.tmpl
modified Bugzilla/WebService/Constants.pm
Committed revision 8124.

Comment 39

[Frédéric Buclin]

Security advisory sent. Tarballs uploaded to the FTP server.

Comment 40

[Mario Gomes]

@Michael Coates: Can you please responde my question in comment 34? :)

Comment 41

[Reed Loden [:reed]]

(In reply to Mario Gomes from comment #40)
> @Michael Coates: Can you please responde my question in comment 34? :)

Please send questions regarding the bounty to security @ mozilla.org.

Comment 42

[Mario Gomes]

Okay.

Comment 43

[Akash Chugh]

For info, here's a blog post describing this technique:

http://www.techtrick.in

We could get the XML-RPC User.login operation to return a session key in the returned hash, and then require that on every request afterwards. This would, of course, break every single client...

Comment 44

[Dylan Hardison [:dylan] (he/him)]

With same-site cookies, is this still even possible?