400097 - Permission Manager loses data when TLDs added to IDN whitelist

Status: RESOLVED FIXED

(Core :: Networking, defect, P4)

Description

[Daniel Veditz [:dveditz]]

The Permission Manager stores and looks up values based on the host property of the nsIURI.host it is passed (as used by cookies, popup blocking, etc). For IDN domains the hostname may or may not be in punycode format depending on whether the domain's TLD is in the IDN whitelist.

Over time TLDs get added to the IDN whitelist (or perhaps removed: bug 395487) which means immediate loss of any cookie, popup blocking, image blocking, install-whitelisting, offline-app-enabling, and any other permission-manager-based per-site customizations for every IDN domain in that TLD.

Instead of GetHost() the Permission Manager should use GetAsciiHost() to ensure a consistent, normalized, hostname to use as a key. This may have an impact on various permission viewers which might have to convert strings back according to the IDN prefs, depends on whether they already use URIs or just strings.

Sharing such a hostperm.1 file between old and new versions of Firefox will also cause dataloss for IDN domains. We'll need to version the file and move forward with hostperm.2 after conversion. Converting to a new file would also help with possible dataloss if we fix bug 400092.

We also need to investigate what other storage we have that might suffer from the same problem. DOMStorage uses the Ascii-host already, but I think password manager suffers from this same problem.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So... this isn't a networking bug, right?  All the code discussed above lives outside of networking, I believe.

Comment 2

[Justin Dolske [:Dolske]]

Yeah, password manager has some issues like this... I went off on a tangent in 396316, and was looking at making the stored hostnames as consistent as possible. One thing I caught was that it can treat "http://bücher.dolske.org/" and "http://xn--bcher-kva.dolske.org/" as different hosts, depending on how it happens across the values.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Perhaps we should have an nsINetUtil method to normalize an ASCII representation of a host for display or something.

Comment 4

[Justin Dolske [:Dolske]]

There's nsIIDNService, which does that if you remember to call it where needed. :-) It might be nice if the browser itself handled some of this instead of every consumer (eg document.location, form.action), but I haven't really thought about the compatibility impact of that.

Comment 5

[[e-mail]]

(In reply to comment #3)
> Perhaps we should have an nsINetUtil method to normalize an ASCII
> representation of a host for display or something.

filed bug 402008 to do that.

-> me

Comment 6

[[e-mail]]

(In reply to comment #0)
> We'll need to version the file and move forward with hostperm.2 after
> conversion.

while we're here, i'd like to throw the idea of migrating to sqlite out there. the reasons we did it for cookies were

a) to move to a format with extensive testing and ACID capabilities (ok, so maybe we don't have all those letters, but hey ;) - yes, there are still bugs wrt multiple copies of cookies.txt files on different platforms and such!
b) to escape the versioning lock-in of a text file format: sqlite gives us some flexibility wrt adding columns to a db without revving the version, and provides schema versioning in case we do.
c) improved performance for the common use case of writing out a few new cookies to disk per pageload - no need to write out the entire file each time.
d) because it's all the rage these days.

there are perf downsides, so as with cookies we'd probably want to keep the in-memory hash.

i think for most people, their permissions files are pretty small, so c) doesn't really apply. the convenience and reliability factors, a) and b), certainly would.

Comment 7

[AndrewM]

Migrating to SQLite and renaming the file to something like permissions.sqlite would certainly prevent typos like this: http://quotes.burntelectrons.org/2063

Comment 8

[[e-mail]]

i have a patch in hand that collectively fixes this, and bug 400092, and migrates to sqlite. and does a bunch of other cleanups too, related to these things, which are somewhat unavoidable given how the changes affect the implementation. this means the patch isn't small :(

i'm going to test it, make sure everything's good, and then post it. the nice thing is, even after the sqlite migration, there's a codesize saving of 1.4kb, and plenty more lines removed than added...

Comment 9

[[e-mail]]

Attachment: patch v1

this patch does the following:

