394610 - (CVE-2008-0417) Content can corrupt stored passwords by injecting line breaks

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Justin Dolske [:Dolske]]

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\[e-mail]/ ?]
* 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]]

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]]

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

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

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]]

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

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

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

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]]

Attachment: Patch for checkin, v.2

Updated to trunk with review nits fixed.

Comment 10

[Justin Dolske [:Dolske]]

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]]

Attachment: Branch Patch for review, v.1

Comment 12

[Justin Dolske [:Dolske]]

Attachment: Manual testcase

Branch doesn't have the testing infrastructure of trunk, so here's a manual testcase.

Comment 13

[Justin Dolske [:Dolske]]

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

Waiting for review of the branch patch before landing on trunk (per IRC w/Gavin).

Comment 15

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

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]]

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

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

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]]

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

Attachment: Trunk patch for checkin, v.4

Updated trunk patch (had merge conflicts).

Comment 21

[Justin Dolske [:Dolske]]

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]]

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]]

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]]

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 - ex-MoCo]]

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]]

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 - ex-MoCo]]

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]]

Yes, that's right. Marking VERIFIED. Thanks!

Comment 29

[Al Billings [:abillings - ex-MoCo]]

I didn't verify it in trunk, just branch... which I am marking now. 

Comment 30

[Alexander Sack]

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

Attachment: 1.8 to 1.8.0 plaindiff

Comment 32

[Justin Dolske [:Dolske]]

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.