787328 - xmlrpc.cgi doesn't send any security-related headers

Status: RESOLVED FIXED

(Bugzilla :: WebService, defect)

Description

[Reed Loden [:reed]]

xmlrpc.cgi doesn't seem to send any of the security-related headers that are sent for every other page. The one that worries me the most is the lack of X-Frame-Options...

$ curl -I https://landfill.bugzilla.org/bugzilla-tip/xmlrpc.cgi
HTTP/1.1 200 OK
Date: Fri, 31 Aug 2012 08:27:47 GMT
Server: Apache/2.2.3 (CentOS)
Connection: close
Content-Type: text/plain; charset=UTF-8

$ curl -I https://landfill.bugzilla.org/bugzilla-tip/jsonrpc.cgi
HTTP/1.1 403 Forbidden
Date: Fri, 31 Aug 2012 08:27:55 GMT
Server: Apache/2.2.3 (CentOS)
X-xss-protection: 1; mode=block
Strict-transport-security: max-age=604800
X-frame-options: SAMEORIGIN
X-content-type-options: nosniff
Connection: close
Content-Type: application/json; charset=UTF-8

Comment 1

[:glob ✱]

Attachment: patch v1

this patch copies across all X- headers that Bugzilla:CGI generates into xmlrpc responses.

i'm not totally happy with this patch, as i suspect there's an easier way to achieve this, but this does the job.

Comment 2

[David Lawrence [:dkl]]

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

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

I don't see another easy way to do it either as SOAP::Transport::HTTP.pm doesnt use CGI.pm and use its
own instance of HTTP::Response. Tested this on my local instance and works well.

r=dkl

Comment 3

[Reed Loden [:reed]]

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

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

This will miss the 'Strict-Transport-Security' header. Note that it's very likely that some of the other headers will be losing the 'X-' prefix shortly, as the various standards groups are deprecating it. Maybe instead of only sending the 'X-' prefixed headers, we send those that don't match the standard ones (whatever they might be)? That may be easier to handle, and it's less likely that a new header will be added that causes problems there vs. custom headers added in Bugzilla/CGI.pm.

Comment 4

[Frédéric Buclin]

Why is this bug filed with the security flag set? We are not talking about a security hole here, but only about some headers not returned and which are of no used when using XML-RPC. About JSON-RPC, it's a different story because it's fine to call jsonrpc.cgi from a web browser.

Comment 5

[David Lawrence [:dkl]]

Maybe this instead? Works for me in my testing:

    # Copy across security related headers from Bugzilla::CGI
    foreach my $header (split(/[\r\n]+/, Bugzilla->cgi->header)) {
        my ($name, $value) = $header =~ /^([^:]+): (.*)/;
        if (!$self->response->headers->header($name)) {
           $self->response->headers->header($name => $value);
        }
    }

It makes it more difficult due to CGI.pm not using HTTP::Headers so cannot use header_field_names to loop through the headers one at a time.

dkl

Comment 6

[Reed Loden [:reed]]

