FileSystemAccess: Block sensitive paths in TWA launch [chromium/src : main]
Nate Chapin (Gerrit)
Nate Chapin added 1 comment![]( "Open in Gerrit")
bool IsSensitivePath(const base::FilePath& path) {Gemini decided what cases to filter here. It passes a sniff test, but I'm not confident I've thought of every case 😊
Related details
- Daniel Murphy
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Murphy (Gerrit)
Daniel Murphy added 1 comment![]( "Open in Gerrit")
bool IsSensitivePath(const base::FilePath& path) {Gemini decided what cases to filter here. It passes a sniff test, but I'm not confident I've thought of every case 😊
Looking at other bugs, I see:
https://source.chromium.org/chromium/chromium/src/+/main:android_webview/browser/file_system_access/aw_file_system_access_permission_context.cc;l=59;drc=6921036f402c9e2fecd9f01ea498bb836934a72a;bpv=1;bpt=1
maybe other at:
https://source.chromium.org/search?q=::ConfirmSensitiveEntryAccess%20&sq=&ss=chromium
that might help?
Related details
- Nate Chapin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Chapin (Gerrit)
Nate Chapin added 1 comment![]( "Open in Gerrit")
bool IsSensitivePath(const base::FilePath& path) {Daniel MurphyGemini decided what cases to filter here. It passes a sniff test, but I'm not confident I've thought of every case 😊
Looking at other bugs, I see:
https://source.chromium.org/chromium/chromium/src/+/main:android_webview/browser/file_system_access/aw_file_system_access_permission_context.cc;l=59;drc=6921036f402c9e2fecd9f01ea498bb836934a72a;bpv=1;bpt=1maybe other at:
https://source.chromium.org/search?q=::ConfirmSensitiveEntryAccess%20&sq=&ss=chromiumthat might help?
Ok, this mostly matches https://source.chromium.org/chromium/chromium/src/+/main:android_webview/browser/file_system_access/aw_file_system_access_permission_context.cc;l=59;drc=6921036f402c9e2fecd9f01ea498bb836934a72a now.
I don't see a good way to share the logic, given the layering.
Differences:
- We can't rely on content URIs already been filtered for us (that logic looks more like the general chrome logic in chrome/browser/file_system_access/chrome_file_system_access_permission_context.cc)
- Gemini thinks we should block `file://` urls because they're effectively absolute paths. I'm not certain whether we can get `file://` urls here.
Related details
- Daniel Murphy
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Murphy (Gerrit)
Daniel Murphy voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel Murphy
| Code-Review | +1 |
1 comment
bool IsSensitivePath(const base::FilePath& path) {Daniel MurphyGemini decided what cases to filter here. It passes a sniff test, but I'm not confident I've thought of every case 😊
Nate ChapinLooking at other bugs, I see:
https://source.chromium.org/chromium/chromium/src/+/main:android_webview/browser/file_system_access/aw_file_system_access_permission_context.cc;l=59;drc=6921036f402c9e2fecd9f01ea498bb836934a72a;bpv=1;bpt=1maybe other at:
https://source.chromium.org/search?q=::ConfirmSensitiveEntryAccess%20&sq=&ss=chromiumthat might help?
Ok, this mostly matches https://source.chromium.org/chromium/chromium/src/+/main:android_webview/browser/file_system_access/aw_file_system_access_permission_context.cc;l=59;drc=6921036f402c9e2fecd9f01ea498bb836934a72a now.
I don't see a good way to share the logic, given the layering.
Differences:
- We can't rely on content URIs already been filtered for us (that logic looks more like the general chrome logic in chrome/browser/file_system_access/chrome_file_system_access_permission_context.cc)
- Gemini thinks we should block `file://` urls because they're effectively absolute paths. I'm not certain whether we can get `file://` urls here.
I think we can install file:// urls on desktop on non-android, so we might? But happy to block here for now.
Related details
- Nate Chapin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Chapin (Gerrit)
Nate Chapin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nate Chapin
| Commit-Queue | +2 |
1 comment
bool IsSensitivePath(const base::FilePath& path) {Daniel MurphyGemini decided what cases to filter here. It passes a sniff test, but I'm not confident I've thought of every case 😊
Nate ChapinLooking at other bugs, I see:
https://source.chromium.org/chromium/chromium/src/+/main:android_webview/browser/file_system_access/aw_file_system_access_permission_context.cc;l=59;drc=6921036f402c9e2fecd9f01ea498bb836934a72a;bpv=1;bpt=1maybe other at:
https://source.chromium.org/search?q=::ConfirmSensitiveEntryAccess%20&sq=&ss=chromiumthat might help?
Daniel MurphyOk, this mostly matches https://source.chromium.org/chromium/chromium/src/+/main:android_webview/browser/file_system_access/aw_file_system_access_permission_context.cc;l=59;drc=6921036f402c9e2fecd9f01ea498bb836934a72a now.
I don't see a good way to share the logic, given the layering.
Differences:
- We can't rely on content URIs already been filtered for us (that logic looks more like the general chrome logic in chrome/browser/file_system_access/chrome_file_system_access_permission_context.cc)
- Gemini thinks we should block `file://` urls because they're effectively absolute paths. I'm not certain whether we can get `file://` urls here.
I think we can install file:// urls on desktop on non-android, so we might? But happy to block here for now.
Non-android, sure. But this android-specific code. I'll leave it for now, and can remove it in a followup if you think that makes sense.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Chapin (Gerrit)
Nate Chapin added 2 comments![]( "Open in Gerrit")
hartmanng: PTAL at `chrome/browser/android/webapps/twa_launch_queue_delegate.cc`
bool IsSensitivePath(const base::FilePath& path) {Daniel MurphyGemini decided what cases to filter here. It passes a sniff test, but I'm not confident I've thought of every case 😊
Nate ChapinLooking at other bugs, I see:
https://source.chromium.org/chromium/chromium/src/+/main:android_webview/browser/file_system_access/aw_file_system_access_permission_context.cc;l=59;drc=6921036f402c9e2fecd9f01ea498bb836934a72a;bpv=1;bpt=1maybe other at:
https://source.chromium.org/search?q=::ConfirmSensitiveEntryAccess%20&sq=&ss=chromiumthat might help?
Daniel MurphyOk, this mostly matches https://source.chromium.org/chromium/chromium/src/+/main:android_webview/browser/file_system_access/aw_file_system_access_permission_context.cc;l=59;drc=6921036f402c9e2fecd9f01ea498bb836934a72a now.
I don't see a good way to share the logic, given the layering.
Differences:
- We can't rely on content URIs already been filtered for us (that logic looks more like the general chrome logic in chrome/browser/file_system_access/chrome_file_system_access_permission_context.cc)
- Gemini thinks we should block `file://` urls because they're effectively absolute paths. I'm not certain whether we can get `file://` urls here.
Nate ChapinI think we can install file:// urls on desktop on non-android, so we might? But happy to block here for now.
Non-android, sure. But this android-specific code. I'll leave it for now, and can remove it in a followup if you think that makes sense.
hartmanng, do you have an opinion here on whether the file:// case needs to be considered?
Related details
- Daniel Murphy
- Glenn Hartmann
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Glenn Hartmann (Gerrit)
Glenn Hartmann voted and added 1 comment![]( "Open in Gerrit")
Votes added by Glenn Hartmann
| Code-Review | +1 |
1 comment
bool IsSensitivePath(const base::FilePath& path) {Daniel MurphyGemini decided what cases to filter here. It passes a sniff test, but I'm not confident I've thought of every case 😊
Nate ChapinLooking at other bugs, I see:
https://source.chromium.org/chromium/chromium/src/+/main:android_webview/browser/file_system_access/aw_file_system_access_permission_context.cc;l=59;drc=6921036f402c9e2fecd9f01ea498bb836934a72a;bpv=1;bpt=1maybe other at:
https://source.chromium.org/search?q=::ConfirmSensitiveEntryAccess%20&sq=&ss=chromiumthat might help?
Daniel MurphyOk, this mostly matches https://source.chromium.org/chromium/chromium/src/+/main:android_webview/browser/file_system_access/aw_file_system_access_permission_context.cc;l=59;drc=6921036f402c9e2fecd9f01ea498bb836934a72a now.
I don't see a good way to share the logic, given the layering.
Differences:
- We can't rely on content URIs already been filtered for us (that logic looks more like the general chrome logic in chrome/browser/file_system_access/chrome_file_system_access_permission_context.cc)
- Gemini thinks we should block `file://` urls because they're effectively absolute paths. I'm not certain whether we can get `file://` urls here.
Nate ChapinI think we can install file:// urls on desktop on non-android, so we might? But happy to block here for now.
Nate ChapinNon-android, sure. But this android-specific code. I'll leave it for now, and can remove it in a followup if you think that makes sense.
hartmanng, do you have an opinion here on whether the file:// case needs to be considered?
I've never heard of a file:// URL being used for a TWA. Doesn't mean it's never happened, but it should be extremely uncommon. I think it's ok to block it.
Related details
- Nate Chapin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Chapin (Gerrit)
Nate Chapin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nate Chapin
| Commit-Queue | +2 |
1 comment
bool IsSensitivePath(const base::FilePath& path) {Daniel MurphyGemini decided what cases to filter here. It passes a sniff test, but I'm not confident I've thought of every case 😊
Nate ChapinLooking at other bugs, I see:
https://source.chromium.org/chromium/chromium/src/+/main:android_webview/browser/file_system_access/aw_file_system_access_permission_context.cc;l=59;drc=6921036f402c9e2fecd9f01ea498bb836934a72a;bpv=1;bpt=1maybe other at:
https://source.chromium.org/search?q=::ConfirmSensitiveEntryAccess%20&sq=&ss=chromiumthat might help?
Daniel MurphyOk, this mostly matches https://source.chromium.org/chromium/chromium/src/+/main:android_webview/browser/file_system_access/aw_file_system_access_permission_context.cc;l=59;drc=6921036f402c9e2fecd9f01ea498bb836934a72a now.
I don't see a good way to share the logic, given the layering.
Differences:
- We can't rely on content URIs already been filtered for us (that logic looks more like the general chrome logic in chrome/browser/file_system_access/chrome_file_system_access_permission_context.cc)
- Gemini thinks we should block `file://` urls because they're effectively absolute paths. I'm not certain whether we can get `file://` urls here.
Nate ChapinI think we can install file:// urls on desktop on non-android, so we might? But happy to block here for now.
Nate ChapinNon-android, sure. But this android-specific code. I'll leave it for now, and can remove it in a followup if you think that makes sense.
Glenn Hartmannhartmanng, do you have an opinion here on whether the file:// case needs to be considered?
I've never heard of a file:// URL being used for a TWA. Doesn't mean it's never happened, but it should be extremely uncommon. I think it's ok to block it.
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
FileSystemAccess: Block sensitive paths in TWA launch
A malicious Android application could bypass the FileSystemAccess API
blocklist to obtain read and write access to arbitrary files within
Chrome's private data directory via TWA launch intents.
This CL implements path validation in TwaLaunchQueueDelegate to block
sensitive paths (parent references, Chrome's own Content URIs, app data
and cache directories, system directories).
It also modifies LaunchQueue to gracefully clear invalid paths instead
of crashing or bypassing the check.
TAG=agy
CONV=614f9db2-61b7-44b4-830b-5ebe9a33ae04
- M chrome/browser/android/webapps/twa_launch_queue_delegate.cc
- M components/webapps/browser/launch_queue/launch_queue.cc
- M components/webapps/browser/launch_queue/launch_queue_unittest.cc
Code-Review: +1 by Daniel Murphy, +1 by Glenn Hartmann
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |