650509 - Other apps can read Firefox profile files

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Kevin Brosnan [Ex-Mozilla]]

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

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch for default sqlite perms

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch for autocomplete cache

should this just use FileUtis.FILE_PERMS?

Comment 4

[Robert Strong (they/them - no direct email)]

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.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

(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.

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

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.

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

(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

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 9

[Shawn Wilsher :sdwilsh]

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.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

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

[Shawn Wilsher :sdwilsh]

(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?

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

looks like my tree needed a clobber, please disregard comment 10

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch for default sqlite perms

this is android only, but it would seem reasonable to do for all platforms

Comment 14

[Shawn Wilsher :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.

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

(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

[Shawn Wilsher :sdwilsh]

(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.

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: 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

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: test patch

updated to use nspr permissions constants

Comment 19

[Robert Strong (they/them - no direct email)]

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.

Comment 20

[Shawn Wilsher :sdwilsh]

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

Comment 21

[Shawn Wilsher :sdwilsh]

Comment on attachment 531474 [details] [diff] [review]
patch for default sqlite perms

r=sdwilsh

Comment 22

[Brad Lassey [:blassey] (use needinfo?)]

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

Comment 23

[Shawn Wilsher :sdwilsh]

Is this security fix wanted in 5.0 or 4.0?

Comment 24

[Brad Lassey [:blassey] (use needinfo?)]

(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

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 26

[Brad Lassey [:blassey] (use needinfo?)]

not taking this for 5

Comment 27

[Kevin Brosnan [Ex-Mozilla]]

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

Comment 28

[Mark Finkle (:mfinkle) (use needinfo?)]

Testing seems solid and we get a slightly better security story on Android. Let's takes this for Fx5

Comment 29

[Brad Lassey [:blassey] (use needinfo?)]

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

Comment 30

[Brad Lassey [:blassey] (use needinfo?)]

not sure how I bashed all these flags...

Comment 31

[Ed Morley [:emorley]]

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

[Ed Morley [:emorley]]

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 ‘|’
}

Comment 33

[:Gavin Sharp [email: [e-mail]]]

Can you file a new bug? Reopening FIXED bugs is basically never the right thing to do.