Last Comment Bug 394610 - (CVE-2008-0417) Content can corrupt stored passwords by injecting line breaks
(CVE-2008-0417)
: Content can corrupt stored passwords by injecting line breaks
Status: VERIFIED FIXED
[sg:moderate?]
: dataloss, testcase, verified1.8.1.12
Product: Toolkit
Classification: Components
Component: Password Manager (show other bugs)
: Trunk
: All All
: P1 critical (vote)
: mozilla1.9beta3
Assigned To: Justin Dolske [:Dolske]
:
Mentors:
: 400600 (view as bug list)
Depends on: 449698
Blocks:
  Show dependency treegraph
 
Reported: 2007-09-01 19:07 PDT by Justin Dolske [:Dolske]
Modified: 2009-01-06 12:54 PST (History)
9 users (show)
mconnor: blocking1.9+
dveditz: blocking1.8.1.12+
dveditz: wanted1.8.1.x+
dolske: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test-case that adds arbitrary password manager contents (1000 bytes, text/html)
2007-09-06 12:53 PDT, Johnathan Nightingale [:johnath]
no flags Details
Patch, work in progress (12.39 KB, patch)
2007-09-11 22:22 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Patch for review, v.1 (21.46 KB, patch)
2007-09-16 18:15 PDT, Justin Dolske [:Dolske]
gavin.sharp: review+
Details | Diff | Splinter Review
Patch for checkin, v.2 (21.39 KB, patch)
2007-11-12 01:25 PST, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
Branch Patch for review, v.1 (10.45 KB, patch)
2007-11-29 16:34 PST, Justin Dolske [:Dolske]
gavin.sharp: review+
Details | Diff | Splinter Review
Manual testcase (2.58 KB, text/html)
2007-11-29 16:36 PST, Justin Dolske [:Dolske]
no flags Details
Patch for checkin, v.3 (22.38 KB, patch)
2007-11-29 17:43 PST, Justin Dolske [:Dolske]
gavin.sharp: review+
Details | Diff | Splinter Review
Branch Patch for review, v.2 (10.67 KB, patch)
2007-12-23 22:54 PST, Justin Dolske [:Dolske]
cbiesinger: review+
Details | Diff | Splinter Review
Branch Patch for checkin, v.3 (10.68 KB, patch)
2007-12-26 17:06 PST, Justin Dolske [:Dolske]
dveditz: approval1.8.1.12+
Details | Diff | Splinter Review
Trunk patch for checkin, v.4 (21.94 KB, patch)
2007-12-26 18:32 PST, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review
1.8.0 version of patch (9.38 KB, patch)
2008-03-18 04:26 PDT, Alexander Sack
no flags Details | Diff | Splinter Review
1.8 to 1.8.0 plaindiff (5.20 KB, patch)
2008-03-18 04:27 PDT, Alexander Sack
no flags Details | Diff | Splinter Review

Description Justin Dolske [:Dolske] 2007-09-01 19:07:03 PDT
This is a potential issue raised at last week's pwmgr security/design review.

The current storage file, signons2.txt, is a line-based text file with an expected structure. If newlines were to be injected into the file, following entries would be shifted by one or more lines from where expected.

If this is possible, I think the only serious impact is that saving a login at an evil site would corrupt the file, and make existing logins inaccessible. I've thought about how entries shifted up/down by one or more lines would be handled, and tricking the pwmgr into disclosing a username or password shouldn't be possible. It might be possible to inject an extra, valid (not-corrupt) login with this technique... The username and password would already be known to the malicious site, though, so I'm not sure if that's useful for anything. [The injected entry would have to a base64 encoded user/pass]