(In reply to Frédéric Buclin from comment #4)
> Why is this bug filed with the security flag set? We are not talking about a
> security hole here, but only about some headers not returned and which are
> of no used when using XML-RPC. About JSON-RPC, it's a different story
> because it's fine to call jsonrpc.cgi from a web browser.

You can call XML-RPC from a web browser, just not as easily as JSON-RPC (as in, you're likely to have to use JavaScript to do it).

Comment 7

[Frédéric Buclin]

(In reply to Reed Loden [:reed] from comment #6)
> You can call XML-RPC from a web browser, just not as easily as JSON-RPC (as
> in, you're likely to have to use JavaScript to do it).

You cannot call *our* XML-RPC API from a web browser, even with JavaScript. We explicitly blocked simple cross-site requests in bug 725663. And if you try to alter the Content-Type, your browser won't get anything back anyway.

Comment 8

[Reed Loden [:reed]]

dkl, can you attach an actual patch with what you said in comment #5?

Comment 9

[David Lawrence [:dkl]]

Attachment: patch v2

Comment 10

[Reed Loden [:reed]]

Comment on attachment 659613 [details] [diff] [review]
patch v2

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

This doesn't work with my testing...

$ curl -I .../jsonrpc.cgi
HTTP/1.1 403 Forbidden
Date: Mon, 10 Sep 2012 04:40:31 GMT
Server: Apache/2.2.3 (CentOS)
X-xss-protection: 1; mode=block
X-frame-options: SAMEORIGIN
X-content-type-options: nosniff
Connection: close
Content-Type: application/json; charset=UTF-8

$ curl -I .../xmlrpc.cgi
HTTP/1.1 411 Length Required
Date: Mon, 10 Sep 2012 04:40:41 GMT
Server: Apache/2.2.3 (CentOS)
Connection: close
Content-Type: text/plain; charset=UTF-8

Comment 11

[:glob ✱]

(In reply to Reed Loden [:reed] from comment #10)
> This doesn't work with my testing...

please test with a successful request; iirc failures take a different path in the code.

Comment 12

[Reed Loden [:reed]]

(In reply to Byron Jones ‹:glob› from comment #11)
> (In reply to Reed Loden [:reed] from comment #10)
> > This doesn't work with my testing...
> 
> please test with a successful request; iirc failures take a different path
> in the code.

Will test, but can we fix the failure path as well so it matches everything else?

Comment 13

[:glob ✱]

(In reply to Reed Loden [:reed] from comment #12)
> Will test, but can we fix the failure path as well so it matches everything
> else?

unfortunately not - it's deep in SOAP::Transport::HTTP

Comment 14

[:glob ✱]

Comment on attachment 659613 [details] [diff] [review]
patch v2

this works when you send a valid request:

T 127.0.0.1:45903 -> 127.0.0.1:80 [AP]
  POST /787328/xmlrpc.cgi HTTP/1.1..TE: deflate,gzip;q=0.3..Connection: TE, close..Accept: text/xml..Accept: multipart/*..Accept: 
  application/soap..Host: fedora..User-Agent: SOAP::Lite/Perl/0.712..Content-Length: 114..Content-Type: text/xml....<?xml version=
  "1.0" encoding="UTF-8"?><methodCall><methodName>Bugzilla.version</methodName><params /></methodCall>                            
##
T 127.0.0.1:80 -> 127.0.0.1:45903 [AP]
  HTTP/1.1 200 OK..Date: Mon, 12 Nov 2012 15:03:11 GMT..Server: Apache/2.2.22 (Fedora)..SOAPServer: SOAP::Lite/Perl/0.712..X-Conte
  nt-Type-Options: nosniff..X-Frame-Options: SAMEORIGIN..X-Xss-Protection: 1; mode=block..Content-Length: 207..Connection: close..
  Content-Type: text/xml....<?xml version="1.0" encoding="UTF-8"?><methodResponse><params><param><value><struct><member><name>vers
  ion</name><value><string>4.5</string></value></member></struct></value></param></params></methodResponse>

Comment 15

[Reed Loden [:reed]]

*poke* --- can we get this into a release?

Comment 16

[Frédéric Buclin]

(In reply to Reed Loden [:reed] from comment #15)
> *poke* --- can we get this into a release?

No, not until someone replies to comment 7.

Comment 17

[Reed Loden [:reed]]

(In reply to Frédéric Buclin from comment #16)
> (In reply to Reed Loden [:reed] from comment #15)
> > *poke* --- can we get this into a release?
> 
> No, not until someone replies to comment 7.

What part of comment #7 needs a reply? Even if this turns out to not be directly exploitable, it's still a bug, and we have a reviewed fix. Let's take it and get it into a release.

Comment 18

[Frédéric Buclin]

(In reply to Reed Loden [:reed] from comment #17)
> What part of comment #7 needs a reply? Even if this turns out to not be
> directly exploitable, it's still a bug

The part which needs a reply is "You cannot call *our* XML-RPC API from a web browser, even with JavaScript." So your comment 6 doesn't stand. The security headers are for the web browser, and so it's not a security bug to not send them if you cannot use your browser anyway.

If it's a security bug, the patch must land in all supported branches and need a security advisory. If it's simply a security enhancement, it should land in trunk + 4.4 only and doesn't need a security advisory. This makes a big difference. And from the comments above, I see no evidence that it's a security bug.

Comment 19

[Reed Loden [:reed]]

Do we allow GETs to xmlrpc.cgi to affect anything of consequence?

Comment 20

[Reed Loden [:reed]]

(In reply to Frédéric Buclin from comment #18)
> If it's a security bug, the patch must land in all supported branches and
> need a security advisory. If it's simply a security enhancement, it should
> land in trunk + 4.4 only and doesn't need a security advisory. This makes a
> big difference. And from the comments above, I see no evidence that it's a
> security bug.

Then let's treat it as a (non-security) bug, and land it on 4.0 and higher (since that's when we started sending X-Frame-Options and HSTS), yet not put it into a security advisory.

Comment 21

[Dave Miller [:justdave]]

I can go with comment 20's proposal.  Looks like we have enough stuff already on the 4.0 branch since the last release to be worth doing another release of it next time we have an excuse to push one (so we don't need to worry about doing a release just for this).

Comment 22

[David Lawrence [:dkl]]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk
modified Bugzilla/WebService/Server/XMLRPC.pm
Committed revision 8647.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.4
modified Bugzilla/WebService/Server/XMLRPC.pm
Committed revision 8576.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.2
modified Bugzilla/WebService/Server/XMLRPC.pm
Committed revision 8218.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.0
modified Bugzilla/WebService/Server/XMLRPC.pm
Committed revision 7752.