User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20041101 Firefox/1.0RC2 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20041101 Firefox/1.0RC2 Between not checking for a spoofed URL with a username/password, and the unresizeable, unwrapped dialog for XPInstall, it's possible to make a fairly convincing spoofed URL for an XPI with InstallTrigger: <a href="javascript:void(InstallTrigger.install({'Safety First!':'http://update%2Emozilla%2Eorg%5C%5Cextensions%5C%5Csafe_as_houses@philringnalda.com/mozilla/livemarkthis.xpi'}))">Install from update.mozilla.org</a> The install dialog (with my font size, on WinXP) will then say the xpi is coming from |http://update.mozilla.org\extensions\safe_a|, with the full URL only visible by copying and pasting into another program. I'm not sure if it's possible to get a more convincing URL than that, thanks to other bugs I discovered while trying (the EM apparently misparses URLs, so any / in the username (even as %2F) causes a download failure, and the install dialog treats \ as an escape (even as %5C), so a pair are required to display one). If that's the very best spoofed URL possible, it's not quite earth-shaking serious, but then I'm not a very good spoofer. Reproducible: Always Steps to Reproduce: 1. Visit URL, click link to install. 2. Add site to install-prompt whitelist. 3. Click link again to really install. 4. Vaguely examine URL in install prompt, think "update.mozilla.org, that's safe" Actual Results: The extension (it's just my right-click "Add Live Bookmark for this link", safe enough to actually install) was installed. Expected Results: Throw up the spoof-warning alert before the install prompt.

Need to fix. The Suite likely has the same problem, ossibly with variants on how escaped characters are handled.

+ 1.0.1

need an eta on this. We're running short on time.

My opinion is that the aspect of this bug which relates to the display of white-listed sites is the most critical part. This is our last line of defense, and needs to be a protected means of ensuring people know who they are trusting. (Especially in light of potential attacks wherein malicious Extensions add to the white-list.)

Comment on attachment 174782 [details] [diff] [review] strip user:pass from URL display fixup or remove the comment in xpinstall/src/nsXPITriggerInfo.cpp and r=dougt.

Comment on attachment 174782 [details] [diff] [review] strip user:pass from URL display a=dbaron assuming (1) you get sr and (2) that you've tested that an install requiring user:pass still works.

Fix checked into trunk plus 1.7 and aviary1.0.1 branches.

Verified Fixed. Stripping the username/password now results in properly displaying the expected url "http://philringnalda.com/mozilla/livemarkthis.xpi" in the install dialog. Aviary 1.0.1 branch: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.6) Gecko/20050219 Firefox/1.0.1 Mozilla 1.7.6 branch: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.6) Gecko/20050219 MozillaTrunk: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050219

Comment on attachment 187766 [details] [diff] [review] patch for 1.4 branch >Index: src/nsXPITriggerInfo.cpp >+ nsCAutoString spec; >+ spec.Assign("\0"); >+ uri->SetUserPass(spec); >+ uri->GetSpec(spec); OK, I don't understand why you need to call |spec.Assign("\0");| What is it that you are trying to accomplish? |spec| is already null terminated just by virtue of being a nsCAutoString.

(In reply to comment #11) > OK, I don't understand why you need to call |spec.Assign("\0");| [...] > |spec| is already null terminated just by virtue of being a nsCAutoString. I think they're trying to make sure the string is empty. The more usual idiom for that is mystr.Truncate(), but a newly created nsCString is going to be an empty to start with. But making the variable spec do double-duty here is confusing. Just set the user/pass to a literal blank string (since the EmptyCString() helper didn't exist back then).

Comment on attachment 187766 [details] [diff] [review] patch for 1.4 branch sr- requesting revised patch per previous comments.

modification according to comments

Comment on attachment 189142 [details] [diff] [review] patch for 1.4 branch v2 >Index: src/nsXPITriggerInfo.cpp >+ nsCAutoString spec; >+ uri->SetUserPass(NS_LITERAL_CSTRING("\0")); >+ uri->GetSpec(spec); I believe that you should change this to: nsCAutoString spec; uri->SetUserPass(spec); uri->GetSpec(spec); Is there some reason why this does not work?

(In reply to comment #15) > (From update of attachment 189142 [details] [diff] [review] [edit]) > >Index: src/nsXPITriggerInfo.cpp > > >+ nsCAutoString spec; > >+ uri->SetUserPass(NS_LITERAL_CSTRING("\0")); > >+ uri->GetSpec(spec); > > I believe that you should change this to: > > nsCAutoString spec; > uri->SetUserPass(spec); > uri->GetSpec(spec); > > Is there some reason why this does not work? > sure that works. My opinion is that not give variable spec double duty as Daniel said in comment 12