Last Comment Bug 268059 - InstallTrigger.install doesn't check for username:password URL spoofing
: InstallTrigger.install doesn't check for username:password URL spoofing
Status: VERIFIED FIXED
[sg:fix]
: fixed-aviary1.0.1, fixed1.7.6
Product: Core Graveyard
Classification: Graveyard
Component: Installer: XPInstall Engine (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Daniel Veditz [:dveditz]
:
:
Mentors:
http://philringnalda.com/mtests/xpi-s...
Depends on:
Blocks: sg-ff101 sg-moz176
  Show dependency treegraph
 
Reported: 2004-11-05 23:57 PST by Phil Ringnalda (:philor)
Modified: 2015-12-11 07:21 PST (History)
8 users (show)
chofmann: blocking‑aviary1.0.1+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
strip user:pass from URL display (4.18 KB, patch)
2005-02-18 23:32 PST, Daniel Veditz [:dveditz]
darin.moz: superreview+
dbaron: approval‑aviary1.0.1+
Details | Diff | Splinter Review
patch for 1.4 branch (4.47 KB, patch)
2005-06-30 01:03 PDT, Nian Liu(n/a in a long time)
darin.moz: superreview-
Details | Diff | Splinter Review
patch for 1.4 branch v2 (4.46 KB, patch)
2005-07-12 21:04 PDT, Nian Liu(n/a in a long time)
darin.moz: superreview-
Details | Diff | Splinter Review

Description Phil Ringnalda (:philor) 2004-11-05 23:57:08 PST
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.
Comment 1 Daniel Veditz [:dveditz] 2004-12-16 13:40:07 PST
Need to fix. The Suite likely has the same problem, ossibly with variants on how
escaped characters are handled.
Comment 2 chris hofmann 2005-02-02 12:29:17 PST
+ 1.0.1
Comment 3 Asa Dotzler [:asa] 2005-02-10 12:39:46 PST
need an eta on this. We're running short on time.
Comment 4 Chris Beard 2005-02-18 17:55:02 PST
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] 2005-02-18 23:32:54 PST
Created attachment 174782 [details] [diff] [review]
strip user:pass from URL display
Comment 6 Doug Turner (:dougt) 2005-02-18 23:40:25 PST
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: ⌚️UTC-8 2005-02-18 23:43:47 PST
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] 2005-02-19 00:32:55 PST
Fix checked into trunk plus 1.7 and aviary1.0.1 branches.
Comment 9 Jay Patel [:jay] 2005-02-19 17:33:58 PST
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) 2005-06-30 01:03:38 PDT
Created attachment 187766 [details] [diff] [review]
patch for 1.4 branch
Comment 11 Darin Fisher 2005-06-30 14:05:52 PDT
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] 2005-06-30 23:30:05 PDT
(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 2005-07-07 12:07:23 PDT
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) 2005-07-12 21:04:41 PDT
Created attachment 189142 [details] [diff] [review]
patch for 1.4 branch v2

modification according to comments
Comment 15 Darin Fisher 2005-07-13 19:09:02 PDT
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) 2005-07-31 20:31:54 PDT
(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

Note You need to log in before you can comment on or make changes to this bug.