472824 - promptPassword should unescape user names before use.

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P2)

Description

[Mark Banner (:standard8)]

Attachment: The fix

I've just been debugging some mailnews issues with promptPassword.

What I have realised is that some user names can be of the format:

foo.bar@host

When mailnews forms the url, it encodes this so we get urls such as:

smtp://foo%2Ebar%40host@host/

When promptPassword comes along and extracts the user name from the url, it is using the escaped version rather than the unescaped version to compare to the stored logins, and hence isn't matching.

We should be storing the unescaped form (as we can and its prettier to display), indeed, the migration code already stores the unescaped version, we should do the same in promptPassword.

The fix is simple, attaching patch and test case - I'm still just rebuilding Firefox and testing there, so holding off on requesting review.

Comment 1

[Mark Banner (:standard8)]

Attachment: The fix v2

Minor change to patch to add login2C to the list of variables. All FF password manager tests still pass.

Comment 2

[Phil Ringnalda (:philor)]

Assuming this is the reason my migrated username for gmail smtp didn't work until I let the prompt save a new "philringnalda%40gmail%2Ecom" one, tb3needs rather badly.

Comment 3

[Justin Dolske [:Dolske]]

Does promptUsernameAndPassword() also need to support this?

Do all the mailnews callers explicitly urlencode the username part of the realm string when constructing it? I'm a little wary of this unencoding something a caller didn't expect. EG: aPasswordRealm == "http://[e-mail]"

Seems like the better, or else there may be funky attacks if someone can make the caller think the username is "[e-mail]/" (slash is important), such that the URL parsing gets confused.

Should probably also add a comment to the IDL saying that callers *must* urlencode the username to avoid these problems.

Comment 4

[Mark Banner (:standard8)]

(In reply to comment #3)
> Does promptUsernameAndPassword() also need to support this?

Not as far as I can tell because that's not extracting usernames from URIs.

> Do all the mailnews callers explicitly urlencode the username part of the realm
> string when constructing it?

I need to check this still.

> I'm a little wary of this unencoding something a
> caller didn't expect. EG: aPasswordRealm == "http://[e-mail]"

Good point.

> Seems like the better, or else there may be funky attacks if someone can make
> the caller think the username is "[e-mail]/" (slash is important),
> such that the URL parsing gets confused.

I also need to check if the url parsing is going to get confused with smtp://user@host@realhost/

Depending on the answers to the questions above my current thoughts are:

a) do this patch with additional comments in the idl.
b) don't apply this bug, and back out bug 461341 and have escaped names shown in password manager
c) keep bug 461341 and don't escape names in mailnews uris (which may be difficult as all the non-smtp ones are essentially rdf uris which prefs rely on).

I'll do some digging tomorrow to confirm what mailnews is doing in the various areas.

Comment 5

[Mark Banner (:standard8)]

Attachment: The fix v3

Updated patch to include comment as discussed on irc.

Comment 6

[Mark Banner (:standard8)]

Requesting blocking as TB 3 needs this. We either need to land straight after 1.9.1b3 or before - in any case I'd like to get it into 1.9.1 by approx 28th to allow time to bake for TB 3 beta 2.

This bug is needed for mailnews to switch to toolkit's password manager. Thunderbird/SeaMonkey will be using promptPassword, afaik Firefox doesn't so this is little effect on Firefox. If we don't get this bug in in time then there's at least two other bugs we need to find alternative fixes for, and it will increase work in extensions which we are trying to get to support Thunderbird as well as Firefox (e.g. weave).

Comment 7

[Justin Dolske [:Dolske]]

Comment on attachment 356589 [details] [diff] [review]
The fix v3

r+, but also add a test to ensure usenames like "100%beef" are correctly handled (ie, test for too much unescaping).

Also flagging mconnor for sr, just because nsIAuthPrompt has been around forever, and this makes an interesting test of the proposed "sr all API changes" policy. :)

Comment 8

[Mark Banner (:standard8)]

Attachment: Patch for checkin

100%beef in a uri comes up as malformed, so I included two additional tests, one with the uri username of 100%25beef, and the other with 100@beef. This is the patch I'll be pushing.

Comment 9

[Mark Banner (:standard8)]

Checked into trunk: http://hg.mozilla.org/mozilla-central/rev/5542884b9bdc

Comment 10

[Mark Banner (:standard8)]

Checked into 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a5770527971e

Comment 11

[Justin Dolske [:Dolske]]

>+  login2E.init("http://example2.com", null, "http://example2.com",
>+               "100%beef", "user3pass", "", "");
...
>+isOk = prompter1.promptPassword(dialogTitle(), dialogText, "http://[e-mail]",
>+                                Ci.nsIAuthPrompt.SAVE_PASSWORD_NEVER, pword);

Right, that's what I was expecting. Caller has the username encoded, and login manger is decoding it *once*. If it decoded it twice, it would get a 0xBE character and not match the login.