Bug 449698 – login manager shouldn't allow nulls in username or password. r=gavin
authorJustin Dolske <dolske@mozilla.com>
Mon, 11 Aug 2008 15:25:21 -0700
changeset 16565 cdeb3b9d9b0f10cb30f2121a74c525fe18ebb8a4
parent 16564 f5016884eec41d3372914e99f1ee6a113dd8c3c4
child 16566 4593610d9715b7de0aa9cb716dcd3159393ccd5e
push id1138
push userjdolske@mozilla.com
push dateMon, 11 Aug 2008 22:25:31 +0000
treeherdermozilla-central@cdeb3b9d9b0f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgavin
bugs449698
milestone1.9.1a2pre
Bug 449698 – login manager shouldn't allow nulls in username or password. r=gavin
toolkit/components/passwordmgr/src/storage-Legacy.js
toolkit/components/passwordmgr/test/unit/test_storage_legacy_3.js
--- a/toolkit/components/passwordmgr/src/storage-Legacy.js
+++ b/toolkit/components/passwordmgr/src/storage-Legacy.js
@@ -508,16 +508,24 @@ LoginManagerStorage_legacy.prototype = {
                                         l.passwordField.indexOf(c) != -1);
         }
 
         // Nulls are invalid, as they don't round-trip well.
         // Mostly not a formatting problem, although ".\0" can be quirky.
         if (badCharacterPresent(aLogin, "\0"))
             throw "login values can't contain nulls";
 
+        // In theory these nulls should just be rolled up into the encrypted
+        // values, but nsISecretDecoderRing doesn't use nsStrings, so the
+        // nulls cause truncation. Check for them here just to avoid
+        // unexpected round-trip surprises.
+        if (aLogin.username.indexOf("\0") != -1 ||
+            aLogin.password.indexOf("\0") != -1)
+            throw "login values can't contain nulls";
+
         // Newlines are invalid for any field stored as plaintext.
         if (badCharacterPresent(aLogin, "\r") ||
             badCharacterPresent(aLogin, "\n"))
             throw "login values can't contain newlines";
 
         // A line with just a "." can have special meaning.
         if (aLogin.usernameField == "." ||
             aLogin.formSubmitURL == ".")
--- a/toolkit/components/passwordmgr/test/unit/test_storage_legacy_3.js
+++ b/toolkit/components/passwordmgr/test/unit/test_storage_legacy_3.js
@@ -480,31 +480,35 @@ tryAddUser(storage, nullUser, /login val
 nullUser.usernameField = "usernull";
 
 // check passwordField
 nullUser.passwordField = "passnull\0X";
 tryAddUser(storage, nullUser, /login values can't contain nulls/);
 nullUser.passwordField = "passnull";
 
 
-// check username and password, which are OK with embedded nulls.
+// check username with null
 nullUser.username = "user\0name";
+tryAddUser(storage, nullUser, /login values can't contain nulls/);
+nullUser.username = "username";
+
+// check password with null
 nullUser.password = "pass\0word";
-tryAddUser(storage, nullUser, null);
+tryAddUser(storage, nullUser, /login values can't contain nulls/);
+nullUser.password = "password";
+
 
-LoginTest.checkStorageData(storage, [], [nullUser]);
+// Final sanity check, to make sure we didn't store anything unexpected.
+LoginTest.checkStorageData(storage, [], []);
 var numLines = LoginTest.countLinesInFile(OUTDIR, "output-394610-4.txt");
-do_check_eq(numLines, 10);
+do_check_eq(numLines, 2);
 
 testdesc = "[flush and reload for verification]"
 LoginTest.initStorage(storage, OUTDIR, "output-394610-4.txt");
-LoginTest.checkStorageData(storage, [], [nullUser]);
-
-nullUser.username = "username";
-nullUser.password = "password";
+LoginTest.checkStorageData(storage, [], []);
 
 
 
 /* ========== end ========== */
 } catch (e) {
     throw ("FAILED in test #" + testnum + " -- " + testdesc + ": " + e);
 }