396316 - Logins for non-HTTP[S] protocols can be filled into content

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Justin Dolske [:Dolske]]

For HTTP[S] protocols, logins are stored with the associated HTTP Realm. This is done to separate protocol-based authentication from content-based authentication (forms).

[For example, consider http://example.com/form.html. The authentication used to access the document (via a WWW-Authenticate header) should never be filled into a login form on the resulting document, and vice versa. If it was, an attacker able to upload arbitrary content to the server could steal the login info when it's filled into the form.]

Bug 396295 reports that non-HTTP[S] logins are currently not working on trunk. I checked how branch was working, and found that it saves these logins without any associated "realm" data, just like a form login. This will cause the password manager to fill in data on content protected by a protocol login.

[For example, consider ftp://example.org/form.html. The FTP login used to access the document would be filled into a login form in form.html.]

On 2.0.0.6, accessing ftp://ftp.dolske.net/ and saving a dummy login looks like this in signons2.txt:

----------
ftp://ftp.dolske.net

MDoEEPgAAAAAAAAAAAAAAAAAAAEwFAYIhgfjhgfjhfjgsdfsdffj
*
MDoEEPgAAAAAAAAAAAAAAAAAAAEwFAYIKojghfkjhgkjhgkjgkjgh

.
----------

To be secure, this should look like:

----------
ftp.dolske.net:21 (ftp.dolske.net)

MDoEEPgAAAAAAAAAAAAAAAAAAAEwFAYIhgfjhgfjhfjgsdfsdffj
*
MDoEEPgAAAAAAAAAAAAAAAAAAAEwFAYIKojghfkjhgkjhgkjgkjgh

.
----------

[Note that bug 379111 covers the issue of storing the scheme with the login, so that the first line would then be "ftp://ftp.dolske.net (ftp.dolske.net)". The need for a "(realm)" appended to the line is more important then.]

This is a bit of a backwards-compatibility headache, in that existing stored logins would need to be converted to the new format to keep working... There's a risk we could mistakenly convert a content-login to a protocol-login, because they're stored the same way. But I suspect that case isn't terribly common, and that it's better to err on the side of the protocol than the content. [Although content logins saved since FF 2.0.0.2, when bug 360493 was fixed, will contain a non-blank formSubmitURL field. That could be checked when migrating.]

Comment 1

[Justin Dolske [:Dolske]]

Removing "post-1.8-branch" from whiteboard; as this is almost certainly a problem on 1.8. [1.9 is sort of ok at the moment, because as bug 396295 reportsnon-HTTP[S] auth is broken.]

Comment 2

[Justin Dolske [:Dolske]]

Attachment: Patch, work in progress

This is the basic idea, needs some testing and refinement.

* For protocol-based logins, always set the .httpRealm field in the nsILoginInfo
* Fixes bug 379111. It touches the same code, and would have the same backwards-compatibility issues (in for a penny, in for a pound)
* Mostly fixes issues where explicitly specifying the default port in a URL could cause it to not match a stored login, and vice versa.
* A non-HTTP[S] URL with a username/password (ftp://user:[e-mail]) shouldn't be stored with the user/pass shown. [Need to see if the old code was trying to do that or not, the current code would have but was broken due to bug 396295]

To do:
* Add migration code somewhere so existing logins will be upgraded to the proper format.
* Investigate if we're consistently handling IDN
* NS_GetAuthKey in windowwatcher should be removed, as it's wrong and unneeded.
* Take a look at including some more info about the authentication in the stored login (eg, so that a SSL login with strong crypto is never used w/o crypto, or so a HTTP digest-auth login isn't used with basic-auth).
* Biesi should probably give this a review pass when it's ready

Comment 3

[Justin Dolske [:Dolske]]

The code to make URLs consistent ought to address 393973 as well.

Comment 4

[Justin Dolske [:Dolske]]

This patch is starting to get a bit large, so for branch I'd like to minimize the risk of change and the pain of porting to C++... I think a simpler fix on branch would be to only match non-HTTP[S] logins to a form when the formSubmitURL is an exact match.

If the stored login was for a protocol, the formSubmitURL will always be blank. This is normally treated as a wildcard value. Form logins saved since 360493 was fixed will not be blank. This would mean that old (pre-360493) form logins for forms served from a non-HTTP[S] protocol would stop working.

I think this is probably a rare case (I've never even seen a login form served from ftp://). If the user enters their password and saves the new login, that login should work.

Comment 5

[Justin Dolske [:Dolske]]

Attachment: Patch, work in progress v.2

* Decided bug 393973 doesn't need to be fixed here, so I won't.
* Storing auth mechanism / crypto strength would be nice to do if we're going to change the format anyway, but at this point I'm going to punt for FF3.
* Will file followup for checking correct IDN usage, not required here.

Todo:

* Test proxy and IDN usage/conversion
* Write tests
* Do we need to switch to signons3.txt? [Could also check to see if conversion is robust enough to just always run it.]

Comment 6

[Justin Dolske [:Dolske]]

Attachment: Patch for review, v.1 (Branch)

This should work. Will request review when I actually test it. :-)

Comment 7

[Justin Dolske [:Dolske]]

So... I think this might actually not be a problem on branch. For whatever reason, password manager is not getting the DOMContentLoaded event for non-http[s] pages... Some simple debug printfs show that the WebProgressListener is correctly being triggered and adds event listeners, but the listener is never called. Reproducable for ftp:// and file://. Same test file from http:// correctly triggers the event. This implies passwords are never filled in for non-http[s]:// pages, so this bug wouldn't be exploitable.

I'm not sure if the failure to trigger  and event here is a known bug or not. We could land the branch patch anyway (belt-and-suspenders), although it can't really be tested to verify it prevents the problem.

Comment 8

[Mark Banner (:standard8)]

Attachment: Sections 2 and 3 incorporating changes for mailnews

I've just looked at this migration from the mailnews login perspective. I'm not sure how we want to play it, but to get this patch to work (for mailnews) I had to change some of how section 2 worked and a little of section 3 (of the trunk v2 patch).

I'm attaching the full sections 2 and 3 that we would need.

The changes are mainly to deal with things like:

ldap://localhost:389/dc=test
and
mailbox://mm@host/

This will split the URIs up into hostname, username and set the httpRealm to the original URI minus the username. This will mean everything will be set up well for mailnews.

Comment 9

[Justin Dolske [:Dolske]]

Attachment: Patch for review, v.1

Ugh, let it be over! Here's the updated, hopefully final, patch for review. Test updates forthcoming.

Notes:
* We should relnote that this has the potential to break logins stored for HTTP auth on non-standard ports and proxy servers.
* I tested the proxy authentication stuff since I touched a bit of it here and there was a "TODO: Untested" comment. :-)

Stuff to spinoff, to keep this bug in scope:
* Inconsistencies in IDN handling
* mailnews/Thunderbird conversion
* NS_GetAuthHostPort / NS_GetAuthKey in promptUtils.h are incorrect now, and should be removed with the rest of the old password manager headers when possible.

Comment 10

[Justin Dolske [:Dolske]]

Attachment: Small addition to "Patch for review, v.1"

Oops, I was cleaning up my tree and saw I failed to attach this bit. As the "XXX" indicates, I'll also need to pull and update the prefs files for the projects that are not in a normal FF tree (I looked at what we did for bug 360493, when moving to signons2.txt).

Comment 11

[Justin Dolske [:Dolske]]

Attachment: Patch for review, v.2

Updated to trunk, changed default-port code now that bug 403480 has landed, and made _upgrade_entry_to_2E() add both http:// and https:// versions of a login when it can determine which is correct (per discussion on IRC).

Comment 12

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

Comment on attachment 290330 [details] [diff] [review]
Patch for review, v.2

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

>+    _getAuthTarget : function (aChannel, aAuthInfo) {

>+            // Proxies don't have a scheme, but we'll use "proxy://"
>+            // so that it's more obvious what the login is for.

This concerns me a bit... it's possible for a "proxy" scheme to be implemented by an extension, say, and I'm not sure if that would cause trouble if someone tried to use the password manager to save passwords for it. Is that a valid concern? Can we use some other way of differentiating proxy logins? Just omit it?

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

>     _getSignonsFile : function() {

>+        // We've used a number of prefs over time due to compatibility issues.
>+        // Use the filename specified in the newest pref, but import from
>+        // older files if needed.
>+        var prefs = ["SignonFileName3", "SignonFileName2", "SignonFileName"];
>+        for each (var prefName in prefs) {

This code assumes iteration will start at 0 and end at length-1, but that isn't garanteed to be true according to the spec, iirc. It would be clearer to use a normal for loop, since the ordering matters.

>+    _upgrade_entry_to_2E : function (aLogin) {

>+        if (aLogin.hostname.indexOf("://") == -1) {
>+            var oldHost = aLogin.hostname;
>+            var extraLogin = null;

Never assigned to, the "add both logins" code isn't in this patch. Forgot to rediff, or something?

>+            // Parse out "host:port".
>+            var matches = /^(.+):(\d+)$/.exec(aLogin.hostname);

I'd kinda prefer using nsIIOService/newURI to parse this, even if you have to prepend the scheme to get it to work.

>+                // Upgrading an entry to 2E can sometimes result in the need
>+                // to create an extra login.
>+                var entries = [entry];
>+                if (formatVersion <= 0x2d)
>+                    entries = this._upgrade_entry_to_2E(entry);

nit: I find < 0x2e easier to read given the comment and method name.

>Index: modules/libpref/src/init/all.js

>-pref("signon.SignonFileName2",              "signons2.txt");
>+pref("signon.SignonFileName2",              "signons2.txt"); // obsolete
>+pref("signon.SignonFileName3",              "signons3.txt");
>+// XXX also update other files in tree referencing SignonFileName2 (minimo, TB, sunbird)

Why do all those users have these prefs duplicated in their pref files? If they're all the same you might as well just remove them, no? New bug, maybe.

Need tests for this, particularly for the migration code. r=me with those addressed, I'll take a look at the updated patch.

Comment 13

[Justin Dolske [:Dolske]]

(In reply to comment #12)

> This concerns me a bit... it's possible for a "proxy" scheme to be implemented
> by an extension, say, and I'm not sure if that would cause trouble if someone
> tried to use the password manager to save passwords for it. Is that a valid
> concern? Can we use some other way of differentiating proxy logins? Just omit
> it?

Maybe moz-proxy://?

Dropping the scheme could work too, although I wanted to make it clear what the login was for (to help avoid the mess that got us into this state to begin with).

> Why do all those users have these prefs duplicated in their pref files? If
> they're all the same you might as well just remove them, no? New bug, maybe.

Isn't there some long-standing hairy bug about toolkit prefs?

Comment 14

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

(In reply to comment #13)
> Maybe moz-proxy://?

I guess so. moz-pwmgr-proxy for extra paranoid specificity?

> Isn't there some long-standing hairy bug about toolkit prefs?

I guess there's bug 335931 and maybe some other bug I'm misremembering, but I don't see any reason why we'd need to wait for a "toolkit" pref file instead of just using all.js for now - new bug fodder, I guess.

Comment 15

[Justin Dolske [:Dolske]]

Attachment: Patch for review, v.3

Address review comments. Testcases coming up next.

Comment 16

[Justin Dolske [:Dolske]]

Attachment: Patch for review, v.4

Updated, with test cases... Yay tests, boo tests. *sigh*

I found a handful null vs "" edge cases and fixed those, but otherwise it's mostly the same. While verifying that the tests correctly tested the patch, I found some flaws in head_storage_legacy_1.js that could let errors slip though, so fixed that too. I trimmed the giant signons-08.txt test because the inefficient verification code in the test was taking too long to wade through it.

Comment 17

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

Comment on attachment 291631 [details] [diff] [review]
Patch for review, v.4

I didn't review all of the tests closely. The code changes look good.

Comment 18

[Justin Dolske [:Dolske]]

Checked in.

Comment 19

[[e-mail]]

Comment on attachment 291631 [details] [diff] [review]
Patch for review, v.4

>-            realm += uri.hostPort;
>+            realm = uri.scheme + "://" + uri.host;
>+
>+            // If the URI explicitly specified a port, only include it when
>+            // it's not the default. (We never want "http://foo.com:80")
>+            var port = uri.port;
>+            if (port != -1) {
>+                var handler = this._ioService.getProtocolHandler(uri.scheme);
>+                if (port != handler.defaultPort)
>+                    realm += ":" + port;
>+            }
Given bug 403480 is this still possible? (If I type http://foo:80/ into the location bar the :80 is stripped.)

Comment 20

[Justin Dolske [:Dolske]]

When I was testing the upgrade code, I found that feeding "http://foo:80" to nsIURI was still giving me uri.port == 80. I figured it was safest and easiest to just leave this code in, lest there be some other way trigger it. Belt, suspenders, etc.

Comment 21

[Justin Dolske [:Dolske]]

Attachment: Patch for IDL comment changes

Since the hostname formatting changed, some comments in the IDL files need updating too.

Comment 22

[Justin Dolske [:Dolske]]

IDL comment changes checked in.

Checking in toolkit/components/passwordmgr/public/nsILoginInfo.idl;
/cvsroot/mozilla/toolkit/components/passwordmgr/public/nsILoginInfo.idl,v  <--  nsILoginInfo.idl
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/components/passwordmgr/public/nsILoginManager.idl;
/cvsroot/mozilla/toolkit/components/passwordmgr/public/nsILoginManager.idl,v  <--  nsILoginManager.idl
new revision: 1.6; previous revision: 1.5
done
Checking in toolkit/components/passwordmgr/public/nsILoginManagerStorage.idl;
/cvsroot/mozilla/toolkit/components/passwordmgr/public/nsILoginManagerStorage.idl,v  <--  nsILoginManagerStorage.idl
new revision: 1.8; previous revision: 1.7
done