Issue metadata
Sign in to add a comment
|
Security: bypassing CORS by returning 308 for revalidating request for Resource previously without redirects from MemoryCache |
||||||||||||||||||||||
Issue descriptionVULNERABILITY DETAILS Resource::revalidationFailed() clears |m_redirectChain|. When revalidating request to [URL A] receives a redirect response (e.g. 308) with 'Location: [URL B]' and then receives 200 from [URL B], revalidationFailed() is called for the response from [URL B], and clears the redirect chain from [URL A] to [URL B]. This results in a Resource object with url() = [URL A], response().url() = [URL B], but redirectChain() is empty. This causes bypassing CORS check (because the checks at redirects are omitted). The use of <link rel=preload> enables bypassing CORS checks for all URLs, not only for cacheable URLs. Affected URL: All. Affected Resource type: Raw (probably all). VERSION 51.0.2704.63 (Official Build) (64-bit) on Ubuntu 51.0.2704.63 (Official Build) beta on Ubuntu 52.0.2743.0 (Official Build) canary on Windows 7 52.0.2743.6 (Official Build) dev-m on Windows 7 53.0.2744.0 (Developer Build) on Ubuntu REPRODUCTION CASE 1. Download and run exploit6b.py. 2. (optional) Clear browser data and restart Chrome for better reproducing ratio. 3. Access http://localhost:8020/. 4. Alert of "NG: CORS bypassed." if CORS was bypassed. 5. Open DevTools to see the contents of |URL| accessed from http://localhost:8020/.
,
May 26 2016
,
May 27 2016
Draft CL: https://codereview.chromium.org/2017883002/ The CL makes revalidation to fail at the first redirect response. This is consistent with normal revalidation failure cases: "if revalidating request receives other than 304, call revalidationFailed()". But not so sure this is correct. Please double check.
,
May 27 2016
I can consistently repro on Beta for any URL.
,
May 28 2016
,
May 31 2016
The CL in Comment #3 doesn't fix cases like: - First request to [URL A] -> returns 200. - Revalidating request to [URL A] -> returns 308 redirect to [URL B] -> [URL B] returns 304. [URL B] can return 304 because request headers like If-Modified-Since are retained after receiving redirects during revalidation. In this case, the Resource has non-empty redirectChain() and no body. I think this is not a regression: currently in this case the Resource has empty redirectChain() and no body. The CL in Comment #3 leaves this not fixed, because: - Fixing such cases would require larger changes (to make WebURLLoaderImpl to change the request headers etc.) - Probably the case doesn't result in security issues because the Resource will have empty body, i.e. nothing to be leaked (but please double check). - I don't have a good idea for a mergeable fix for this.
,
May 31 2016
I added a unit test to https://codereview.chromium.org/2017883002/. Does the test expose hints for exploit code?
,
May 31 2016
hiroshige: In general we don't worry about patches exposing hints about vulnerabilities, since anyone determined enough could figure them out once they land.
,
Jun 2 2016
>#6 > I think this is not a regression: currently in this case the Resource has empty redirectChain() and no body. Sorry for the slow response. Can you explain this a bit? I don't understand why the Resource has no body.
,
Jun 2 2016
> #9 Sorry, the current behavior at comment #6 was wrong. Correction: Current behavior: The Resource has - redirectChain() of size 1 ([URL A] -> [URL B]) (Unchanged by this CL) - The body that was returned by the 200 response from [URL A] (Cleared after this CL) - The response headers returned by the 200 response from [URL A] and updated by the 304 response from [URL B]. (The response headers returned by the 304 response from [URL B] is used as-is after this CL) after revalidation. Anyway it is not working correctly.
,
Jun 2 2016
What happens when both servers are innocent? First request: [URL C] -> 308 -> [URL D] -> 200 Revalidation: [URL C] -> 308 -> [URL D] -> 304 I guess that works with the current impl but doesn't work with you change.
,
Jun 2 2016
> #11 Such cases are Reloaded (not Revalidated) by https://codereview.chromium.org/2011283002 ( Issue 613971 ).
,
Jun 3 2016
Ah, I see, thank you.
,
Jun 3 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a9531777113bf6e6ea527c1ec34bbb65a128b560 commit a9531777113bf6e6ea527c1ec34bbb65a128b560 Author: hiroshige <hiroshige@chromium.org> Date: Fri Jun 03 07:08:55 2016 Make revalidation to fail when received a redirect response BUG= 614989 Review-Url: https://codereview.chromium.org/2017883002 Cr-Commit-Position: refs/heads/master@{#397650} [modify] https://crrev.com/a9531777113bf6e6ea527c1ec34bbb65a128b560/third_party/WebKit/Source/core/fetch/RawResourceTest.cpp [modify] https://crrev.com/a9531777113bf6e6ea527c1ec34bbb65a128b560/third_party/WebKit/Source/core/fetch/Resource.cpp
,
Jun 3 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d0753f72046d160bc34584a47eb3f3a3c56daff1 commit d0753f72046d160bc34584a47eb3f3a3c56daff1 Author: hiroshige <hiroshige@chromium.org> Date: Fri Jun 03 11:28:20 2016 [Loader] Add asserts about revalidation and redirects After https://codereview.chromium.org/2011283002/ and https://codereview.chromium.org/2017883002/, redirect chain must be empty when revalidation starts or during revalidation is ongoing. This CL adds asserts to verify that. BUG= 613971 , 614989 Review-Url: https://codereview.chromium.org/2020253002 Cr-Commit-Position: refs/heads/master@{#397683} [modify] https://crrev.com/d0753f72046d160bc34584a47eb3f3a3c56daff1/third_party/WebKit/Source/core/fetch/Resource.cpp
,
Jun 6 2016
#14 and #15 were landed on 53.0.2758.0. Requesting merging them to M-52 beta.
,
Jun 6 2016
Your change meets the bar and is auto-approved for M52 (branch: 2743)
,
Jun 6 2016
Also confirmed the test case (Comment #0) works correctly on 53.0.2759.0 canary on Windows 7.
,
Jun 6 2016
,
Jun 6 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bf790b4bd8a2cffb01176399a937a2f100ffadf1 commit bf790b4bd8a2cffb01176399a937a2f100ffadf1 Author: Hiroshige Hayashizaki <hiroshige@chromium.org> Date: Mon Jun 06 16:46:54 2016 Make revalidation to fail when received a redirect response BUG= 614989 Review-Url: https://codereview.chromium.org/2017883002 Cr-Commit-Position: refs/heads/master@{#397650} (cherry picked from commit a9531777113bf6e6ea527c1ec34bbb65a128b560) Review URL: https://codereview.chromium.org/2042843002 . Cr-Commit-Position: refs/branch-heads/2743@{#237} Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939} [modify] https://crrev.com/bf790b4bd8a2cffb01176399a937a2f100ffadf1/third_party/WebKit/Source/core/fetch/RawResourceTest.cpp [modify] https://crrev.com/bf790b4bd8a2cffb01176399a937a2f100ffadf1/third_party/WebKit/Source/core/fetch/Resource.cpp
,
Jun 6 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/774c808f29118e5ea8c9087a51367c53b522e181 commit 774c808f29118e5ea8c9087a51367c53b522e181 Author: Hiroshige Hayashizaki <hiroshige@chromium.org> Date: Mon Jun 06 17:59:33 2016 Revert "Make revalidation to fail when received a redirect response" This reverts commit bf790b4bd8a2cffb01176399a937a2f100ffadf1. Reason: Compile failure in RawResourceTest.cpp BUG= 614989 Review URL: https://codereview.chromium.org/2045623002 . Cr-Commit-Position: refs/branch-heads/2743@{#241} Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939} [modify] https://crrev.com/774c808f29118e5ea8c9087a51367c53b522e181/third_party/WebKit/Source/core/fetch/RawResourceTest.cpp [modify] https://crrev.com/774c808f29118e5ea8c9087a51367c53b522e181/third_party/WebKit/Source/core/fetch/Resource.cpp
,
Jun 7 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/66988329a1011173517bbbc9d01e1c8273eb2079 commit 66988329a1011173517bbbc9d01e1c8273eb2079 Author: Hiroshige Hayashizaki <hiroshige@chromium.org> Date: Tue Jun 07 08:56:13 2016 Reland to M-52: Make revalidation to fail when received a redirect response In addition to the original CL [1], this CL de-Oilpanize DummyClient because DummyClient is on-heap in M-53 due to [2] but not in M-52. [1] https://codereview.chromium.org/2017883002 [2] https://codereview.chromium.org/1998073002 BUG= 614989 Review-Url: https://codereview.chromium.org/2017883002 Cr-Commit-Position: refs/heads/master@{#397650} (cherry picked from commit a9531777113bf6e6ea527c1ec34bbb65a128b560) R=japhet@chromium.org, kinuko@chromium.org, yhirano@chromium.org Review URL: https://codereview.chromium.org/2041723004 . Cr-Commit-Position: refs/branch-heads/2743@{#259} Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939} [modify] https://crrev.com/66988329a1011173517bbbc9d01e1c8273eb2079/third_party/WebKit/Source/core/fetch/RawResourceTest.cpp [modify] https://crrev.com/66988329a1011173517bbbc9d01e1c8273eb2079/third_party/WebKit/Source/core/fetch/Resource.cpp
,
Jun 8 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/762426dfb504e5b631ee0b48ffe9e3ba7f7ba6e3 commit 762426dfb504e5b631ee0b48ffe9e3ba7f7ba6e3 Author: Hiroshige Hayashizaki <hiroshige@chromium.org> Date: Wed Jun 08 04:19:58 2016 [Loader] Add asserts about revalidation and redirects After https://codereview.chromium.org/2011283002/ and https://codereview.chromium.org/2017883002/, redirect chain must be empty when revalidation starts or during revalidation is ongoing. This CL adds asserts to verify that. BUG= 613971 , 614989 Review-Url: https://codereview.chromium.org/2020253002 Cr-Commit-Position: refs/heads/master@{#397683} (cherry picked from commit d0753f72046d160bc34584a47eb3f3a3c56daff1) Review URL: https://codereview.chromium.org/2046923002 . Cr-Commit-Position: refs/branch-heads/2743@{#275} Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939} [modify] https://crrev.com/762426dfb504e5b631ee0b48ffe9e3ba7f7ba6e3/third_party/WebKit/Source/core/fetch/Resource.cpp
,
Jun 13 2016
This missed last Wednesday's Beta. This would be ready for a Stable release after 20th June.
,
Jun 15 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/66988329a1011173517bbbc9d01e1c8273eb2079 commit 66988329a1011173517bbbc9d01e1c8273eb2079 Author: Hiroshige Hayashizaki <hiroshige@chromium.org> Date: Tue Jun 07 08:56:13 2016 Reland to M-52: Make revalidation to fail when received a redirect response In addition to the original CL [1], this CL de-Oilpanize DummyClient because DummyClient is on-heap in M-53 due to [2] but not in M-52. [1] https://codereview.chromium.org/2017883002 [2] https://codereview.chromium.org/1998073002 BUG= 614989 Review-Url: https://codereview.chromium.org/2017883002 Cr-Commit-Position: refs/heads/master@{#397650} (cherry picked from commit a9531777113bf6e6ea527c1ec34bbb65a128b560) R=japhet@chromium.org, kinuko@chromium.org, yhirano@chromium.org Review URL: https://codereview.chromium.org/2041723004 . Cr-Commit-Position: refs/branch-heads/2743@{#259} Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939} [modify] https://crrev.com/66988329a1011173517bbbc9d01e1c8273eb2079/third_party/WebKit/Source/core/fetch/RawResourceTest.cpp [modify] https://crrev.com/66988329a1011173517bbbc9d01e1c8273eb2079/third_party/WebKit/Source/core/fetch/Resource.cpp
,
Jun 15 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/762426dfb504e5b631ee0b48ffe9e3ba7f7ba6e3 commit 762426dfb504e5b631ee0b48ffe9e3ba7f7ba6e3 Author: Hiroshige Hayashizaki <hiroshige@chromium.org> Date: Wed Jun 08 04:19:58 2016 [Loader] Add asserts about revalidation and redirects After https://codereview.chromium.org/2011283002/ and https://codereview.chromium.org/2017883002/, redirect chain must be empty when revalidation starts or during revalidation is ongoing. This CL adds asserts to verify that. BUG= 613971 , 614989 Review-Url: https://codereview.chromium.org/2020253002 Cr-Commit-Position: refs/heads/master@{#397683} (cherry picked from commit d0753f72046d160bc34584a47eb3f3a3c56daff1) Review URL: https://codereview.chromium.org/2046923002 . Cr-Commit-Position: refs/branch-heads/2743@{#275} Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939} [modify] https://crrev.com/762426dfb504e5b631ee0b48ffe9e3ba7f7ba6e3/third_party/WebKit/Source/core/fetch/Resource.cpp
,
Jul 20 2016
,
Sep 12 2016
This bug has been closed for more than 14 weeks. Removing security view restrictions. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 1 2016
This bug has been closed for more than 14 weeks. Removing security view restrictions. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 2 2016
This bug has been closed for more than 14 weeks. Removing security view restrictions. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 2 2016
,
Nov 22 2016
If the Console tab of DevTools shows "LOADThis is dummy", it is NOT a security bug. If the Console tab of DevTools shows "LOAD" + real facebook contents, it IS a security bug. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by hirosh...@chromium.org
, May 26 2016