the fix for bug 161228 possibly re-introduced the princeton-style exploit for password manager. mitch, dveditz and i discussed this issue before choosing to go ahead with the fix for bug 161228, which was a regression resulting from the original princeton attack fix (bug 149943). basically, the problem is that our DNS table now maps host:port -> ipaddress instead of host -> ipaddress. see bug 161228 for a description of why this was necessary. as a result, we may connect to a different ip node depending on the desired port. this is expected, but since in most cases password manager only stores the hostname of the site corresponding to the remembered password, mozilla might be vulnerable to giving out a remembered password to a malicious website. i'm not certain that an attack actually exists in this case, because the user would have to enter their password under a domain controlled by the attacker. at first glance, that seems unlikely.
Shouldn't the password manager always store the password based on protocol+hostname? Otherwise an attacker can steal passwords that should be been protected by SSL by faking up a non-SSL connection. Consider this scenario: Alice connects to https://site.com and enters her password. Later, Alice connects to http://site2.com. Mallory hijacks Alice's HTTP connection to http://site2.com and injects an <IFRAME SRC="http://site.com">. Mallory then hijacks Alice's HTTP connection to http://site.com, injecting a password-capturing form field and arranging the content so that a button that appears to be on site2.com actually submits the form. Alice clicks on the button, and Mallory gets Alice's https://site.com password.
yes, indeed, that is the big problem. because we used to "pin" the ip-address of site.com, it was much harder to spoof site.com (although in your example, another browser session would have defeated our ip-address pinning). now it becomes easier since the ip-address of https://site.com and http://site.com are not necessarily the same. password manager should store not only the protocol and hostname, but also the port. this will be tricky to do without breaking existing password databases. i'm not exactly sure how best to migrate old password databases.
When you see a legacy entry for HTTP auth, convert it to a new entry for HTTPS:667 (or whatever the port is for https). This is nice and conservative because HTTPS shouldn't be spoofable. This means that people will have to reenter their passwords for plain HTTP sites, but there should not be so many of those because those passwords are easily sniffed. This bug is actually quite serious; the scenario I outlined is not hard to pull off by someone who is on the same local network as the user. We really need this fixed for 1.2.
sounds like a good plan... working on a patch.
one possible problem. a lot of the time, a password form is served up via http:, but the resulting form submission is to a https: site. password manager only knows about the http: site. so, while the password is meant to be securely transfered, password manager will make it appear as though it is an insecurely transfered password. this is probably still ok, but it screws with our assumption that most password entries are for https: sites. in fact, most in my password file are for http: sites that get POSTed to a https: site. what to do, what to do.
If we key off the form's protocol+port then those passwords will be stealable. Can't we change the password engine to key off the form submission URL?
An HTTP form page is just plain insecure against active attacks. Someone can hijack the connection, and replace the form page with their own form that uploads the password to their own server (e.g. by generating a GET request using a carefully constructed dynamically generated IMG SRC). These sites should be evangelised. Anyway, this means we might as well key off the form's protocol and port since anything else is no more secure. How about this, a little more complicated: leave legacy (host only) entries alone initially. When looking for a password entry, look up using host+port+protocol first, and use that if found. If not found, look for a legacy entry using only host. If we get a legacy entry, then upgrade the entry by adding the current port+protocol to its key.
yeah, that's sort of what dveditz and i just came up with. we could try to phase out old-style password entries as we visit them (possibly inside the code to remember a password entry).
i thought about this solution some more and i'm starting to think that it might be acceptable to leave the existing password entries as is. that way a user would not lose the ability to go back to an older version of mozilla. this is not really a complete solution as existing profiles will be vulnerable, but at least any new passwords will be stored with the fully qualified key. does this sound good to everyone?
Created attachment 103020 [details] [diff] [review] v1 patch this patch makes the following changes: 1- password forms now use "scheme://host[:port]" as database key. the port is optionally not specified since the scheme implies a default port. this seems reasonable since the keys will be shown to users in the password manager UI. this format should be familiar to users since it resembles URLs. 2- database lookup (SINGSIGN_RestoreSignonData) will first try the fully qualified key. if password entries is not found, then it will try the old-style host key. 3- database write (SINGSIGN_RememberSignonData) will always write a fully qualified key. if password entry exists under old-style key, it will not be modified or deleted. i factored the creation of the fully qualified key out into a helper function called si_ExtractRealm, which takes a nsIURI as an argument. please review.
Comment on attachment 103020 [details] [diff] [review] v1 patch r=morse
Comment on attachment 103020 [details] [diff] [review] v1 patch >+ realm = scheme + NS_LITERAL_CSTRING("://") + hostPort; Would nsIURI::GetPrePath() have simplified this? I guess you're trying to avoid any username/password by doing this, but you could argue that if the URI specifies a username/password on a password form then it might know something we don't and require different single-signon values. I'm fine with this, just wanted to raise the question. sr=dveditz. Is this worth getting on the branch, too?
I really think we should do something about legacy passwords. Maybe in six months or so we could land a patch which removes any legacy passwords :-).
dveditz: i thought about that, but the username/password is parsed out of the URL by necko before communicating with the site. in fact, HTTP only sends username/password to the server after it is challenged. so, i think we should be ok filtering out username/password. yeah, i think this is important for moz 1.0.2. roc: yeah, that sounds like a good idea to me. i'll file a follow up bug.
Comment on attachment 103020 [details] [diff] [review] v1 patch a=asa for checkin to 1.2 (on behalf of drivers).
i filed bug 175120 about the legacy passwords.
adt+ for branch -- have you mailed drivers for approval?
dveditz: no i have not, but i will ;-) we will also want the fix for bug 175495 on the branch in addition to this patch.
firstname.lastname@example.org for 1.0 branch; change mozilla1.0.2+ to fixed1.0.2 when checked in.
Verified fix checked into bonsai.mcom.com, adding verified1.0.2 keyword