Closed
Bug 650509
Opened 13 years ago
Closed 13 years ago
Other apps can read Firefox profile files
Categories
(Firefox for Android Graveyard :: General, defect, P1)
Tracking
(firefox5 fixed, status1.9.2 unaffected, status1.9.1 unaffected, fennec5+)
RESOLVED
FIXED
Firefox 5
Tracking | Status | |
---|---|---|
firefox5 | --- | fixed |
status1.9.2 | --- | unaffected |
status1.9.1 | --- | unaffected |
fennec | 5+ | --- |
People
(Reporter: kbrosnan, Assigned: blassey)
References
Details
(Whiteboard: [sg:moderate local])
Attachments
(4 files, 2 obsolete files)
569 bytes,
patch
|
robert.strong.bugs
:
review-
|
Details | Diff | Splinter Review |
688 bytes,
patch
|
mfinkle
:
review+
mfinkle
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
694 bytes,
patch
|
sdwilsh
:
review+
mfinkle
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
3.54 KB,
patch
|
sdwilsh
:
review+
mfinkle
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
We leave many of the profile files in Firefox Mobile open to be read by any app. There is a bit of a stir around Skype doing this. The good thing is that as long as Firefox is running we should have an exclusive lock on sqlite files. /data/data/org.mozilla.fennec/files/mozilla/pj09r2wd.default # ls -l -rw-rw-rw- app_89 app_89 169 2011-04-13 15:15 localstore.rdf -rw-r--r-- app_89 app_89 3344816 2011-04-16 09:15 places.sqlite-wal -rw-r--r-- app_89 app_89 98304 2011-04-13 17:42 chromeappsstore.sqlite -rw-r--r-- app_89 app_89 13796 2011-04-15 15:35 search.json -rw-r--r-- app_89 app_89 246901 2011-04-15 15:35 XUL.mfasl drwxr-xr-x app_89 app_89 2011-04-13 15:24 extensions -rw-r--r-- app_89 app_89 590288 2011-04-16 05:32 cookies.sqlite-wal -rw------- app_89 app_89 885 2011-04-13 15:18 pkcs11.txt -rw-r--r-- app_89 app_89 163840 2011-04-15 23:31 formhistory.sqlite -rw-rw-rw- app_89 app_89 251 2011-04-13 17:43 mimeTypes.rdf -rw-r--r-- app_89 app_89 163840 2011-04-14 20:08 webappsstore.sqlite -rw------- app_89 app_89 229376 2011-04-15 22:04 cert9.db -rw------- app_89 app_89 294912 2011-04-13 20:28 key4.db -rw------- app_89 app_89 11796 2011-04-15 17:01 sessionstore.bak -rw------- app_89 app_89 2900 2011-04-15 23:40 prefs.js -rw------- app_89 app_89 5967 2011-04-15 15:33 recommended-addons.json drwxrwxrwx app_89 app_89 2011-04-15 15:37 startupCache -rw-r--r-- app_89 app_89 154 2011-04-13 15:50 extensions.ini -rw-r--r-- app_89 app_89 3852 2011-04-16 00:26 autocomplete.json -rw-r--r-- app_89 app_89 131072 2011-04-16 00:26 cookies.sqlite -rw-r--r-- app_89 app_89 65536 2011-04-13 15:15 permissions.sqlite -rw-r--r-- app_89 app_89 3207 2011-04-15 15:38 blocklist.xml -rw-r--r-- app_89 app_89 65536 2011-04-13 15:15 search.sqlite -rw-r--r-- app_89 app_89 262144 2011-04-13 15:24 addons.sqlite -rw-r--r-- app_89 app_89 1212416 2011-04-16 08:21 places.sqlite lrwxrwxrwx app_89 app_89 2011-04-15 17:01 lock -> 127.0.0.1:+6575 -rw------- app_89 app_89 37162 2011-04-16 00:26 sessionstore.js -rw-r--r-- app_89 app_89 32768 2011-04-16 05:32 cookies.sqlite-shm -rw------- app_89 app_89 183 2011-04-15 15:35 compatibility.ini drwx------ app_89 app_89 2011-04-13 15:15 minidumps drwx------ app_89 app_89 2011-04-13 15:19 OfflineCache -rw-r--r-- app_89 app_89 66064 2011-04-13 15:50 extensions.sqlite-journal -rw-r--r-- app_89 app_89 32768 2011-04-16 09:15 places.sqlite-shm -rw-r--r-- app_89 app_89 393216 2011-04-13 15:50 extensions.sqlite -rw-r--r-- app_89 app_89 294912 2011-04-13 20:28 signons.sqlite
Reporter | ||
Updated•13 years ago
|
Severity: minor → critical
tracking-fennec: --- → ?
Priority: -- → P1
Assignee | ||
Updated•13 years ago
|
tracking-fennec: ? → 2.0-
Assignee | ||
Comment 1•13 years ago
|
||
Assignee: nobody → blassey.bugs
Attachment #530109 -
Flags: review?(robert.bugzilla)
Assignee | ||
Comment 2•13 years ago
|
||
Attachment #530111 -
Flags: review?(sdwilsh)
Assignee | ||
Comment 3•13 years ago
|
||
should this just use FileUtis.FILE_PERMS?
Attachment #530112 -
Flags: review?(mark.finkle)
Comment 4•13 years ago
|
||
Comment on attachment 530109 [details] [diff] [review] patch Brad, does this actually fix the issue? As I understand it, apps running in the user context would still be able to read the files. There are also places in out codebase that don't use FileUtils. minusing to get answers for the above.
Attachment #530109 -
Flags: review?(robert.bugzilla) → review-
Assignee | ||
Comment 5•13 years ago
|
||
(In reply to comment #4) > Comment on attachment 530109 [details] [diff] [review] [review] > patch > > Brad, does this actually fix the issue? As I understand it, apps running in the > user context would still be able to read the files. There are also places in > out codebase that don't use FileUtils. > > minusing to get answers for the above. heh.. we mid air'd, The question I was trying to pose to you was: This obviously makes this change for all platforms. Is that okay? It seems like a reasonable change to me, but I might be missing something.
Assignee | ||
Comment 6•13 years ago
|
||
with these three patches, my profile looks like this: rw------- app_32 app_32 4196 2011-05-04 13:59 sessionstore.js -rw------- app_32 app_32 163840 2011-05-04 13:59 formhistory.sqlite -rw------- app_32 app_32 845 2011-05-04 14:04 autocomplete.json -rw------- app_32 app_32 32768 2011-05-04 15:07 places.sqlite-shm -rw------- app_32 app_32 1082168 2011-05-04 15:07 places.sqlite-wal -rw------- app_32 app_32 1081344 2011-05-04 14:04 places.sqlite -rw-r--r-- app_32 app_32 216641 2011-05-04 13:59 XUL.mfasl -rw-rw-rw- app_32 app_32 169 2011-05-04 13:59 localstore.rdf -rw------- app_32 app_32 32 2011-05-04 13:59 extensions.ini -rw------- app_32 app_32 545 2011-05-04 13:59 prefs.js -rw------- app_32 app_32 512 2011-05-04 13:59 extensions.sqlite-journal -rw------- app_32 app_32 393216 2011-05-04 13:59 extensions.sqlite -rw------- app_32 app_32 65536 2011-05-04 13:59 permissions.sqlite drwxrwxrwx app_32 app_32 2011-05-04 14:00 startupCache -rw------- app_32 app_32 203 2011-05-04 13:59 compatibility.ini lrwxrwxrwx app_32 app_32 2011-05-04 13:59 lock -> 127.0.0.1:+1244 so XUL.mfasl and localstore.rdf are still world readable. I think that's okay though.
Assignee | ||
Comment 7•13 years ago
|
||
(In reply to comment #4) > Comment on attachment 530109 [details] [diff] [review] [review] > patch > > Brad, does this actually fix the issue? As I understand it, apps running in the > user context would still be able to read the files. There are also places in > out codebase that don't use FileUtils. > > minusing to get answers for the above. On android each app runs as its own user on the system. So only fennec can read fennec's files if they're permissions are set correctly.
Comment 8•13 years ago
|
||
Comment on attachment 530112 [details] [diff] [review] patch for autocomplete cache This is good for now. We can switch to FileUtils in a different bug, if appropriate.
Attachment #530112 -
Flags: review?(mark.finkle) → review+
Comment 9•13 years ago
|
||
Comment on attachment 530111 [details] [diff] [review] patch for default sqlite perms All of our other defines for SQLite go here: https://hg.mozilla.org/mozilla-central/annotate/7299264c5204/db/sqlite3/src/Makefile.in#l99 With a comment explaining why we have it. I also would like to see a test (either xpcshell or cpp) to make sure this does this since we are not using the default SQLite configuration here.
Attachment #530111 -
Flags: review?(sdwilsh) → review-
Assignee | ||
Comment 10•13 years ago
|
||
interestingly (and unfortunitely) these changes seem to have no effect on my atrix, which insists on making all the sqlite files in my profile world readable: drwxrwxrwx app_104 app_104 2011-05-10 15:47 startupCache -rw-r--r-- app_104 app_104 32768 2011-05-10 15:47 places.sqlite-shm -rw-r--r-- app_104 app_104 32 2011-05-10 15:47 extensions.ini lrwxrwxrwx app_104 app_104 2011-05-10 15:47 lock -> 127.0.0.1:+5067 -rw-rw-rw- app_104 app_104 169 2011-05-10 15:47 localstore.rdf -rw-r--r-- app_104 app_104 512 2011-05-10 15:47 extensions.sqlite-journal -rw------- app_104 app_104 203 2011-05-10 15:47 compatibility.ini -rw-r--r-- app_104 app_104 65536 2011-05-10 15:47 permissions.sqlite -rw------- app_104 app_104 4160 2011-05-10 15:47 sessionstore.js -rw-r--r-- app_104 app_104 845 2011-05-10 15:47 autocomplete.json -rw-r--r-- app_104 app_104 163840 2011-05-10 15:47 formhistory.sqlite -rw------- app_104 app_104 545 2011-05-10 15:47 prefs.js -rw-r--r-- app_104 app_104 1081344 2011-05-10 15:47 places.sqlite -rw-r--r-- app_104 app_104 3344816 2011-05-10 15:47 places.sqlite-wal -rw-r--r-- app_104 app_104 393216 2011-05-10 15:47 extensions.sqlite -rw-r--r-- app_104 app_104 212433 2011-05-10 15:47 XUL.mfasl
Comment 11•13 years ago
|
||
(In reply to comment #10) > interestingly (and unfortunitely) these changes seem to have no effect on my > atrix, which insists on making all the sqlite files in my profile world > readable: It's quite possible that this configuration is not tested by SQLite and could be broken. Is that the case here, or is it just that the Atrix is special?
Assignee | ||
Comment 12•13 years ago
|
||
looks like my tree needed a clobber, please disregard comment 10
Assignee | ||
Comment 13•13 years ago
|
||
this is android only, but it would seem reasonable to do for all platforms
Attachment #530111 -
Attachment is obsolete: true
Attachment #531474 -
Flags: review?(sdwilsh)
Comment 14•13 years ago
|
||
Comment on attachment 531474 [details] [diff] [review] patch for default sqlite perms This is fine in itself, but I also asked for a test in comment 9 because we actually have tests for all of our non-standard build options for SQLite. Probably easier to write a cpp test so you can ifdef it with the same ifdef in the Makefile.
Attachment #531474 -
Flags: review?(sdwilsh) → review-
Assignee | ||
Comment 15•13 years ago
|
||
(In reply to comment #14) > Comment on attachment 531474 [details] [diff] [review] [review] > patch for default sqlite perms > > This is fine in itself, but I also asked for a test in comment 9 because we > actually have tests for all of our non-standard build options for SQLite. > Probably easier to write a cpp test so you can ifdef it with the same ifdef > in the Makefile. Can you point me to one of your existing cpp tests? I don't see anything in db/sqlite3/src
Comment 16•13 years ago
|
||
(In reply to comment #15) > (In reply to comment #14) > Can you point me to one of your existing cpp tests? I don't see anything in > db/sqlite3/src Look at storage/test.
Assignee | ||
Comment 17•13 years ago
|
||
this test passes on linux desktop. I'd like to send it to try, but bug 656757 is preventing me from doing that
Attachment #532030 -
Flags: review?(sdwilsh)
Assignee | ||
Comment 18•13 years ago
|
||
updated to use nspr permissions constants
Attachment #532030 -
Attachment is obsolete: true
Attachment #532030 -
Flags: review?(sdwilsh)
Attachment #532229 -
Flags: review?(sdwilsh)
Assignee | ||
Updated•13 years ago
|
Attachment #531474 -
Flags: review- → review?
Assignee | ||
Updated•13 years ago
|
Attachment #530109 -
Flags: review- → review?(robert.bugzilla)
Comment 19•13 years ago
|
||
Comment on attachment 530109 [details] [diff] [review] patch I've had a bit of time to think about this and don't think this will fly as a global change. For example, an app dir installed extension's files needs to be readable by all users. There are other files as well such as the updates.xml and active-update.xml. To change this I suspect an audit of the call sites will be required and last I checked there is code besides sql that sets permissions on files directly.
Attachment #530109 -
Flags: review?(robert.bugzilla) → review-
Comment 20•13 years ago
|
||
Comment on attachment 532229 [details] [diff] [review] test patch Review of attachment 532229 [details] [diff] [review]: ----------------------------------------------------------------- r=sdwilsh with comments addressed. ::: storage/test/Makefile.in @@ +60,4 @@ > test_AsXXX_helpers.cpp \ > test_StatementCache.cpp \ > test_async_callbacks_with_spun_event_loops.cpp \ > + test_file_perms.cpp Missing trailing slash here. ::: storage/test/test_file_perms.cpp @@ +15,5 @@ > + * > + * The Original Code is storage test code. > + * > + * The Initial Developer of the Original Code is > + * Mozilla Corporation. the Mozilla Foundation @@ +40,5 @@ > +#include "storage_test_harness.h" > +#include "nsILocalFile.h" > + > +/** > + * This file test our statement scoper in mozStorageHelper.h. this is not correct
Attachment #532229 -
Flags: review?(sdwilsh) → review+
Comment 21•13 years ago
|
||
Comment on attachment 531474 [details] [diff] [review] patch for default sqlite perms r=sdwilsh
Attachment #531474 -
Flags: review? → review+
Assignee | ||
Comment 22•13 years ago
|
||
pushed: http://hg.mozilla.org/mozilla-central/rev/b8616e1cb51c http://hg.mozilla.org/mozilla-central/rev/6105fb36f613 http://hg.mozilla.org/mozilla-central/rev/e0ff8262c7b5
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Target Milestone: --- → Firefox 6
Comment 23•13 years ago
|
||
Is this security fix wanted in 5.0 or 4.0?
Assignee | ||
Comment 24•13 years ago
|
||
(In reply to comment #23) > Is this security fix wanted in 5.0 or 4.0? I don't think its severe enough to warrent taking on 4.0.x, but it seems reasonable to take for aurora
Assignee | ||
Updated•13 years ago
|
tracking-fennec: - → ?
Updated•13 years ago
|
tracking-firefox5:
--- → ?
tracking-firefox6:
--- → ?
Comment 25•13 years ago
|
||
Let this bake on trunk, but if we can land this for Fx5 we should. It seems like it will help our security story and it should not be risky.
tracking-fennec: ? → 5+
Updated•13 years ago
|
Whiteboard: [sg:moderate local]
Updated•13 years ago
|
status1.9.1:
--- → unaffected
status1.9.2:
--- → unaffected
Reporter | ||
Comment 27•13 years ago
|
||
Verified - Mozilla/5.0 (Android; Linux armv7l; rv:6.0a1) Gecko/20110516 Firefox/6.0a1 Fennec/6.0a1 ID:20110516042149 Tested * adding/removing bookmarks * Restarting Fennec * Sync * installing/uninstalling extensions * moving profile to sdcard and back * downloading a file * clearing private data
Status: RESOLVED → VERIFIED
Updated•13 years ago
|
Attachment #530112 -
Flags: approval-mozilla-aurora+
Updated•13 years ago
|
Attachment #531474 -
Flags: approval-mozilla-aurora+
Updated•13 years ago
|
Attachment #532229 -
Flags: approval-mozilla-aurora+
Comment 28•13 years ago
|
||
Testing seems solid and we get a slightly better security story on Android. Let's takes this for Fx5
Assignee | ||
Comment 29•13 years ago
|
||
pushed: http://hg.mozilla.org/releases/mozilla-aurora/rev/db9127a31e7c http://hg.mozilla.org/releases/mozilla-aurora/rev/ce21cb5d0b59 http://hg.mozilla.org/releases/mozilla-aurora/rev/ff8a010a1bfd (In reply to comment #26) > not taking this for 5 not the first time I've been wrong
tracking-fennec: 6+ → -
status1.9.1:
unaffected → ---
status1.9.2:
unaffected → ---
status-firefox5:
--- → fixed
tracking-firefox5:
? → ---
tracking-firefox6:
? → ---
Target Milestone: Firefox 6 → ---
Assignee | ||
Comment 30•13 years ago
|
||
not sure how I bashed all these flags...
tracking-fennec: - → 5+
status1.9.1:
--- → unaffected
status1.9.2:
--- → unaffected
Target Milestone: --- → Firefox 5
Updated•13 years ago
|
Group: core-security
Comment 31•13 years ago
|
||
Comment on attachment 532229 [details] [diff] [review] test patch >+ // This reflexts the permissions defined by SQLITE_DEFAULT_FILE_PERMISSIONS in >+ // db/sqlite3/src/Makefile.in and must be kept in sync with that >+#ifdef ANDROID >+ do_check_true(perms == PR_IRUSR | PR_IWUSR); >+#else >+ do_check_true(perms == PR_IRUSR | PR_IWUSR | PR_IRGRP | PR_IWGRP | PR_IROTH | PR_IWOTH); >+#endif Whilst fixing test_file_perms.cpp build warnings in bug 659234, it was noticed that the do_check_true lines above were incorrect. From bug 659234 comment 6... > I agree with you that that looks busted -- I seriously doubt that's the > intended behavior. > > blassey, mind taking a look at this? It looks to me like there's a bug in > the code and/or the test (in which case bug 650509 may not actually be > fixed, or at least not tested sufficiently...) See also bug 659234 comment 9 and bug 659234 comment 10.
Comment 32•13 years ago
|
||
At the same time as fixing the test, can the following build warning please be dealt with (if correcting the behaviour doesn't sort it already): { mozilla/storage/test/test_file_perms.cpp: In function ‘void test_file_perms()’: mozilla/storage/test/test_file_perms.cpp:64:44: warning: suggest parentheses around comparison in operand of ‘|’ }
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Comment 33•13 years ago
|
||
Can you file a new bug? Reopening FIXED bugs is basically never the right thing to do.
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•