Bug 15530 - XMLHttpRequest should not support certain methods
Summary: XMLHttpRequest should not support certain methods
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: XML (show other bugs)
Version: 523.x (Safari 3)
Hardware: All All
: P3 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-10-16 04:07 PDT by Alexey Proskuryakov
Modified: 2007-11-22 03:04 PST (History)
0 users

See Also:


Attachments
Patch + test case (5.30 KB, patch)
2007-11-21 03:41 PST, Julien Chaffraix
ap: review-
Details | Formatted Diff | Diff
Patch updated with Ap's comments (5.25 KB, patch)
2007-11-21 07:55 PST, Julien Chaffraix
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.