Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

[HackerOne] local files should not have read-access to other local files #4906

Closed
diracdeltas opened this issue Oct 18, 2016 · 12 comments · Fixed by brave/muon#80
Closed

[HackerOne] local files should not have read-access to other local files #4906

diracdeltas opened this issue Oct 18, 2016 · 12 comments · Fixed by brave/muon#80
Assignees
@diracdeltas
Labels
QA/checked-macOS QA/checked-Win32 QA/checked-Win64 security
Milestone
0.12.7dev

Comments

@diracdeltas
Copy link
Member

for instance, this is disallowed in Chrome unless http://peter.sh/experiments/chromium-command-line-switches/#allow-file-access-from-files is set

PoC:
0. echo 'hello world' > /tmp/test.txt

  1. save the following to a local html file:
<html>
<body>
<div id='div1'>
</div>
<script>
current_href = document.location.href
frame = document.createElement('iframe'); frame.src = 'file:///tmp/test.txt'; frame.id = 'frm'; document.getElementById('div1').appendChild(frame)
setTimeout(function func(){loot = document.getElementById('frm').contentWindow.document.getElementsByTagName('pre')[0].innerHTML
alert('Your data is: ' + loot)
}, 500)
</script>
</body>
</html>
  1. open the file in brave

the alert should not happen

cc @bridiver
@diracdeltas diracdeltas modified the milestones: 0.12.6dev, 0.12.7dev Oct 18, 2016
@willy-b
Copy link
Contributor

willy-b commented Oct 19, 2016 •
edited
Loading

Do we want to implement a preference setting / commandline flag or just turn it off for now?

I think that if we just wanted to disable it we could toggle it in brave/electron's atom/browser/atom_browser_client.cc:
https://github.com/brave/electron/blob/master/atom/browser/atom_browser_client.cc#L138

@bridiver
Copy link
Collaborator

they can default to false to match chrome. They are explicitly enabled in browser-laptop when required

@willy-b
Copy link
Contributor

willy-b commented Oct 19, 2016 •
edited
Loading

Thanks for the quick reply @bridiver.
What's your opinion then on whether it should be set to default to false in brave/electron as suggested above or in browser-laptop's js/stores/appStore.js:windowDefaults()?

browser-laptop/js/stores/appStore.js

Line 278 in 497ecf8

allowFileAccessFromFileUrls: true,

@bridiver
Copy link
Collaborator

that is a requirement to work at all and doesn't affect the tabs

@willy-b
Copy link
Contributor

willy-b commented Oct 19, 2016

Whoops. Yes, definitely don't want to change it there :-). Thanks!

@diracdeltas
Copy link
Member Author

@willy-b should i assign this one to you?

@willy-b
Copy link
Contributor

willy-b commented Oct 21, 2016

@diracdeltas, sure! I'll hop on it now

@willy-b
Copy link
Contributor

willy-b commented Oct 21, 2016

actually, no need to wait on me. we already discussed the one line change above and the latest brave/electron isn't building on my machine so it may be a few hours before I can test. If you don't get to it by the time I get brave/electron building again, I'll submit a PR, otherwise go for it.

@diracdeltas diracdeltas self-assigned this Oct 24, 2016
@diracdeltas
Copy link
Member Author

i tried toggling the flags in https://github.com/brave/electron/blob/master/atom/browser/atom_browser_client.cc#L138 but it didn't work

@diracdeltas diracdeltas mentioned this issue Oct 25, 2016
Merged
@diracdeltas
Copy link
Member Author

i think i figured it out in brave/muon#80. will add a test once that is merged.

diracdeltas added a commit to brave/muon that referenced this issue Oct 25, 2016
@diracdeltas
harden default web preferences
997c364 
and prevent local files loaded in webviews from reading other files unless
explicitly allowed by a webview attribute.
fix brave/browser-laptop#4906

auditors: @bridiver
diracdeltas added a commit that referenced this issue Oct 25, 2016
@diracdeltas
Add test for #4906
ed01bc5 
requires brave/muon#80

Auditors: @bridiver

Test Plan: n/a
This was referenced Oct 26, 2016
Closed
Closed
Closed
@luixxiul luixxiul mentioned this issue Oct 26, 2016
Closed
bbondy added a commit that referenced this issue Oct 26, 2016
@bbondy
Merge pull request #5133 from brave/tests/4906
ee5cb60 
Add test for #4906
This was referenced Oct 27, 2016
Closed
Closed
Closed
Closed
This was referenced Oct 28, 2016
Closed
Closed
Closed
Closed
@diracdeltas diracdeltas mentioned this issue Jul 3, 2018
Closed
@diracdeltas
Copy link
Member Author

this appears to have regressed

@diracdeltas
Copy link
Member Author

nvm, #14642 appears to be new, not a regression

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@diracdeltas diracdeltas

Labels
QA/checked-macOS QA/checked-Win32 QA/checked-Win64 security
Projects
None yet
Milestone
0.12.7dev
Development

Successfully merging a pull request may close this issue.

harden default web preferences brave/muon
6 participants
@bridiver @diracdeltas @alexwykoff @luixxiul @willy-b @srirambv