Last Comment Bug 787328 - xmlrpc.cgi doesn't send any security-related headers
: xmlrpc.cgi doesn't send any security-related headers
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: WebService (show other bugs)
: 4.3.2
: All All
: -- normal (vote)
: Bugzilla 4.0
Assigned To: David Lawrence [:dkl]
: default-qa
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-31 01:31 PDT by Reed Loden [:reed] (use needinfo?)
Modified: 2013-07-22 01:20 PDT (History)
6 users (show)
justdave: approval+
justdave: approval4.4+
justdave: approval4.2+
justdave: approval4.0+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (957 bytes, patch)
2012-08-31 02:20 PDT, Byron Jones ‹:glob› [PTO until 2017-01-09]
reed: review-
dkl: review+
Details | Diff | Splinter Review
patch v2 (970 bytes, patch)
2012-09-09 18:26 PDT, David Lawrence [:dkl]
glob: review+
Details | Diff | Splinter Review

Description Reed Loden [:reed] (use needinfo?) 2012-08-31 01:31:06 PDT
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 Byron Jones ‹:glob› [PTO until 2017-01-09] 2012-08-31 02:20:09 PDT
Created attachment 657196 [details] [diff] [review]
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] 2012-08-31 08:58:29 PDT
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] (use needinfo?) 2012-08-31 09:11:44 PDT
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 2012-08-31 09:24:57 PDT
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] 2012-08-31 09:37:44 PDT
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] (use needinfo?) 2012-08-31 09:41:07 PDT
(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 2012-08-31 09:48:42 PDT
(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] (use needinfo?) 2012-09-09 16:05:26 PDT
dkl, can you attach an actual patch with what you said in comment #5?
Comment 9 David Lawrence [:dkl] 2012-09-09 18:26:46 PDT
Created attachment 659613 [details] [diff] [review]
patch v2
Comment 10 Reed Loden [:reed] (use needinfo?) 2012-09-09 21:41:29 PDT
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 Byron Jones ‹:glob› [PTO until 2017-01-09] 2012-09-09 21:42:47 PDT
(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] (use needinfo?) 2012-09-09 21:48:59 PDT
(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 Byron Jones ‹:glob› [PTO until 2017-01-09] 2012-09-09 22:14:10 PDT
(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 Byron Jones ‹:glob› [PTO until 2017-01-09] 2012-11-12 07:04:19 PST
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] (use needinfo?) 2013-02-18 23:41:08 PST
*poke* --- can we get this into a release?
Comment 16 Frédéric Buclin 2013-02-19 01:15:57 PST
(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] (use needinfo?) 2013-02-19 01:34:54 PST
(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 2013-02-19 01:47:22 PST
(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] (use needinfo?) 2013-02-19 01:50:20 PST
Do we allow GETs to xmlrpc.cgi to affect anything of consequence?
Comment 20 Reed Loden [:reed] (use needinfo?) 2013-02-19 09:19:11 PST
(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] (justdave@bugzilla.org) 2013-07-11 18:23:44 PDT
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] 2013-07-14 20:56:24 PDT
Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/trunk
modified Bugzilla/WebService/Server/XMLRPC.pm
Committed revision 8647.

Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/4.4
modified Bugzilla/WebService/Server/XMLRPC.pm
Committed revision 8576.

Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/4.2
modified Bugzilla/WebService/Server/XMLRPC.pm
Committed revision 8218.

Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bugzilla/4.0
modified Bugzilla/WebService/Server/XMLRPC.pm
Committed revision 7752.

Note You need to log in before you can comment on or make changes to this bug.