1) migrates from hostperm.1 to permissions.sqlite.
2) drops support for importing the ancient cookperm.txt format which was obsoleted pre-firefox 1.0. this reduces complexity (hey, that code was big) and simplifies things (hey, that code was ugly) and thus reduces possibilities for bugs incurred by me fiddling around with it to make it work.
3) fixes bug 400092 by dropping the 8/256 type/permission limits and going the whole hog to 32bit/32bit. they're now stored in an array, with each entry being a (type, permission) pair, which should work fine in the common case of 1 or perhaps a few permission entries per host. memory penalty, you say? well, for typical usage it'll cost less than 50%. for even enormous permission files with thousands of entries, that's in the tens of kilobytes. i used an nsAutoTArray with 1 "auto" part, since this will avoid any malloc overhead for the common case. (a note on type strings: they're stored in a linear array, so if you did actually have a large number of types then doing lookups on this list would be slow. for now, we're concerned only about breaking the 8 type barrier. in future, we may want to hash the type strings instead.)
4) fixes this bug (i was getting to it!) by moving to GetAsciiHost() and properly converting old UTF8 entries to ACE.
5) drops support for the "scheme:file" type of syntax, for blocking entire schemes when there is no host to be had. this is out of place!
6) removes that awful custom-rolled nsISimpleEnumerator implementation, which tried to build the list on the fly (while the consumer could be modifying it!), and generally gave me a headache. (don't bother reading it.) let nsCOMArray do the work, it's got its own beautiful enumerator implementation!

testing: i stress tested this by importing a hostperm.1 file with ~50k entries, doing various things to the list, writing it out again and making sure things add up. everything seems to work fine with regard to import, read, and database operations.

perf:
before patch, reading in the 50k entry list took 100ms, and writing it out (e.g. after adding an entry) took 30ms.
after patch, importing the list the first time took 600ms, subsequent reads took 80ms, and adding one entry took ~0.4ms.
so, once the initial import is done, the most common operations (reading in and modifying the list) are faster with sqlite.

unit tests: i'm working on them. i'll post some updated tests soon. ;)

Comment 10

[[e-mail]]

Comment on attachment 290521 [details] [diff] [review]
patch v1

sdwilsh, a little light reading for the plane ride? ;)

your eyes here would be much appreciated, hit me up on irc if you'd like more background.

Comment 11

[Shawn Wilsher :sdwilsh]

Comment on attachment 290521 [details] [diff] [review]
patch v1

> ////////////////////////////////////////////////////////////////////////////////
>+// nsPermissionManager Implementation
nit: two more //

>+static const char kPermissionsFileName[] = "permissions.sqlite";
why are you using a variable here?  As far as I know, all other consumers use a #define for the file name (and NS_LITERAL_CSTRING).

>-nsresult nsPermissionManager::Init()
>+nsresult
>+nsPermissionManager::Init()
:)


>+nsresult
>+nsPermissionManager::InitDB()
>+  permissionsFile->AppendNative(NS_LITERAL_CSTRING(kPermissionsFileName));
you should check the return variable on this

>+  nsCOMPtr<mozIStorageService> storage = do_GetService("@mozilla.org/storage/service;1");
MOZ_STORAGE_SERVICE_CONTRACTID please (mozStorageCID.h)

>+  // cache a connection to the cookie database
this isn't the cookie database :p

>+  nsresult rv = storage->OpenDatabase(permissionsFile, getter_AddRefs(mDBConn));
>+  if (rv == NS_ERROR_FILE_CORRUPTED) {
>+    // delete and try again
>+    permissionsFile->Remove(PR_FALSE);
this result should be checked

>+    rv = storage->OpenDatabase(permissionsFile, getter_AddRefs(mDBConn));
>+  }
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  PRBool tableExists = PR_FALSE;
>+  mDBConn->TableExists(NS_LITERAL_CSTRING("moz_hosts"), &tableExists);
nit: have the table in a #define please (even if you only use it once).

>+// sets the schema version and creates the moz_hosts table.
>+nsresult
>+nsPermissionManager::CreateTable()
>+{
>+  // set the schema version, before creating the table
>+  nsresult rv = mDBConn->SetSchemaVersion(HOSTS_SCHEMA_VERSION);
>+  if (NS_FAILED(rv)) return rv;
>+
>+  // create the table
>+  rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
>+    "CREATE TABLE moz_hosts ("
>+    "host TEXT, type TEXT, permission INTEGER)"));
nit: lets fix this before it becomes a problem down the line
"CREATE TABLE moz_hosts ("
  " host TEXT"
  ",type TEXT"
  ",permission INTEGER"
");"

>+  // create the hosts index
>+  return mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
>+    "CREATE INDEX moz_hosts_index "
>+    "ON moz_hosts (host)"));
Why do you have this?  Do we actually see a performance hit?

