429781 - Update XMLHttpRequest blocked header list

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Adam Barth]

The XMLHttpRequest spec <http://www.w3.org/TR/XMLHttpRequest/> was recently updated to block more dangerous headers.  We should update our implementation to block them as well.

I've attached a first pass at implementing this, but I haven't had a chance to test the patch yet.  Is there an existing test suite for the blocked header list?

Comment 1

[Adam Barth]

Attachment: First pass at a patch

Oops.  Here is the patch.

Comment 2

[Daniel Veditz [:dveditz]]

One test is http://mxr.mozilla.org/mozilla/source/content/base/test/test_bug308484.html?force=1
any others ought to be in that general area.

Comment 3

[Adam Barth]

Attachment: Patch with tests

Thanks.  Here's an updated patch with tests.  I can't run the tests from this machine, but I'll try to do that in the next couple of days.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 316528 [details] [diff] [review]
Patch with tests

Looks good to me. Wish we had a better way of testing the head of a string, but I couldn't find anything that worked well here.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Also, it's your patch so you should have the honors of owning the bug :)

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 316528 [details] [diff] [review]
Patch with tests

Requesting approval. Technically this is something we could take in a dot-release, but the patch is super safe so I see no reason not to take it asap.

Comment 7

[Collin Jackson]

Minor issue with the tests: "aCCept-chaEset" should read "aCCept-chaRset"

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Hmm.. so how come the test is still passing? Or is it not?

Comment 9

[Adam Barth]

As mentioned in comment 0 and comment 3, I haven't had a chance to test the patch yet.  I'll test it today.

Comment 10

[Adam Barth]

Hum...  Turns out "Find" is not a member of |header| because nsACString in this file refers to nsTAString and not to the nsACString defined in nsStringAPI.h.  Jonas, how can we test if the header name starts with one of the banned prefixes?

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 316528 [details] [diff] [review]
Patch with tests

Renom once the comments about failing tests are addressed.

Comment 12

[Adam Barth]

Attachment: Fixed path

I got some help on the string issues from the IRC channel.  This patch builds and passes the supplied test.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 316904 [details] [diff] [review]
Fixed path

Excellent!

See comment 6 for why we should take this in RC1

Comment 14

[Damon Sicore (:damons)]

Comment on attachment 316904 [details] [diff] [review]
Fixed path

a1.9+=damons

Comment 15

[Adam Barth]

Is there anything more to do for this patch besides committing it?

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Nope, I can do that as soon as the tree goes green.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I checked this in. Leaving for Adam to mark fixed

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Backed out. Looked like this might have caused some orange. Or it was just a random failure :(

http://tinderbox.mozilla.org/showlog.cgi?tree=Firefox&errorparser=unittest&logfile=1209773100.1209777310.20904.gz&buildtime=1209773100&buildname=Linux%20qm-centos5-01%20dep%20unit%20test&fulltext=

Comment 19

[Adam Barth]

I'm not that familiar with tinderbox.  Can you help me understand which tests failed?  I search the output for "INFO FAIL" but I didn't find anything.  I see the line "TinderboxPrint: mochitest<br/>FAIL", but I don't understand what caused that.  Thanks.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So tinderbox went green before the backout took effect, so the problem was just temporary rather than because of this patch :(

I'll reland again tomorrow.

Comment 21

[Mike Schroepfer]

Jonas looks like we missed this - we'll have to take for 3.0.1

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 316904 [details] [diff] [review]
Fixed path

Reasking for approval. Patch is very safe so if we respin I think we should take this.

Comment 23

[Mike Schroepfer]

Comment on attachment 316904 [details] [diff] [review]
Fixed path

we are closed down for showstoppers. We can get this in 3.0.1 if needed.

Comment 24

[Adam Barth]

Jonas,

Is there a tree that's appropriate for this patch now?

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 316904 [details] [diff] [review]
Fixed path

Yes, this should land on trunk and on the 1.9.0 branch.

Comment 26

[Reed Loden [:reed]]

Landed in 15889:0f1b3785d279.

Comment 27

[Samuel Sidler (old account; do not CC)]

Comment on attachment 316904 [details] [diff] [review]
Fixed path

Approved for 1.9.0.2. Please land in CVS. a=ss

Comment 28

[Samuel Sidler (old account; do not CC)]

dveditz, can you land this?

Comment 29

[Adam Barth]

I got an email from Samuel Sidler asking me to land this patch, but I don't have commit privileges.  Jonas or dveditz, can you land this?  Thanks.

Comment 30

[Benjamin Smedberg]

This patch did not apply cleanly to CVS. Please provide an updated patch.

Comment 31

[Adam Barth]

I'm out of the country and won't be able to provide an updated patch until I return in a couple weeks.  The change is very simple.

I don't understand why the patch didn't apply cleanly as the last thing that happened to both of these files was that this patch was applied and then backed out due to flakiness in another test.  If you like, you can just revert the last changes to content/base/src/nsXMLHttpRequest.cpp and content/base/test/test_bug308484.html to "un-back out" the patch.

Comment 32

[Daniel Veditz [:dveditz]]

checked-in to 1.9 "branch" (cvs trunk)