Other apps can read Firefox profile files

RESOLVED FIXED in Firefox 5

Status

Fennec Graveyard
General
P1
critical
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: kbrosnan, Assigned: blassey)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 5
ARM
Android
Dependency tree / graph

Details

(Whiteboard: [sg:moderate local])

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

7 years ago
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

7 years ago
Severity: minor → critical
tracking-fennec: --- → ?
Priority: -- → P1
(Assignee)

Updated

7 years ago
tracking-fennec: ? → 2.0-
Created attachment 530109 [details] [diff] [review]
patch
Assignee: nobody → blassey.bugs
Attachment #530109 - Flags: review?(robert.bugzilla)
Created attachment 530111 [details] [diff] [review]
patch for default sqlite perms
Attachment #530111 - Flags: review?(sdwilsh)
Created attachment 530112 [details] [diff] [review]
patch for autocomplete cache

should this just use FileUtis.FILE_PERMS?
Attachment #530112 - Flags: review?(mark.finkle)
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-
(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.
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.
(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 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 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-
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
(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?
looks like my tree needed a clobber, please disregard comment 10
Created attachment 531474 [details] [diff] [review]
patch for default sqlite perms

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 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-
(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
(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.
Created attachment 532030 [details] [diff] [review]
test patch

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)
Created attachment 532229 [details] [diff] [review]
test patch

updated to use nspr permissions constants
Attachment #532030 - Attachment is obsolete: true
Attachment #532030 - Flags: review?(sdwilsh)
Attachment #532229 - Flags: review?(sdwilsh)
(Assignee)

Updated

7 years ago
Attachment #531474 - Flags: review- → review?
(Assignee)

Updated

7 years ago
Attachment #530109 - Flags: review- → review?(robert.bugzilla)
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 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 on attachment 531474 [details] [diff] [review]
patch for default sqlite perms

r=sdwilsh
Attachment #531474 - Flags: review? → review+

Updated

7 years ago
Target Milestone: --- → Firefox 6
Is this security fix wanted in 5.0 or 4.0?
(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

7 years ago
tracking-fennec: - → ?

Updated

7 years ago
tracking-firefox5: --- → ?
tracking-firefox6: --- → ?
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+
Whiteboard: [sg:moderate local]
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected
not taking this for 5
tracking-fennec: 5+ → 6+
(Reporter)

Comment 27

7 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
Attachment #530112 - Flags: approval-mozilla-aurora+
Attachment #531474 - Flags: approval-mozilla-aurora+
Attachment #532229 - Flags: approval-mozilla-aurora+
Testing seems solid and we get a slightly better security story on Android. Let's takes this for Fx5
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 → ---
not sure how I bashed all these flags...
tracking-fennec: - → 5+
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected
Target Milestone: --- → Firefox 5
Depends on: 659234
Group: core-security
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.
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 → ---
Can you file a new bug? Reopening FIXED bugs is basically never the right thing to do.
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Depends on: 672324

Updated

6 years ago
Depends on: 690693
You need to log in before you can comment on or make changes to this bug.