Lastly - why don't we have some id for each permission - indexing on an ID is oh so much faster than on text.

> NS_IMETHODIMP
> nsPermissionManager::Add(nsIURI     *aURI,
>                          const char *aType,
>                          PRUint32    aPermission)
> {
>   NS_ENSURE_ARG_POINTER(aURI);
>   NS_ENSURE_ARG_POINTER(aType);
> 
>   nsresult rv;
> 
>   nsCAutoString host;
>   rv = GetHost(aURI, host);
>-  // no host doesn't mean an error. just return the default
>-  if (NS_FAILED(rv)) return NS_OK;
>+  NS_ENSURE_SUCCESS(rv, rv);
I'm assuming that it is an error now?

>+    // all three statements we use (insert, delete, update) use the same
>+    // ordering of bound arguments.
please make sure you have a comment up by those statements about that

> NS_IMETHODIMP
> nsPermissionManager::RemoveAll()
>+    nsresult rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING("DELETE FROM moz_hosts"));
Why not TRUNCATE TABLE moz_hosts?

>+    if (!nsCRT::strcmp(someData, NS_LITERAL_STRING("shutdown-cleanse").get()) && mDBConn) {
>+      // clear the permissions file
>+      nsresult rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING("DELETE FROM moz_hosts"));
ditto

>+  PRBool hasResult;
>+  while (NS_SUCCEEDED(stmt->ExecuteStep(&hasResult)) && hasResult) {
>+    stmt->GetUTF8String(0, host);
>+    stmt->GetUTF8String(1, type);
You really *should* check this return types

I'd really like it if you (void)'d return values you don't care about, but I know you don't like doing that...

I'm only comfortable reviewing the storage bits of this.

Comment 12

[[e-mail]]

(In reply to comment #11)

thanks much! new patch forthcoming:

> >+static const char kPermissionsFileName[] = "permissions.sqlite";
> why are you using a variable here?  As far as I know, all other consumers use a
> #define for the file name (and NS_LITERAL_CSTRING).

for consistency with other uses in the file. i can switch them all to #defines if you'd like but it doesn't really make a difference ;)

> >+  permissionsFile->AppendNative(NS_LITERAL_CSTRING(kPermissionsFileName));
> you should check the return variable on this

done

> MOZ_STORAGE_SERVICE_CONTRACTID please (mozStorageCID.h)

done

> >+  // cache a connection to the cookie database
> this isn't the cookie database :p

right you are :)

> >+  if (rv == NS_ERROR_FILE_CORRUPTED) {
> >+    // delete and try again
> >+    permissionsFile->Remove(PR_FALSE);
> this result should be checked

done

> >+  mDBConn->TableExists(NS_LITERAL_CSTRING("moz_hosts"), &tableExists);
> nit: have the table in a #define please (even if you only use it once).

ok; did you want me to use that #define in place of "moz_hosts" all the other statement strings too? that'd hurt readability imo, but i can do it.

> nit: lets fix this before it becomes a problem down the line
> "CREATE TABLE moz_hosts ("
>   " host TEXT"
>   ",type TEXT"
>   ",permission INTEGER"
> ");"

good idea; done

> >+  // create the hosts index
> >+  return mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> >+    "CREATE INDEX moz_hosts_index "
> >+    "ON moz_hosts (host)"));
> Why do you have this?  Do we actually see a performance hit?

yes, for sure. looking up a string without an index involves traversing the whole table, but:

> Lastly - why don't we have some id for each permission - indexing on an ID is
> oh so much faster than on text.

i opted for the index instead of storing the rowid, because storing that 64bit int for each permission value has a memory cost. but, on further thought, so will that index, and i bet storing the int instead is cheaper (and 3x faster on lookup as well, see bug 230933 comment 29). sqlite will slurp ~400k of the table into memory (default on linux), so that index cost only applies up to that value, after which the int will be more expensive - but probably not "too expensive". so let's just keep track of rowid's instead (see new patch).

> >+  NS_ENSURE_SUCCESS(rv, rv);
> I'm assuming that it is an error now?

yes, i think we should propagate that error back.

> Why not TRUNCATE TABLE moz_hosts?

does sqlite support that? the docs don't say it does.

> >+    stmt->GetUTF8String(0, host);
> >+    stmt->GetUTF8String(1, type);
> You really *should* check this return types