The following things might be avenues for funky input:
* HTML name/id attribute on the input fields for a username and password
* The HTTP realm for WWW-Authenticate (equivalents for other protocols?)
* Site address? We ignore the URL Path, but maybe "evil\n.site.com" would work? [Also http://us\ner:pa\nss@site.com/ ?]
* The form submit URL. Handled the same as the site address, but because it doesn't actually have to work (and comes more or less directly from content),more bizarre values might be possible]

The actual username and password values are encrypted and stored as base64 encoded values, so it's unlikely bizarre values will do anything.
Comment 1 Justin Dolske [:Dolske] 2007-09-05 22:17:00 PDT
Hmm, one special case: a username field name of "." may cause problems due to the file format for storing multiple logins for a site... The readFile() code assumes that "." marks the end of a series of logins at this point.
Comment 2 Johnathan Nightingale [:johnath] 2007-09-06 12:53:15 PDT
Created attachment 279958 [details]
Test-case that adds arbitrary password manager contents

This is pretty worrisome to me, and *feels* exploitable.  Obvious things like setting an input's name to a bunch of backspaces, in order to intrude into the previous entry, don't work (they just get serialized as 0x08 characters) but this testcase proves that, at very least, I can write arbitrary contents into the pwmgr file.

Writing isn't as bad as reading, but I think there might be more here with digging - we should consider scrubbing all the strings we serialize, I think.
Comment 3 Justin Dolske [:Dolske] 2007-09-11 22:22:59 PDT
Created attachment 280557 [details] [diff] [review]
Patch, work in progress

This should fix the problem, need to do a bit more poking to make sure extra parenthesis in the hostname/realm don't cause confusion.

Next up: branch patch. Ugh.
Comment 4 Johnathan Nightingale [:johnath] 2007-09-12 22:35:52 PDT
Comment on attachment 280557 [details] [diff] [review]
Patch, work in progress

>     setLoginSavingEnabled : function (hostname, enabled) {
>+        // File format prohibits a certain values...
>+        if (hostname == "." || hostname.indexOf("\n") != -1)
>+            throw "Invalid hostname";
>+
...
>+    _checkLoginValues : function (aLogin) {
>+        // Newlines are invalid for any field stored as plaintext.
>+        if ((aLogin.formSubmitURL &&
>+                    aLogin.formSubmitURL.indexOf("\n") != -1) ||
>+            (aLogin.httpRealm &&
>+                    aLogin.httpRealm.indexOf("\n")     != -1) ||
>+            aLogin.hostname.indexOf("\n")      != -1 ||
>+            aLogin.usernameField.indexOf("\n") != -1 ||
>+            aLogin.passwordField.indexOf("\n") != -1)
>+            throw "login values can't contain newlines";
>+        
>+        // A line with just a "." can have special meaning.
>+        if (aLogin.usernameField == "." ||
>+            aLogin.formSubmitURL == ".")
>+            throw "login values can't be periods";

We're doing blacklist filtering here instead of whitelist permitting, which is generally frowned upon.  Are we sure that there is no character other than "\n" that will cause us problems here?  A quick test on Mac seems to indicate that null characters 0x00 are okay (though they truncate the file when opened in Komodo edit, a sign that they're at least a little confusing) but it strikes me that there might be others too.

Is there a regex match we can build that captures all and only the legit characters?  Should we maybe just filter all character codes under 32, plus '.'?  And in the case of the |=='.'| lines, is it a problem to have a line in signons2 which starts with a period, but has other content?  Could I mess up the DB with an entry like ". " (a period character followed by a space), for example?

I know it's a work in progress, and that you're slogging through a branch patch right now, just want to make sure we don't get caught here.
Comment 5 Justin Dolske [:Dolske] 2007-09-14 15:48:51 PDT
(In reply to comment #4)

> We're doing blacklist filtering here instead of whitelist permitting

Well, yes and no. The issue here is about file corruption, and from that angle anything is ok long as it doesn't confuse the file parsing (thus the checking for \n and \.). I'd put code directly into writeFile(), but it's better to handle the errors before then.

> Are we sure that there is no character other than "\n"
> that will cause us problems here?

I'll add a null-character test just to be sure, but the cases raised so far are all I see that the code explicitly cares about.

> And in the case of the |=='.'| lines, is it a problem to have a line in
> signons2 which starts with a period, but has other content?

No. The code that makes use of '.' on a line always checks |line.value == "."|. [Probably another good case for a null-character test, although I'm starting to think that if there's a problem here they should just be banned from all DOM strings...]
Comment 6 Justin Dolske [:Dolske] 2007-09-16 18:15:09 PDT
Created attachment 281135 [details] [diff] [review]
Patch for review, v.1

I went ahead and prohibited nulls in the storage module, as there were some round-trip issues I ran into when testing. It's more of a correctness issue than security, but while we're here...
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2007-10-21 07:38:55 PDT
*** Bug 400600 has been marked as a duplicate of this bug. ***
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-11-06 17:00:25 PST
Comment on attachment 281135 [details] [diff] [review]
Patch for review, v.1

>Index: toolkit/components/passwordmgr/src/nsLoginManager.js

>     setLoginSavingEnabled : function (hostname, enabled) {
>+        const nullChar = String.fromCharCode(0);

Could just use "\0" (same with all other uses).

>Index: toolkit/components/passwordmgr/src/storage-Legacy.js

>+    _checkLoginValues : function (aLogin) {

>+        // Nulls are invalid, as they don't round-trip well.
>+        // Mostly not a formatting problem, although ".\0" can be quirky.

>+        // Newlines are invalid for any field stored as plaintext.

These just do the same check with a different string, could factor out into a function?
Comment 9 Justin Dolske [:Dolske] 2007-11-12 01:25:24 PST
Created attachment 288286 [details] [diff] [review]
Patch for checkin, v.2

Updated to trunk with review nits fixed.
Comment 10 Justin Dolske [:Dolske] 2007-11-29 16:22:22 PST
Ugh. While testing a branch fix, I found that my original diagnosis of the severity (in comment #0) was wrong. In some circumstances, an injected newline can result in a "valid" login for the attacking site containing a username/password from some other site. There are some catches and complications that would make this tricky in practice, but it could work.

Example:

The user saves a password on attacker.com, where the name of the username field is "fieldname_with\n_newline" (note embedded newline). The following file would be stored:

----------
#2d
.
http://attacker.com
fieldname_with
_newline
MDIEEPgAAAAAA2XGw==
*testp3B
MDIEEPgAAAAAAraLw==
http://cgi.attacker.com
.
https://www.google.com
Email
MDIEEPgAAAAAAUVtQ==
*Passwd
MDIEEPgAAAAAAbZLg==
https://cgi.google.com
.
https://bugzilla.mozilla.org
Bugzilla_login
MDIEEPgAAAAAnXyyQ==
*Bugzilla_password
MDIEEPgAAAAAEpI9g==
https://cgi.bugzilla.mozilla.org
.
----------

The next time the browser is launched, the password manager will attempt to parse the file. The embedded newline causes the first and second entries to be corrupted and effectively ignored. However the bugzilla entry lines up just right, such that a usable entry is built.

Here's how pwmgr parses the file:

 1 http://attacker.com          -- url
 2 fieldname_with               -- user field name
 3 _newline                     -- enc. username   [Note 1]
 4 MDIEEPgAAAAAA2XGw==          -- pw field name
 5 *testp3B                     -- enc. password
 6 MDIEEPgAAAAAAraLw==          -- form submit URL
 7 http://cgi.attacker.com      -- user field name [Note 2]
 8 .                            -- enc. username
 9 https://www.google.com       -- pw field name
10 Email                        -- enc. password
11 MDIEEPgAAAAAAUVtQ==          -- form submit URL
12 *Passwd                      -- user field name [Note 3]
13 MDIEEPgAAAAAAbZLg==          -- enc. username
14 https://cgi.google.com       -- pw field name
15 .                            -- enc. password
16 https://bugzilla.mozilla.org -- form submit URL
17 Bugzilla_login               -- user field name [Note 4]
18 MDIEEPgAAAAAnXyyQ==          -- enc. username
19 *Bugzilla_password           ...
20 MDIEEPgAAAAAEpI9g==
21 https://cgi.bugzilla.mozilla.org
22 .

Notes:

1. We're out of sync on line 3 because of the injected newline.
2. Since this line isn't a ".", pwmgr thinks there are multiple entries for the
   host on line 1.
3. Same as Note 2.
4. The input lines now match what pwmgr's parser state, but for the wrong host.

After reading this file, pwmgr has 3 broken login entries that effectively don't exist (the "encrypted" data isn't, and so it can never decrypt a user/pass), and 1 non-broken entry. The non-broken entry is for http://attacker.com, but with the bugzilla username/password. It also gets the field names and form submit url from the bugzilla entry, so this makes it a bit tricky for the attacker to actually get the username/password.

Anyway, the fix for this problem is still the same.
Comment 11 Justin Dolske [:Dolske] 2007-11-29 16:34:58 PST
Created attachment 290785 [details] [diff] [review]
Branch Patch for review, v.1
Comment 12 Justin Dolske [:Dolske] 2007-11-29 16:36:41 PST
Created attachment 290786 [details]
Manual testcase

Branch doesn't have the testing infrastructure of trunk, so here's a manual testcase.
Comment 13 Justin Dolske [:Dolske] 2007-11-29 17:43:19 PST
Created attachment 290803 [details] [diff] [review]
Patch for checkin, v.3

Updated trunk patch.

While working on the branch patch, I noticed that we need to check for \r in addition to \n. :-( This update adds the needed checks and tests for \r. It's a minor change, but I'm re-requesting review lest gavin want to shake out any more lurking fail.
Comment 14 Justin Dolske [:Dolske] 2007-12-12 14:35:38 PST
Waiting for review of the branch patch before landing on trunk (per IRC w/Gavin).
Comment 15 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-12-18 07:21:09 PST
Comment on attachment 290785 [details] [diff] [review]
Branch Patch for review, v.1

>Index: toolkit/components/passwordmgr/base/nsPasswordManager.cpp

>@@ -1018,16 +1033,21 @@ nsPasswordManager::Notify(nsIContent* aF

>               if (NS_SUCCEEDED(GetActionRealm(formElement, formActionOrigin)) &&
>                   !entry->actionOrigin.Equals(formActionOrigin)) {
>+                // Reject values that would cause problems when parsing the storage file
>+                nsresult rv = CheckLoginValues(EmptyCString(), EmptyString(),
>+                                               EmptyString(),  formActionOrigin);
>+                NS_ENSURE_SUCCESS(rv, NS_OK);

Hrm, do we really want to fail completely here, rather than just not add the actionOrigin if it's invalid? Couldn't the previous block have already modified entry->passValue, meaning the change will be flushed to disk the next time WritePasswords is called anyways?

>@@ -1096,19 +1116,32 @@ nsPasswordManager::Notify(nsIContent* aF

>+        // Reject values that would cause problems when parsing the storage file
>+        // We do this after prompting, lest any code run from prompt context.
>+        nsresult rv = CheckLoginValues(realm,
>+                                       entry->userField, entry->passField,
>+                                       entry->actionOrigin);
>+        NS_ENSURE_SUCCESS(rv, NS_OK);
>+
>         AddSignonData(realm, entry);
>         WritePasswords(mSignonFile);
>       } else if (selection == 2) {
>+        // Reject values that would cause problems when parsing the storage file
>+        // We do this after prompting, lest any code run from prompt context.
>+        nsresult rv = CheckLoginValues(realm,
>+                               EmptyString(), EmptyString(), EmptyCString());
>+        NS_ENSURE_SUCCESS(rv, NS_OK);
>+
>         AddReject(realm);

Not sure what you mean by "lest any code run from the prompt context" in these two comments. Could you clarify?

>@@ -1968,18 +2002,25 @@ nsPasswordManager::FillDocument(nsIDOMDo

>-        if ((e->passField).IsEmpty())
>-          passField->GetName(e->passField);
>+        if ((e->passField).IsEmpty()) {
>+          nsAutoString passName;
>+          passField->GetName(passName);
>+
>+          // Reject values that would cause problems when parsing the storage file
>+          if (NS_SUCCEEDED(CheckLoginValues(EmptyCString(), EmptyString(),
>+                                            passName, EmptyCString())))
>+            e->passField.Assign(passName);
>+        }

I guess this will behave the same way as a password input with no name should the name contain invalid characters?

>+nsPasswordManager::BadCharacterPresent(const nsAString &aString)

>+nsPasswordManager::CheckLoginValues(const nsACString &aHost,
>+                                    const nsAString  &aUserField,
>+                                    const nsAString  &aPassField,
>+                                    const nsACString &aActionOrigin)

Perhaps you could ask biesi to take a quick look at these and see if he can suggest any improvements? There maybe be a better way to implement these, and I'm not too familiar with our string classes.

r=me, but I'd like to take a look at the new patch.
Comment 16 Justin Dolske [:Dolske] 2007-12-23 22:49:39 PST
(In reply to comment #15)

> Hrm, do we really want to fail completely here, rather than just not add the
> actionOrigin if it's invalid?

Hrm, I suppose so. I was thinking that if shenanigans are going on, it's better to just completely bail out... But if there's already a login stored (we're changing the password), then the bad URL is most likely an innocent mistake anyway.

> Not sure what you mean by "lest any code run from the prompt context" in these
> two comments. Could you clarify?

The concern here was if there's a way for content to execute code while the prompt is up (setTimeout, or some such), then we want to ensure we're checking the actual values to be saved. Basically, a race condition. I don't think this can generally happen, but I thought there were some tricks to do stuff like this. Comment clarified.

> I guess this will behave the same way as a password input with no name should
> the name contain invalid characters?

Yes, I think so. This is the weird code that's trying to update existing logins while filling in the form (I guess for IE imported entries or something?), so if we don't update the stored field name we'll just end up here again next time.

> Perhaps you could ask biesi to take a quick look at these and see if he can
> suggest any improvements? There maybe be a better way to implement these, and
> I'm not too familiar with our string classes.

Sure. I think it's fairly simple as-is, and isn't in a perf-sensitive path, but it wouldn't hurt.
Comment 17 Justin Dolske [:Dolske] 2007-12-23 22:54:02 PST
Created attachment 294467 [details] [diff] [review]
Branch Patch for review, v.2

Updated patch.

Biesi, per comment 15 (at end), can you look at the string checking in the BadCharacterPresent() and CheckLoginValues() functions?
Comment 18 Christian :Biesinger (don't email me, ping me on IRC) 2007-12-26 12:48:24 PST
Comment on attachment 294467 [details] [diff] [review]
Branch Patch for review, v.2

+                if (NS_SUCCEEDED(CheckLoginValues(EmptyCString(), EmptyString(),
+                                               EmptyString(), formActionOrigin))) {

wrong indentation

+        nsresult rv = CheckLoginValues(realm,
+                               EmptyString(), EmptyString(), EmptyCString());

wrong indentation
Comment 19 Justin Dolske [:Dolske] 2007-12-26 17:06:39 PST
Created attachment 294615 [details] [diff] [review]
Branch Patch for checkin, v.3

Updated with biesi's nits.

Should I go ahead and land the branch and trunk patches? Or is it usual to wait until closer to the next planned branch release to land?
Comment 20 Justin Dolske [:Dolske] 2007-12-26 18:32:15 PST
Created attachment 294620 [details] [diff] [review]
Trunk patch for checkin, v.4

Updated trunk patch (had merge conflicts).
Comment 21 Justin Dolske [:Dolske] 2008-01-06 13:02:42 PST
Talked with dveditz, decided we should target the 13th to land this on trunk and branch. That's early enough to make the next trunk beta (B3) and branch point release freeze, with time for testing, yet late enough to avoid unduly exposing users should the Bad Guys notice the checkin and try to exploit it.
Comment 22 Daniel Veditz [:dveditz] 2008-01-07 15:14:08 PST
Comment on attachment 294615 [details] [diff] [review]
Branch Patch for checkin, v.3

approved for 1.8.1.12, a=dveditz for release-drivers

Please delay checkins until near the end of the FF2.0.0.12/FF3b3 dev cycle
Comment 23 Justin Dolske [:Dolske] 2008-01-13 22:29:29 PST
Checked in on trunk.

Checking in toolkit/components/passwordmgr/src/nsLoginManager.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/src/nsLoginManager.js,v  <--  nsLoginManager.js
new revision: 1.24; previous revision: 1.23
done
Checking in toolkit/components/passwordmgr/src/storage-Legacy.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/src/storage-Legacy.js,v  <--  storage-Legacy.js
new revision: 1.21; previous revision: 1.20
done
Checking in toolkit/components/passwordmgr/test/unit/head_storage_legacy_1.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/test/unit/head_storage_legacy_1.js,v  <--  head_storage_legacy_1.js
new revision: 1.6; previous revision: 1.5
done
Checking in toolkit/components/passwordmgr/test/unit/test_storage_legacy_3.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/test/unit/test_storage_legacy_3.js,v  <--  test_storage_legacy_3.js
new revision: 1.5; previous revision: 1.4
done
Comment 24 Justin Dolske [:Dolske] 2008-01-14 16:58:48 PST
I did a branch build, and verified that saving normal logins works, and that the bogus logins we're filtering are handled correctly (testcase in attachment 290786 [details], copied to http://people.mozilla.com/~dolske/pwtest.html).

Checked in on branch.

Checking in toolkit/components/passwordmgr/base/nsPasswordManager.cpp;
/cvsroot/mozilla/toolkit/components/passwordmgr/base/Attic/nsPasswordManager.cpp,v  <--  nsPasswordManager.cpp
new revision: 1.65.2.17; previous revision: 1.65.2.16
done
Checking in toolkit/components/passwordmgr/base/nsPasswordManager.h;
/cvsroot/mozilla/toolkit/components/passwordmgr/base/Attic/nsPasswordManager.h,v  <--  nsPasswordManager.h
new revision: 1.15.4.3; previous revision: 1.15.4.2
done
Comment 25 Al Billings [:abillings] 2008-01-18 13:10:12 PST
Trying to verify this for branch. The test case seems to require a debug build. Looking at it in a normal build with 2.0.0.11 and last night's nightly branch, the overt behavior is the same but then I don't have stderr output...

Can I get someone to verify that the fix is in the nightly branch builds and update this bug?
Comment 26 Justin Dolske [:Dolske] 2008-01-18 15:02:51 PST
The console output will only be in a debug build, but you should also be able to verify by telling it to remember the login and then checking to ensure that it didn't. [ie, if you tell FF to remember a login with these bad characters we just silently fail to do so.]
Comment 27 Al Billings [:abillings] 2008-01-18 15:12:26 PST
Ok, that is the difference in behavior that I witnessed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.12pre) Gecko/2008011803 BonEcho/2.0.0.12pre.

Test cases #3, #4, and #6 on the page don't prompt but your overall file indicates that these cases are broken, right?

If #1, #2, and #5 (and variants) not remembering is the patched behavior, then this is verified.
Comment 28 Justin Dolske [:Dolske] 2008-01-18 22:36:30 PST
Yes, that's right. Marking VERIFIED. Thanks!
Comment 29 Al Billings [:abillings] 2008-01-19 00:27:27 PST
I didn't verify it in trunk, just branch... which I am marking now. 
Comment 30 Alexander Sack 2008-03-18 04:26:44 PDT
Created attachment 310223 [details] [diff] [review]
1.8.0 version of patch

dolske, please review if the adaptions are appropriate for 1.8.0 branch. The main diff is that the

 | if ((e->passField).IsEmpty()) {| 

change is in OnStateChange for 1.8.0 versus FillDocument for 1.8.

Thanks!
Comment 31 Alexander Sack 2008-03-18 04:27:45 PDT
Created attachment 310224 [details] [diff] [review]
1.8 to 1.8.0 plaindiff
Comment 32 Justin Dolske [:Dolske] 2009-01-06 12:54:25 PST
Comment on attachment 310223 [details] [diff] [review]
1.8.0 version of patch

*sigh* Realistically, I'm never going to find the time to review this FF1.5-era patch.

I'm not very familiar with the old password manager code, and its terrible design makes it really hard to verify changes to it.

I suppose the patch looks sensible enough compared to the 1.8 patch, but checking to make sure it fixes all the cases for 1.8.0 would take some serious spelunking.

Note You need to log in before you can comment on or make changes to this bug.