Bug 15530

Summary: XMLHttpRequest should not support certain methods
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: XMLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P3    
Version: 523.x (Safari 3)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch + test case
ap: review-
Patch updated with Ap's comments ap: review+

Description Alexey Proskuryakov 2007-10-16 04:07:00 PDT
These methods make various XSS attacks possible.

   TRACE   http://www.kb.cert.org/vuls/id/867593
   TRACK   http://www.kb.cert.org/vuls/id/288308
   CONNECT http://www.kb.cert.org/vuls/id/150227

AFAIK the network layer blocks them on the Mac anyway, but I think we should explicitly check for these in XMLHttpRequest implementation itself.
Comment 1 Julien Chaffraix 2007-11-21 03:41:02 PST
Created attachment 17429 [details]
Patch + test case

Other browsers' behaviour :

- IE7 does not support these methods so raise an exception (message: invalid argument)

- Firefox raises a NS_ERROR_INVALID_ARG exception for track and trace (no exception for connect)

- Opera does not raise any exception


The patch makes us raise a SECURITY_ERR for the 3 methods (IE's behaviour and close to Firefox's).
Comment 2 Alexey Proskuryakov 2007-11-21 05:48:12 PST
Comment on attachment 17429 [details]
Patch + test case

+    layoutTestController.waitUntilDone();

This is not needed.

+    try {
+        xhr.open(method, "resources/1251.html", true);
+        xhr.send(null);

This test doesn't differentiate between open() and send() raising an exception, I think it should.

It may be slightly better to use sync XHR in the test, to maqke it more explicit that we don't wait for anything to finish.
Comment 3 Julien Chaffraix 2007-11-21 07:55:41 PST
Created attachment 17430 [details]
Patch updated with Ap's comments

> (From update of attachment 17429 [details] [edit])
> +    layoutTestController.waitUntilDone();

> This is not needed.

Removed.

> +    try {
> +        xhr.open(method, "resources/1251.html", true);
> +        xhr.send(null);

> This test doesn't differentiate between open() and send() raising an
> exception,
> I think it should.

In fact, the exception should be raised only for open() so I removed send() as it was not relevant.

> It may be slightly better to use sync XHR in the test, to maqke it more
> explicit that we don't wait for anything to finish.

The new test case uses a sync XHR.
Comment 4 Alexey Proskuryakov 2007-11-21 09:01:46 PST
Comment on attachment 17430 [details]
Patch updated with Ap's comments

r=me
Comment 5 Alexey Proskuryakov 2007-11-22 03:04:00 PST
Committed revision 27970.