Closed
Bug 817264
Opened 12 years ago
Closed 11 years ago
systemXHR allows reading file: URLS
Categories
(Firefox OS Graveyard :: General, defect, P2)
Tracking
(blocking-basecamp:+, b2g18 fixed)
Tracking | Status | |
---|---|---|
b2g18 | --- | fixed |
People
(Reporter: amac, Assigned: dhylands)
References
Details
Attachments
(1 file)
884 bytes,
patch
|
Details | Diff | Splinter Review |
Currently when using XHR with mozSystem: true, you can access the filesystem through the file: protocol. See for example: https://github.com/AntonioMA/gaia/commit/daa0d954de01a619486699906e6c4e7d4794c538#L1R548 There I open the "/sdcard/certs" directory and it works although it supposedly shouldn't.
It appears that child processes have read access to the sdcard, that should not be the case.
Presumably that also means that they have write access.
blocking-basecamp: --- → ?
Assignee | ||
Comment 3•12 years ago
|
||
No. I filed bug 777046 a while ago (and in reflection, it's probably fine). The permissions on the sdcard are: ----rwxr-x system sdcard_rw (for files) d---rwxr-x system sdcard_rw (for directories) This says that users belonging to the sdcard_rw group are the only ones which have write access to the sdcard, and that everybody (except the system user) has read access.
Updated•12 years ago
|
blocking-basecamp: ? → +
Flags: needinfo?(overholt)
Great that there's no write access! We should not be accessing the sdcard from the child process at all right now though, other than through bugs like the one in comment 0. So it should be safe to limit read access to only the sdcard_rw group.
Summary: systemXHR allows accessing file: URLS → systemXHR allows reading file: URLS
Comment 5•12 years ago
|
||
Michael, is this appropriate to do? Triage drivers think this can only be handled with a kernel rebuild.
Assignee: nobody → mwu
Comment 6•12 years ago
|
||
Can we get a priority and target milestone for this>
Updated•12 years ago
|
Priority: -- → P1
Target Milestone: --- → B2G C3 (12dec-1jan)
Comment 7•12 years ago
|
||
Michael, can you provide any info here? Saying you're not the right owner is also fine :)
Flags: needinfo?(overholt) → needinfo?(mwu)
Comment 8•12 years ago
|
||
Sorry about the delay - I haven't had a chance to look at the issue till now. The permission in question is here: http://androidxref.com/4.0.4/xref/system/vold/Volume.cpp#424 So a change in vold would be necessary to change the permission. Vold is not in the kernel so no kernel repack is necessary.
Flags: needinfo?(mwu)
Comment 9•12 years ago
|
||
I agree that eliminating read access to sdcard_rw is a good thing. Ideally, we'd just chroot the whole thing and not even worry about this, but it looks like bug 776648 won't make v1. I don't think this counts as a P1 blocker, though.
Priority: P1 → P2
Comment 10•11 years ago
|
||
Thanks for the info, Michael. Re-nomming for discussing today.
blocking-basecamp: + → ?
Comment 11•11 years ago
|
||
We discussed this again during triage and decided it's still a blocker. Dave volunteered to take on the work.
Assignee: mwu → dhylands
blocking-basecamp: ? → +
Assignee | ||
Comment 12•11 years ago
|
||
I can see 3 ways of resolving this: 1 - change system/vold/Volume.cpp (only a single character change) 2 - Hack the mount command in bionic 3 - Do some LD_PRELOAD magic when loading vold to intercept the mount command What are people's preference for this?
Flags: needinfo?(mvines)
Comment 13•11 years ago
|
||
We should be able to put in a patch for this. Just put up a patch and let mvines know.
Comment 14•11 years ago
|
||
Yep, #1 sgtm. We can certainly patch this in our tree.
Flags: needinfo?(mvines)
Assignee | ||
Comment 15•11 years ago
|
||
This patch removes read & execute priviledge from "others". Note that this patch should definitely NOT be taken until such time as the blocking bug is resolved (Right now, deviceStorage can't see any files on the sdcard once this bug is applied)
Assignee | ||
Comment 16•11 years ago
|
||
And the patch is intended to be applied inside the B2G/system/vold directory (which is a git repository)
I hope that bug 824172 would resolve this additionally, but maybe not. Anyway, dep is fixed and merged. m1, good to go. (ni? to highlight in your email queue.)
Flags: needinfo?(mvines)
Updated•11 years ago
|
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
status-b2g18:
--- → fixed
Comment 19•11 years ago
|
||
(landing this now on CAF)
You need to log in
before you can comment on or make changes to this bug.
Description
•