Status Update
No update yet.
Comments
el...@chromium.org
Security shepherd: sending to the same assignee as
pf...@google.com
caseq@: As the author of this do you have thoughts on the proposed fix? Should Page.reload be exempt from being discarded? It's specifically the falling through that's causing the issue here.
pe...@google.com
ca...@google.com
My recollection is that these lines were there because something was breaking if we just discarded Page.reload in this case -- I would assume it would have to do with reloading a page with a ctrl+r pressed in the front-end after a page crash. But I tried the proposed fix, and (1)
So.. I guess you could try that fix and see if anything breaks. Or you could limit that to the case of when the page has navigated to a DOM UI origin. Also, I just noticed you
pf...@google.com
You are right, cannot reproduce the bug with the loaderId restriction anymore! ading2019@ can you confirm?
ad...@gmail.com
I'm still able to reproduce this bug with the loaderId changes. I tested on
pf...@google.com
I'm able to reproduce this as well. Pinning the reload to a loaderId isn't sufficient. So far I've not been able to repoduce it in a custom build in order to track down the casue. It's extremely timing sensitive. I'm inclined to go with un-specialcasing Page.reload. My best hypothesis currently is that the Page.reload arrives on the "old" document, schedules the injection, and then gets interrupted by the crash.
pf...@google.com
Assigning to caseq@ to make a call on the tentative fix CL.
ts...@google.com
ca...@google.com
As I stated above, I don't object against the suggested fix as long as it doesn't cause any regressions. While I've ran a CQ against it on a WIP CL, I hope you could follow through by accompanying the change with some test coverage.
As for the loaderId check -- Philip, I had another look at your CL and realized you only verify the loaderId on the browser side. Would it help against this attack vector if we were also to
ap...@google.com
Branch: main
commit 43b8b682d05c75caf0daf1643b84575009cc0052
Author: Philip Pfaffe <pfaffe@chromium.org>
Date: Tue Jun 18 10:04:45 2024
Prevent script injection on reload when racing with a navigation
DevTools passes the loaderId now when calling Page.reload, in order to
prevent accidentally reloading the wrong page when a navigation occurred
concurrently. It can still happen that the navigation kicks in in between the reload iniated in the browser and the script injection that happens in the renderer, which would run the injected script on the wrong target. We need to check the loaderId also on the renderer side.
Fixed: 341136300
Change-Id: I891fb37fa10e6789c8697a0f29bf7118788a9319
Reviewed-on:
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Commit-Queue: Philip Pfaffe <pfaffe@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1316330}
M third_party/blink/renderer/core/inspector/build.gni
M third_party/blink/renderer/core/inspector/inspector_page_agent.cc
M third_party/blink/renderer/core/inspector/inspector_page_agent.h
A third_party/blink/renderer/core/inspector/inspector_page_agent_unittest.cc
pe...@google.com
Requesting merge to stable (M126) because latest trunk commit (1316330) appears to be after stable branch point (1300313). Requesting merge to beta (M127) because latest trunk commit (1316330) appears to be after beta branch point (1313161). Thank you for fixing this security bug! We aim to ship security fixes as quickly as possible, to limit their opportunity for exploitation as an "n-day" (that is, a bug where git fixes are developed into attacks before those fixes reach users).
We have determined this fix is necessary on milestone(s): [].
Please answer the following questions so that we can safely process this merge request:
- Which CLs should be backmerged? (Please include Gerrit links.)
- Has this fix been verified on Canary to not pose any stability regressions?
- Does this fix pose any potential non-verifiable stability risks?
- Does this fix pose any known compatibility risks?
- Does it require manual verification by the test team? If so, please describe required testing.
pe...@google.com
Merge review required: M127 is already shipping to beta.
Please answer the following questions so that we can safely process your merge request:
- Why does your merge fit within the merge criteria for these milestones?
- Chrome Browser:
https://chromiumdash.appspot.com/branches - Chrome OS:
https://goto.google.com/cros-release-branch-merge-guidelines
- What changes specifically would you like to merge? Please link to Gerrit.
- Have the changes been released and tested on canary?
- Is this a new feature? If yes, is it behind a Finch flag and are experiments active in any release channels?
- [Chrome OS only]: Was the change reviewed and approved by the Eng Prod Representative?
https://goto.google.com/cros-engprodcomponents - If this merge addresses a major issue in the stable channel, does it require manual verification by the test team? If so, please describe required testing.
Please contact the milestone owner if you have questions. Owners: eakpobaro (Android), eakpobaro (iOS), alonbajayo (ChromeOS), danielyip (Desktop)
pe...@google.com
Merge review required: M126 is already shipping to stable.
Please answer the following questions so that we can safely process your merge request:
- Why does your merge fit within the merge criteria for these milestones?
- Chrome Browser:
https://chromiumdash.appspot.com/branches - Chrome OS:
https://goto.google.com/cros-release-branch-merge-guidelines
- What changes specifically would you like to merge? Please link to Gerrit.
- Have the changes been released and tested on canary?
- Is this a new feature? If yes, is it behind a Finch flag and are experiments active in any release channels?
- [Chrome OS only]: Was the change reviewed and approved by the Eng Prod Representative?
https://goto.google.com/cros-engprodcomponents - If this merge addresses a major issue in the stable channel, does it require manual verification by the test team? If so, please describe required testing.
Please contact the milestone owner if you have questions. Owners: eakpobaro (Android), eakpobaro (iOS), ceb (ChromeOS), srinivassista (Desktop)
am...@chromium.org
Given the regression concerns noted in #11 and upon reviewing the change, I would like this fix to get a bit more bake time on Canary before a potential backmerge. I'll revisit and re-review early next week.
am...@chromium.org
The M126 Stable update for this week was cut on Friday and released on Monday; we are going into a release freeze. I'll revisit this later in the week or early next week for potential M126 Stable merge for the update following release freeze.
ap...@google.com
Branch: refs/branch-heads/6533
commit b2b1d3f5f039c55e2da44353bae763cae796b6b7
Author: Philip Pfaffe <pfaffe@chromium.org>
Date: Wed Jun 26 09:20:25 2024
Prevent script injection on reload when racing with a navigation
DevTools passes the loaderId now when calling Page.reload, in order to
prevent accidentally reloading the wrong page when a navigation occurred
concurrently. It can still happen that the navigation kicks in in between the reload iniated in the browser and the script injection that happens in the renderer, which would run the injected script on the wrong target. We need to check the loaderId also on the renderer side.
(cherry picked from commit 43b8b682d05c75caf0daf1643b84575009cc0052)
Fixed: 341136300
Change-Id: I891fb37fa10e6789c8697a0f29bf7118788a9319
Reviewed-on:
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Commit-Queue: Philip Pfaffe <pfaffe@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1316330}
Reviewed-on:
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Danil Somsikov <dsv@chromium.org>
Reviewed-by: Danil Somsikov <dsv@chromium.org>
Auto-Submit: Philip Pfaffe <pfaffe@chromium.org>
Cr-Commit-Position: refs/branch-heads/6533@{#707}
Cr-Branched-From: 7e0b87ec6b8cb5cb2969e1479fc25776e582721d-refs/heads/main@{#1313161}
M third_party/blink/renderer/core/inspector/build.gni
M third_party/blink/renderer/core/inspector/inspector_page_agent.cc
M third_party/blink/renderer/core/inspector/inspector_page_agent.h
A third_party/blink/renderer/core/inspector/inspector_page_agent_unittest.cc
pe...@google.com
LTS Milestone M126
This issue has been flagged as a merge candidate for Chrome OS' LTS channel. If selected, our merge team will handle any additional merges. To help us determine if this issue requires a merge to LTS, please answer this short questionnaire:
- Was this issue a regression for the milestone it was found in?
- Is this issue related to a change or feature merged after the latest LTS Milestone?
am...@chromium.org
pe...@google.com
This issue requires additional review before it can be merged to the LTS channel. Please answer the following questions to help us evaluate this merge:
- Number of CLs needed for this fix and links to them.
- Level of complexity (High, Medium, Low - Explain)
- Has this been merged to a stable release? beta release?
- Overall Recommendation (Yes, No)
rz...@google.com
https://crrev.com/c/5672472 - Low, there were a few simple merge conflicts
- 127
- Yes
am...@chromium.org
M126 merge approved for
ap...@google.com
Branch: refs/branch-heads/6478
commit e4ecc269979f94db9a9e0eb73287ee84a57b9576
Author: Philip Pfaffe <pfaffe@chromium.org>
Date: Mon Jul 15 17:24:05 2024
Prevent script injection on reload when racing with a navigation
DevTools passes the loaderId now when calling Page.reload, in order to
prevent accidentally reloading the wrong page when a navigation occurred
concurrently. It can still happen that the navigation kicks in in between the reload iniated in the browser and the script injection that happens in the renderer, which would run the injected script on the wrong target. We need to check the loaderId also on the renderer side.
(cherry picked from commit 43b8b682d05c75caf0daf1643b84575009cc0052)
Fixed: 341136300
Change-Id: I891fb37fa10e6789c8697a0f29bf7118788a9319
Reviewed-on:
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Commit-Queue: Philip Pfaffe <pfaffe@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1316330}
Reviewed-on:
Auto-Submit: Daniel Yip <danielyip@google.com>
Owners-Override: Daniel Yip <danielyip@google.com>
Reviewed-by: Philip Pfaffe <pfaffe@chromium.org>
Cr-Commit-Position: refs/branch-heads/6478@{#1775}
Cr-Branched-From: e6143acc03189c5e52959545b110d6d17ecd5286-refs/heads/main@{#1300313}
M third_party/blink/renderer/core/inspector/build.gni
M third_party/blink/renderer/core/inspector/inspector_page_agent.cc
M third_party/blink/renderer/core/inspector/inspector_page_agent.h
A third_party/blink/renderer/core/inspector/inspector_page_agent_unittest.cc
am...@chromium.org
The Chrome VRP panel has decided to award crbug.com/338248595, the parent bug for this issue, a VRP reward of $20,000, because the reward has already been communicated and updated on
Congratulations Allen and nice work!
ap...@google.com
Branch: refs/branch-heads/6099
commit b3cb06fe890bb8d74d826e5c53031476d77c32c1
Author: Philip Pfaffe <pfaffe@chromium.org>
Date: Tue Jul 30 14:41:18 2024
[M120-LTS] Prevent script injection on reload when racing with a navigation
M120 merge issues:
third_party/blink/renderer/core/inspector/inspector_page_agent.cc:
- loader_id isn't a parameter of reload() in 120, kept only the
SetPending() relevant call.
- the script.empty() check on DidCreateMainWorldContext() doesn't
exist in 120. Kept the change as it is on the original fix, except
for the evaluate call that also doesn't exist in 120.
DevTools passes the loaderId now when calling Page.reload, in order to
prevent accidentally reloading the wrong page when a navigation occurred
concurrently. It can still happen that the navigation kicks in in between the reload iniated in the browser and the script injection that happens in the renderer, which would run the injected script on the wrong target. We need to check the loaderId also on the renderer side.
(cherry picked from commit 43b8b682d05c75caf0daf1643b84575009cc0052)
Fixed: 341136300
Change-Id: I891fb37fa10e6789c8697a0f29bf7118788a9319
Reviewed-on:
Commit-Queue: Philip Pfaffe <pfaffe@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1316330}
Reviewed-on:
Reviewed-by: Alex Rudenko <alexrudenko@chromium.org>
Reviewed-by: Victor Gabriel Savu <vsavu@google.com>
Commit-Queue: Roger Felipe Zanoni da Silva (xWF) <rzanoni@google.com>
Cr-Commit-Position: refs/branch-heads/6099@{#2050}
Cr-Branched-From: e6ee4500f7d6549a9ac1354f8d056da49ef406be-refs/heads/main@{#1217362}
M third_party/blink/renderer/core/inspector/build.gni
M third_party/blink/renderer/core/inspector/inspector_page_agent.cc
M third_party/blink/renderer/core/inspector/inspector_page_agent.h
A third_party/blink/renderer/core/inspector/inspector_page_agent_unittest.cc
pe...@google.com
This bug has been closed for more than 14 weeks. Removing issue access restrictions.
Description
As requested, I am submitted this as a separate issue. The original bug report (with the full chain leading to a sandbox escape) is located at
VULNERABILITY DETAILS
example.org, then it evaluateslocation.href = "about:blank"inside the page. This resets the page's origin.chrome.devtools.inspectedWindow.reloadand puts adebuggerstatement in theinjectedScriptargument.chrome.devtools.inspectedWindow.reloadagain with the JS payload in theinjectedScriptargument. This crashes the inspected page.EXPLANATION
An extension with the
devtools_pagepermission in its manifest.json file can use thechrome.devtools.inspectedWindow.reloadAPI, which reloads the inspected page and injects a content script. It is possible to use this API to run arbitrary JS on any page, even if it is supposed to be disallowed.Normally, pending devtools commands are discarded after a page crash. However,
Page.reloadmethod calls are whitelisted and will not be removedchrome.tabs.update), and our custom JS will run on the page we navigate to.A crash can be caused with the
debuggerstatement in the content script, or with[...new Array(2**31)]. Using thedebuggerstatement causes a crash since it putsnavigation_commit_state_into an unexpected stateRenderFrameImpl::SynchronouslyCommitAboutBlankForBug778318is run, which modifies_navigation_commit_stateto a different valueabout:blankURL for the crash to occur.Resetting the inspected page's origin must occur since we do not want to crash our own extension process. Note that using the
debuggerstatement isn't the only way to cause a crash here. You can also use[...new Array(2**31)], but this will take a a few seconds longer to run. As long as the page crashes while the reload command is still pending, the exploit will work.BISECT
The mechanism which queues up devtools messages after a crash was added 9 years ago, on
I confirmed that this is the commit which introduced this bug, since I was able to reproduce it on version 45.0.2423.0 (
PATCH
Removing the exception for
Page.reloadcommands inDevToolsSession::ClearPendingMessagesis enough to fix this bug. I have tested this to work by compiling and running Chromium with the patch applied. I've attached the diff for this asfix-devtools-reload-bug.patch.VERSION
Chrome Version: 124.0.6367.201 stable
Operating System: Debian 12
REPRODUCTION
A video is attached which demonstrates this process.
CREDIT INFORMATION
Reporter credit: Allen Ding