162520 - possible princeton-style password stealing exploit

Status: RESOLVED FIXED

(SeaMonkey :: Passwords & Permissions, defect, P1)

Description

[Darin Fisher]

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.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 2

[Darin Fisher]

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.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 4

[Darin Fisher]

sounds like a good plan... working on a patch.

Comment 5

[Darin Fisher]

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.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 8

[Darin Fisher]

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).

Comment 9

[Darin Fisher]

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?

Comment 10

[Darin Fisher]

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

[Stephen P. Morse]

Comment on attachment 103020 [details] [diff] [review]
v1 patch

r=morse

Comment 12

[Daniel Veditz [:dveditz]]

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?

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 :-).

Comment 14

[Darin Fisher]

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 15

[Asa Dotzler [:asa]]

Comment on attachment 103020 [details] [diff] [review]
v1 patch

a=asa for checkin to 1.2 (on behalf of drivers).

Comment 16

[Darin Fisher]

fixed-on-trunk

Comment 17

[Darin Fisher]

i filed bug 175120 about the legacy passwords.

Comment 18

[Daniel Veditz [:dveditz]]

adt+ for branch -- have you mailed drivers for approval?

Comment 19

[Darin Fisher]

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.

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

a=[e-mail] for 1.0 branch; change mozilla1.0.2+ to fixed1.0.2 when
checked in.

Comment 21

[Darin Fisher]

fixed1.0.2

Comment 22

[Terri Preston]

Verified fix checked into bonsai.mcom.com, adding verified1.0.2 keyword