done.

Comment 13

[[e-mail]]

forgot to cc you, plz see previous comment :)

Comment 14

[[e-mail]]

Attachment: patch v2

nits fixed, use rowids instead of indexing, also rev comments in nsIPermissionManager.idl to reflect new lack of limits.

Comment 15

[Shawn Wilsher :sdwilsh]

(In reply to comment #12)
> for consistency with other uses in the file. i can switch them all to #defines
> if you'd like but it doesn't really make a difference ;)
No, I suppose not - don't worry about it.

> ok; did you want me to use that #define in place of "moz_hosts" all the other
> statement strings too? that'd hurt readability imo, but i can do it.
On second thought - no.  This would hurt readability a bit...

> > Why not TRUNCATE TABLE moz_hosts?
> 
> does sqlite support that? the docs don't say it does.
Whoops.  Apparently it doesn't.  I'd like to see "DELETE * FROM moz_hosts" though.  I've never actually seen anyone write a delete statement without specifying columns (even if it doesn't matter, this is more of a readability nit).

Comment 16

[[e-mail]]

(In reply to comment #15)
> Whoops.  Apparently it doesn't.  I'd like to see "DELETE * FROM moz_hosts"
> though.  I've never actually seen anyone write a delete statement without
> specifying columns (even if it doesn't matter, this is more of a readability
> nit).

right, it works either way - i've made the change locally.

Comment 17

[[e-mail]]

Attachment: permmgr unit tests

wip tests for the whole permmgr API; more to come (e.g. import)

Comment 18

[Mike Shaver (:shaver emeritus)]

I just did a review of this with dwitte, about which he'll post a new patch, but I had to say: "DELETE FROM table" is the correct form -- you're not deleting columns, you're deleting rows, and in the shocking event that SQL permits you to specify columns there, you still shouldn't.

That is all.

Comment 19

[[e-mail]]

Attachment: patch v3

fixed per shaver's comments. no major changes here, just some refactoring to make things a bit more readable. not really sure about the enum names, let me know if you have better ideas ;)

patch passes unit tests as before.

Comment 20

[Mike Shaver (:shaver emeritus)]

Comment on attachment 291571 [details] [diff] [review]
patch v3

I'd prefer for RemoveAllInternal to take the notify and db flags, like AddInternal does, so that we don't have to remember to update the observer path when we add something after RemoveAllFromMemory, but that's not a blocker here.  Get it in the follow-up! Promise me!

sr=shaver

Comment 21

[[e-mail]]

(In reply to comment #20)
>  Get it in the follow-up! Promise me!

will do!

Comment 22

[[e-mail]]

Comment on attachment 291571 [details] [diff] [review]
patch v3

i've landed this for M10, given sr=shaver and a pending r=sdwilsh. i think this is the right thing to do, given that this patch fixes two blocking1.9+ bugs, and is large enough that we definitely want as much bake time as possible before release - which means getting it in for M10. i'll revise my checkin with sdwilsh's review comments during the bakeout period if possible. leaving this bug open to do that.

Comment 23

[[e-mail]]

checked in mozilla/netwerk/test/unit/test_permmgr.js revision 1.1.

Comment 24

[[e-mail]]

As per cookies I guess I get to file three bugs; one on exposing Import and one each on suite and browser to only attempt to import hostperm and not cookperm.

As for shutdown-cleanse, is it not possible to remove the file?
(Closing all statements and connections first, of course.)

Comment 25

[[e-mail]]

(In reply to comment #24)
> As per cookies I guess I get to file three bugs; one on exposing Import and one
> each on suite and browser to only attempt to import hostperm and not cookperm.

yep, that would be much appreciated!

> As for shutdown-cleanse, is it not possible to remove the file?
> (Closing all statements and connections first, of course.)

hmm, i suppose it is - might give the user a better sense of security, too, since the sqlite file size won't shrink on delete (it just overwrites the deleted data with zeros). want to file a followup on that too, and cite cookie code as well? (possibly dlmgr too, not sure?)

Comment 26

[Shawn Wilsher :sdwilsh]

Comment on attachment 291571 [details] [diff] [review]
patch v3

r=sdwilsh

Comment 27

[[e-mail]]

fixed! there'll be some more followup bugs here...

Comment 28

[Martijn Wargers (dead)]

Attachment: this would add it to the xpcshell test