268059 - InstallTrigger.install doesn't check for username:password URL spoofing

Status: VERIFIED FIXED

(Core Graveyard :: Installer: XPInstall Engine, defect)

Description

[Phil Ringnalda (:philor)]

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://[e-mail]/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.

Comment 1

[Daniel Veditz [:dveditz]]

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

Comment 2

[chris hofmann]

+ 1.0.1

Comment 3

[Asa Dotzler [:asa]]

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

Comment 4

[Chris Beard]

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 5

[Daniel Veditz [:dveditz]]

Attachment: strip user:pass from URL display

Comment 6

[Doug Turner (:dougt)]

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 7

[David Baron :dbaron:]

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.

Comment 8

[Daniel Veditz [:dveditz]]

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

Comment 9

[Jay Patel [:jay]]

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 10

[Nian Liu(n/a in a long time)]

Attachment: patch for 1.4 branch

Comment 11

[Darin Fisher]

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.

Comment 12

[Daniel Veditz [:dveditz]]

(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 13

[Darin Fisher]

Comment on attachment 187766 [details] [diff] [review]
patch for 1.4 branch 

sr- requesting revised patch per previous comments.

Comment 14

[Nian Liu(n/a in a long time)]

Attachment: patch for 1.4 branch v2

modification according to comments

Comment 15

[Darin Fisher]

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?

Comment 16

[Nian Liu(n/a in a long time)]

(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