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

VERIFIED FIXED

Status

Core Graveyard
Installer: XPInstall Engine
VERIFIED FIXED
13 years ago
2 years ago

People

(Reporter: philor, Assigned: dveditz)

Tracking

({fixed-aviary1.0.1, fixed1.7.6})

Trunk
x86
Windows XP
fixed-aviary1.0.1, fixed1.7.6
Dependency tree / graph
Bug Flags:
blocking-aviary1.0.1 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:fix], URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

13 years ago
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.
(Assignee)

Comment 1

13 years ago
Need to fix. The Suite likely has the same problem, ossibly with variants on how
escaped characters are handled.
Assignee: firefox → dveditz
Whiteboard: [sg:fix]
(Assignee)

Updated

13 years ago
Blocks: 278184

Updated

13 years ago
Flags: blocking-aviary1.0.1?

Comment 2

13 years ago
+ 1.0.1
Flags: blocking-aviary1.0.1? → blocking-aviary1.0.1+

Comment 3

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

Comment 4

12 years ago
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.)
(Assignee)

Comment 5

12 years ago
Created attachment 174782 [details] [diff] [review]
strip user:pass from URL display
Attachment #174782 - Flags: superreview?(darin)
Attachment #174782 - Flags: review?(dougt)
Attachment #174782 - Flags: approval-aviary1.0.1?

Comment 6

12 years ago
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.
Attachment #174782 - Flags: review?(dougt) → review+
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.
Attachment #174782 - Flags: approval-aviary1.0.1? → approval-aviary1.0.1+
(Assignee)

Comment 8

12 years ago
Fix checked into trunk plus 1.7 and aviary1.0.1 branches.
Blocks: 278186
Keywords: fixed-aviary1.0.1, fixed1.7.6
(Assignee)

Updated

12 years ago
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Attachment #174782 - Flags: superreview?(darin) → superreview+
Component: General → Installer: XPInstall Engine
Flags: review+
Product: Firefox → Core
Version: 1.0 Branch → Trunk

Comment 9

12 years ago
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
Status: RESOLVED → VERIFIED
(Assignee)

Updated

12 years ago
Group: security
Created attachment 187766 [details] [diff] [review]
patch for 1.4 branch
Attachment #187766 - Flags: superreview?(darin)

Comment 11

12 years ago
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.
(Assignee)

Comment 12

12 years ago
(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

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

sr- requesting revised patch per previous comments.
Attachment #187766 - Flags: superreview?(darin) → superreview-
Created attachment 189142 [details] [diff] [review]
patch for 1.4 branch v2

modification according to comments
Attachment #187766 - Attachment is obsolete: true
Attachment #189142 - Flags: superreview?(darin)

Comment 15

12 years ago
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?

Updated

12 years ago
Attachment #189142 - Flags: superreview?(darin) → superreview-
(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
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.