The default bug view has changed. See this FAQ.

promptPassword should unescape user names before use.

RESOLVED FIXED in mozilla1.9.1b3

Status

()

Toolkit
Password Manager
P2
normal
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

({fixed1.9.1})

Trunk
mozilla1.9.1b3
fixed1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tb3needs])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

8 years ago
Created attachment 356176 [details] [diff] [review]
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.
(Assignee)

Comment 1

8 years ago
Created attachment 356181 [details] [diff] [review]
The fix v2

Minor change to patch to add login2C to the list of variables. All FF password manager tests still pass.
Attachment #356176 - Attachment is obsolete: true
Attachment #356181 - Flags: review?(dolske)
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.
Whiteboard: [tb3needs]
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://100%beef@burgers.com"

Seems like the better, or else there may be funky attacks if someone can make the caller think the username is "user@othersite.com/" (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.
(Assignee)

Comment 4

8 years ago
(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://100%beef@burgers.com"

Good point.

> Seems like the better, or else there may be funky attacks if someone can make
> the caller think the username is "user@othersite.com/" (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.
(Assignee)

Comment 5

8 years ago
Created attachment 356589 [details] [diff] [review]
The fix v3

Updated patch to include comment as discussed on irc.
Attachment #356181 - Attachment is obsolete: true
Attachment #356589 - Flags: review?(dolske)
Attachment #356181 - Flags: review?(dolske)
(Assignee)

Comment 6

8 years ago
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).
Flags: blocking1.9.1?
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
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. :)
Attachment #356589 - Flags: superreview?(mconnor)
Attachment #356589 - Flags: review?(dolske)
Attachment #356589 - Flags: review+

Updated

8 years ago
Attachment #356589 - Flags: superreview?(mconnor) → superreview+
(Assignee)

Comment 8

8 years ago
Created attachment 357115 [details] [diff] [review]
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.
Attachment #356589 - Attachment is obsolete: true
Attachment #357115 - Flags: superreview+
Attachment #357115 - Flags: review+
(Assignee)

Comment 9

8 years ago
Checked into trunk: http://hg.mozilla.org/mozilla-central/rev/5542884b9bdc
Flags: in-testsuite+
(Assignee)

Updated

8 years ago
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 10

8 years ago
Checked into 1.9.1: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a5770527971e
Keywords: fixed1.9.1
>+  login2E.init("http://example2.com", null, "http://example2.com",
>+               "100%beef", "user3pass", "", "");
...
>+isOk = prompter1.promptPassword(dialogTitle(), dialogText, "http://100%25beef@example2.com",
>+                                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.
You need to log in before you can comment on or make changes to this bug.