152697 - no limit on the size of a HTTP header

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect, P3)

Description

[Darin Fisher]

as we discussed during one of the recent security review meetings, the HTTP code
should limit the size of the HTTP headers it accepts.  not doing so could be
considered a DoS vulnerability or at the very least cause the browser to consume
large amounts of memory.

marking as security-sensitive for now.  maybe this one doesn't need to be private.

Comment 1

[Darin Fisher]

Attachment: v1 patch

simple patch to limit the mLineBuf used to build up header lines.  i chose a
rather arbitrary header limit of 10k.  i think that should be plenty big.

Comment 2

[Doug Turner (:dougt)]

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

add a commet so that we know what the reason is for 10k.

looks fine.

Comment 3

[Darin Fisher]

will do, thx.

Comment 4

[Darin Fisher]

not critical enough for 1.0.1, pushing out to 1.2alpha

Comment 5

[Daniel Veditz [:dveditz]]

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

>+    if (mLineBuf.Length() + len > MAX_LINEBUF_LENGTH) {
>+        NS_WARNING("excessively long header received, canceling transaction");
>+        return NS_ERROR_ABORT;
>+    }

Would it be possible to log this instead or in addition to the debug-only
NS_WARNING? If the case ever comes up it might help debug the server problem.

sr=dveditz either way

Comment 6

[Darin Fisher]

yup, i'll change that to a PR_LOG before checking in.  since HTTP logging is
compiled into nightly builds, we'll be able to easily check for this case using
regular nightly builds.

Comment 7

[Darin Fisher]

fixed-on-trunk

Comment 8

[Tom Everingham]

verified - win NT4, linux, mac osX

internal testcase attached, 15k Etag

from win NT http.log
185[7f1920]: nsHttpTransaction::ParseHead [count=3508]
185[7f1920]: excessively long header received, canceling transaction [trans=2b85a90]
185[7f1920]: nsHttpTransaction: listener returned [rv=80004004]

Comment 9

[Tom Everingham]

Darin, btw when stepping through the protos http header test suite (ie. the java
server which injects buffer overflow tests),  excessively large header tests are
skipped.  Is this because the http transaction is never completed?  I am seeing
in the log that the transaction and connection are destroyed in this case.

Comment 10

[Darin Fisher]

tever: yes, that would be this patch kicking in.  the request is canceled, and
therefore the underlying connection is also closed.

Comment 11

[Tom Everingham]

ok thanks, that's what i thought.