389106 - (CVE-2007-3845) Escape URIs (especially quotes) when passing them to external protocol handlers

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Mike Connor [:mconnor]]

different takes on this from different people, biesi has a patch for nsOSHelperAppService.cpp

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

I should note... this patch patches windows only and it doesn't affect the URL in the dialog, just what's executed.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Is there an XP location we could patch?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 273260 [details] [diff] [review]
patch

This does look ok for now, though...

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

The XP locations are basically the external helper app service, which only has an nsIURI object, and the nsSimpleURI implementation, changing which affects much more than just this code, so I decided to patch this here instead.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 273260 [details] [diff] [review]
patch

To be safe all around, I'd do this in nsExternalHelperAppService::LoadURI -- get the spec, substitute, create a new URI, load that.

Otherwise we have to worry about various platforms...

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: alternative patch

something like this then?

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: alternative patch (branch version)

branch version of alternative patch (context changed due to thread manager)

Comment 9

[Ben Bucksch (:BenB)]

Possibly another (additional) way to fix this: IIRC, Unix has a way to launch applications directly, not via a shell, and passing commandline options one by one. I.e. instead of launching 'firefox.exe -url "foo:bar"', you say something alike exec("firefox.exe", ["-url", "foo:bar"]); - This has the advantage that the shell commandline parsing is not involved and spaces can appear in an argument without splitting it and without having to escape or quote it.
I don't know, if Windows supports that as well, but a superficial look at 
nsIProcess:run() says it does (and if not, maybe it's possible?).
http://mxr.mozilla.org/seamonkey/source/xpcom/threads/nsIProcess.idl
unsigned long run(in boolean blocking, [array, size_is(count)] in string args, in unsigned long count);

Comment 10

[Boris Zbarsky [:bzbarsky]]

On at least some platforms nsProcessCommon just pastes the args together into a string.  Or at least it used to,,,,

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 273284 [details] [diff] [review]
alternative patch

Indeed.

Comment 12

[Daniel Veditz [:dveditz]]

I'm uneasy seeing the apostrophe escaped. Although the old rfc 1738 explicitly calls out quote (") as an "unsafe" character, the apostrophe is enumerated as a "special" character allowed to appear unencoded.

rfc 1738 was superseded by rfc 3986 which puts apostrophe in the reserved "sub-delims" set, and says "URIs that differ in the replacement of a reserved character with its corresponding percent-encoded octet are not equivalent."

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

I can remove the apostrophe from the list, sure. I'm not quite clear on what the original bug was, would an unescaped apostrophe cause the issue too?

Comment 14

[Daniel Veditz [:dveditz]]

The original problem is bug 384384
  <iframe src='myscheme:foo" -injected evil'></iframe>

Which when we pass to ShellExecute is replaced in something like
  schemehandler.exe "%1"

so the stuff after the dquote ends up being seen as extra params by schemehandler

MS docs that the registry keys should look like the above, with dquotes, so I don't think we need to worry too much about similar problems with single quotes -- though it could be a problem if some handler registers the wrong way (similarly spaces would cause the same problem if the registry key had no quotes at all).

Worrying about platform differences is one consequence of putting the patch in a common point.

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 273260 [details] [diff] [review]
patch

sr=dveditz without the %27 line

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 273284 [details] [diff] [review]
alternative patch

sr=dveditz for this, too, without the %27 line, though I worry a little that some platform somewhere will need that escaped too. For the trunk, platform-specific escaping might be a better way to go.

Comment 17

[Ben Bucksch (:BenB)]

As dveditz said, it's easy for apps to forget the quotes in the registration, and I don't think spaces in URLs are/should be legal, so I'd escape them, too.

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

spaces are already escaped.

Comment 19

[Mike Connor [:mconnor]]

Comment on attachment 273285 [details] [diff] [review]
alternative patch (branch version)

a=mconnor on behalf of drivers, please land on GECKO181_20070712_RELBRANCH for
2.0.0.6, MOZILLA_1_8_BRANCH for 2.0.0.7, and MOZILLA_1_8_0_BRANCH for Thunderbird 1.5.0.13

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

alternative patch (branch version) checked in:

GECKO181_20070712_RELBRANCH:
Checking in nsExternalHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp,v  <--  nsExternalHelperAppService.cpp
new revision: 1.290.4.12.6.1; previous revision: 1.290.4.12
done

MOZILLA_1_8_BRANCH:
Checking in nsExternalHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp,v  <--  nsExternalHelperAppService.cpp
new revision: 1.290.4.13; previous revision: 1.290.4.12
done

MOZILLA_1_8_0_BRANCH:
Checking in nsExternalHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp,v  <--  nsExternalHelperAppService.cpp
new revision: 1.290.4.2.4.2; previous revision: 1.290.4.2.4.1
done

alternative patch / HEAD:
Enter passphrase for key '/c/Dokumente und Einstellungen/chb/.ssh/id_dsa': 
Checking in nsExternalHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp,v  <--  nsExternalHelperAppService.cpp
new revision: 1.317; previous revision: 1.316
done

Comment 21

[Daniel Veditz [:dveditz]]

Adding tracking keywords fixed1.8.0.13, fixed1.8.1.6, fixed1.8.1.7 per above

Comment 22

[Marcia Knous [:marcia]]

QA could use some help identifying which areas we should examine more closely for possible regressions. Thanks.

Comment 23

[Robert Strong (they/them - no direct email)]

I don't have aim installed so I can't verify this but I suspect that the IE / Trillian demo modified to use an iframe's src attribute would work as a testcase.
http://www.xs-sniper.com/nmcfeters/Cross-App-Scripting-2.html

Comment 24

[Christian :Biesinger (don't email me, ping me on IRC)]

Hm, actually, let me verify that statement about spaces...

Comment 25

[Christian :Biesinger (don't email me, ping me on IRC)]

OK, I'm not sure now what made me think that we escaped spaces, but we don't

Comment 26

[Daniel Veditz [:dveditz]]

Attachment: windows exe test handler

Heres a compiled version of the test program from the blog post. Put it on your machine somewhere (c:\tmp\testurl.exe) and then add the registry keys as described in the blog post.

Create a test page that has
  <a href='testurl:one" extra parameters'>double-quote test</a>
  <iframe src='testurl:two" extra parameters'></iframe>

The patch also escapes the back-tick (`) so make some urls that use that, too.
  <a href="testurl:three' single-quote-test">single quote</a>
  <a href='testurl:four` back-tick-test'>back-tick test</a>

The testurl source should work on linux just fine if someone compiles it, but I'm not sure how to register a protocol handler there. Ditto Mac.

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: escape spaces

Escape spaces. I'll have to test javascript: and data: URLs too once this build finishes...

Comment 28

[Christian :Biesinger (don't email me, ping me on IRC)]

ok, spaces in javascript: and data: seem to work fine even with the patch.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 273434 [details] [diff] [review]
escape spaces

r=bzbarsky

Comment 30

[Robert Strong (they/them - no direct email)]

Re: %1 being quoted... a quick scan shows that several handlers do not quote %1. For starters

callto	URL: CallTo Protocol	rundll32.exe msconf.dll,CallToProtocolHandler %l
ftp	URL:File Transfer Protocol	"C:\Program Files\Internet Explorer\IEXPLORE.EXE" %1
LDAP	URL:LDAP Protocol	"C:\Program Files\Outlook Express\wab.exe" /ldap:%1
mailto	URL:MailTo Protocol	"%ProgramFiles%\Outlook Express\msimn.exe" /mailurl:%1

Comment 31

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: escape spaces, branch version

this one just adds a ReplaceSubstring call, should be safer than touching NS_EscapeURL

Comment 32

[Daniel Veditz [:dveditz]]

Comment on attachment 273501 [details] [diff] [review]
escape spaces, branch version

sr=dveditz

I'm happier with this more conservative fix for the FF2 re-spin release. If the NS_EscapeURL fix looks OK on trunk we can do that perhaps for 2.0.0.7

Comment 33

[Daniel Veditz [:dveditz]]

Comment on attachment 273434 [details] [diff] [review]
escape spaces

sr=dveditz for trunk and possibly 1.8 branch.

Comment 34

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 273434 [details] [diff] [review]
escape spaces

trunk:
Checking in nsEscape.cpp;
/cvsroot/mozilla/xpcom/io/nsEscape.cpp,v  <--  nsEscape.cpp
new revision: 1.37; previous revision: 1.36
done

Comment 35

[Mike Connor [:mconnor]]

Comment on attachment 273501 [details] [diff] [review]
escape spaces, branch version

please land on all of the same branches as the previous patch

Comment 36

[Christian :Biesinger (don't email me, ping me on IRC)]

MOZILLA_1_8_BRANCH:
Checking in nsExternalHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp,v  <--  nsExternalHelperAppService.cpp
new revision: 1.290.4.14; previous revision: 1.290.4.13
done

MOZILLA_1_8_0_BRANCH:
Checking in nsExternalHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp,v  <--  nsExternalHelperAppService.cpp
new revision: 1.290.4.2.4.3; previous revision: 1.290.4.2.4.2
done

GECKO181_20070712_RELBRANCH:
Checking in nsExternalHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp,v  <--  nsExternalHelperAppService.cpp
new revision: 1.290.4.12.6.2; previous revision: 1.290.4.12.6.1
done

Comment 37

[Daniel Veditz [:dveditz]]

Fixes checked into branches, adding fix-tracking keywords

Comment 38

[Robert Strong (they/them - no direct email)]

Attachment: Handlers in html format

There are 188 URL Protocol handlers in this list... I removed the vast majority of duplicates but some may have slipped through.

Comment 39

[Robert Strong (they/them - no direct email)]

Comment on attachment 273528 [details]
Handlers in html format

Actually, a lot slipped through but it is quite a bit cleaner than it was. ;)

Comment 40

[Robert Strong (they/them - no direct email)]

Attachment: updated handlers in html format

Same list after I went through a bunch of the handlers. There is a very large number of protocol handlers that can be excluded by verifying that the param can be an escaped url. I've denoted these with an x in the "Safe?" column and would appreciate another set of eyes going over these just in case as well as checking the remaining ones.

Comment 41

[Colin Barrett [:cbarrett]]

Attachment: equivalent test program for mac os x

I wrote a quick app to act like the main.c test program that Jesper wrote. This one. however, works on Mac OS X.

Unzip it, open the .xcodeproj (with Xcode), and click the Build button (hammer shaped). That's enough to get it to register for the testurl:// protocol handler. FYI, on Mac OS X to register an application to handle specific URL types, you add an entry to it's Info.plist. Unlike Windows or Linux, URLs aren't passed in as command line arguments, the application is notified via an Apple Event (an RPC method on the Mac).

What's curious is that running this test app, it doesn't appear Firefox on the Mac is vulnerable. At least in the tests I did. I'm not entirely sure *why*, though.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Because we're not passing the URL on the command line on Mac?  The actual call we use is:

108     rv = ::LSOpenCFURLRef(myURLRef, NULL);

presumably this does the right thing.  ;)

Comment 43

[Robert Strong (they/them - no direct email)]

I believe we do have one scenario on Mac OS X where the data received by the Apple Event is converted to a command line
http://lxr.mozilla.org/seamonkey/source/toolkit/xre/nsCommandLineServiceMac.cpp

This is due to needing a restart during startup. We should be able to use similar handling in Windows if the need to restart on startup is removed.

Comment 44

[Marcia Knous [:marcia]]

Colin: Thanks for creating this test program. I am able to run the program fine, but I am wondering if it is expected to get a blank window (no error message or feedback)? Is it necessary for me to edit the plist file with a URL?

(In reply to comment #41)
> Created an attachment (id=273580) [details]
> equivalent test program for mac os x
> 
> I wrote a quick app to act like the main.c test program that Jesper wrote. This
> one. however, works on Mac OS X.
> 

Comment 45

[Reed Loden [:reed]]

Attachment: equivalent test program for linux i686

I took the code from the blog that reported this, corrected it to work with C99, and compiled it on my Linux i686 machine. Included is a compiled binary and the source code.

Comment 46

[Adam Guthrie]

Comment on attachment 273645 [details]
equivalent test program for linux i686

This is not going to be of any use unless someone can figure out how to do to GNOME what the Windows Registry entries in the blog post do to Windows (assuming it's even possible).

Comment 47

[Christian :Biesinger (don't email me, ping me on IRC)]

To register a protocol handler on linux, just do this (I think gconf-editor can probably do that):

Create /desktop/gnome/url-handlers/foo/enabled (type boolean), set it to true
Create /desktop/gnome/url-handlers/foo/command (type string), set it to /usr/local/bin/foo "%s"

(replace that path with wherever you put the program; replace "foo" with whatever you want to call the protocol)

Comment 48

[Robert Strong (they/them - no direct email)]

Attachment: updated handlers in html format

Comment 49

[Christian :Biesinger (don't email me, ping me on IRC)]

Steps to verify that the patch does what it is supposed to do:

1. Register a protocol handler for the test apps (attachment 273424 [details] etc)
   For Windows, the URL in the URL field has instructions for how to do that
   For Mac, see comment 41
   For Linux, see comment 47, although I haven't tested that
2. Run that protocol handler
   Assuming you named the protocol "foo", enter this in the URL bar:
   foo://"foo" 'bar' `baz`

   The test app should show two arguments; the second one should be:
   foo://%22foo%22%20'bar'%20%60baz%60/

Comment 50

[Daniel Veditz [:dveditz]]

'foo://"foo" blah' doesn't show the original problem in non-fixed builds on Windows -- the first dquote encountered needs to be followed by a space, otherwise it and it's closing dquote are just concatenated as part of the same "argument". Make a page containing the tests in comment 26, too.

Comment 51

[Harry Johnston]

Wouldn't the correct way to respond to a URI with unencoded characters not permitted by STD66 be to inform the user that the link is invalid and refuse to follow it?

Comment 52

[Karl Tomlinson (:karlt)]

(In reply to comment #47)
> To register a protocol handler on linux, just do this (I think gconf-editor
> can probably do that):

I can't work out how to make my gconf-editor do that, but this works:

gconftool-2 --type string --set /desktop/gnome/url-handlers/args/command
 "args \"%s\""
gconftool-2 --type boolean --set /desktop/gnome/url-handlers/args/enabled true

(The extra "" around %s makes no difference.)

Comment 53

[Karl Tomlinson (:karlt)]

Attachment: equivalent test bash script

for Linux, any hardware,
but I doubt Linux argument handling suffered the same problems as the Windows URL protocol handler registration.

Comment 54

[Carsten Book [:Tomcat]]

(In reply to comment #50)
> 'foo://"foo" blah' doesn't show the original problem in non-fixed builds on
> Windows -- the first dquote encountered needs to be followed by a space,
> otherwise it and it's closing dquote are just concatenated as part of the same
> "argument". Make a page containing the tests in comment 26, too.
> 

For the Testcase from comment #26 and #49 i can confirm this bug fixed for Vista and XP and show the expected results - the detailed test results for xp and Vista are on http://wiki.mozilla.org/Firefox:2.0.0.6:Test_Plan:protocol_handler#Windows_Tomcat

Comment 55

[Daniel Veditz [:dveditz]]

(In reply to comment #51)
> Wouldn't the correct way to respond to a URI with unencoded characters not
> permitted by STD66 be to inform the user that the link is invalid and refuse to
> follow it?

space and dquote are perfectly valid characters according to rfc 3986 (std66), encoded or not. Previous specs (rfc 1738) considered them "unsafe" and recommended encoding them, but the most recent leaves it up to the application to figure out when it wants to encode them or not. The spec does make clear that single-quote/apostrophe is a reserved sub-delim and encoded and unencoded forms are NOT equivalent unlike space and dquote.

Comment 56

[Colin Barrett [:cbarrett]]

No, it just prints to standard output. I should have included that in the directions. Open up Console.app (/Applications/Utilities/Console.app), and you'll see it print out the URL that it was passed there.

(In reply to comment #44)
> Colin: Thanks for creating this test program. I am able to run the program
> fine, but I am wondering if it is expected to get a blank window (no error
> message or feedback)? Is it necessary for me to edit the plist file with a URL?
> 
> (In reply to comment #41)
> > Created an attachment (id=273580) [details] [details]
> > equivalent test program for mac os x
> > 
> > I wrote a quick app to act like the main.c test program that Jesper wrote. This
> > one. however, works on Mac OS X.
> > 
> 

Comment 57

[Ben Bucksch (:BenB)]

FWIW, RFC 2394 2.4.3 even called spaces explicitly "disallowed".

Comment 58

[Harry Johnston]

(In reply to comment #55)

> space and dquote are perfectly valid characters according to rfc 3986 (std66),
> encoded or not. 

I've just double checked and they are not permitted (unless encoded).  Are we reading the same document?

http://www.faqs.org/rfc/std/std66.txt

The only characters this permits are (a) the "reserved characters" which schemes may use as delimiters:

gen-delims  = ":" / "/" / "?" / "#" / "[" / "]" / "@"

sub-delims  = "!" / "$" / "&" / "'" / "(" / ")"
                  / "*" / "+" / "," / ";" / "="

(b) the "unreserved characters":

unreserved  = ALPHA / DIGIT / "-" / "." / "_" / "~"

and (c) percent-encoded characters.  Anything else is illegal.

Comment 59

[John Daggett (:jtd)]

Using 2.0.0.5 release source tarball, confirmed that we are indeed relying on the system routine LSOpenCFURLRef to handle escaping the URL passed in.  In the code below, the "url" value is an unescaped url but stepping over the LSOpenCFURLRef with Colin's testapp URL, the escaped version of this URL appears in the console window.  So the system routine is escaping it before passing it along.

// void LaunchURL (in string url);
// Given a url string, call ICLaunchURL using url
// Under OS X use LaunchServices instead of IC
NS_IMETHODIMP nsInternetConfigService::LaunchURL(const char *url)
{
  nsresult rv = NS_ERROR_FAILURE; 

  CFURLRef myURLRef = ::CFURLCreateWithBytes(
                                             kCFAllocatorDefault,
                                             (const UInt8*)url,
                                             strlen(url),
                                             kCFStringEncodingUTF8, NULL);
  if (myURLRef)
  {
    rv = ::LSOpenCFURLRef(myURLRef, NULL);
    ::CFRelease(myURLRef);
  }

  return rv;
}

#0	0x14a1daba in nsInternetConfigService::LaunchURL at nsInternetConfigService.cpp:112
#1	0x14a1c4b7 in nsOSHelperAppService::LoadUriInternal at nsOSHelperAppService.cpp:184
#2	0x14a17111 in nsExternalHelperAppService::handleExternalLoadEvent at nsExternalHelperAppService.cpp:1096
#3	0x01069527 in PL_HandleEvent at plevent.c:688
#4	0x010693d0 in PL_ProcessPendingEvents at plevent.c:623
#5	0x010698f3 in _md_EventReceiverProc at plevent.c:1559
#6	0x9082cf92 in CFRunLoopRunSpecific
#7	0x9082cace in CFRunLoopRunInMode
#8	0x92dec8d8 in RunCurrentEventLoopInMode
#9	0x92debf19 in ReceiveNextEventCommon
#10	0x92e34a74 in _AcquireNextEvent
#11	0x92e348bc in RunApplicationEventLoop
#12	0x2d96cc99 in nsAppShell::Run at nsAppShell.cpp:93
#13	0x2ad8dd36 in nsAppStartup::Run at nsAppStartup.cpp:151
#14	0x0000b144 in XRE_main at nsAppRunner.cpp:2711
#15	0x00002b88 in main at nsBrowserApp.cpp:61

Comment 60

[John Daggett (:jtd)]

Found an interesting article on potential Mac OS X URL handler attacks:

http://www.kernelthread.com/mac/apme/security/url.html

To list URL handlers on the system, type into a Terminal window:

cd /System/Library/Frameworks/ApplicationServices.framework/Frameworks/LaunchServices.framework/Support
./lsregister -dump

To filter out only the list of URL handlers:

./lsregister -dump | grep bindings | egrep -v '[\.]' | awk '{print $2 " " $3 " " $4}' | sort -u | grep ':' 

This outputs the following on my machine:

acrobat:  
addressbook:  
afp:  
aim:  
applescript:  
daap:  
dict:  
feed:  
feed:, feeds:, feedsearch:
file:  
ftp:  
gopher:  
help:  
http:  
http:, https: 
https:  
ical:  
ichat:  
irc:  
itms:, itmss: 
keynote:  
magnet:  
mailto:  
nib:  
pcast:, itpc: 
photo:  
qs:  
qsinstall:  
qss-http:  
qssp-http:  
rtsp:  
see:  
sherlock:  
silc:  
ssh:  
telnet:  
testurl:  
txmt:  
vdownload:  
webcal:  
x-man-page:  
xtorrentsearch:  

Comment 61

[georgi - hopefully not receiving bugspam]

(In reply to comment #53)
> Created an attachment (id=273738) [details]
> equivalant test bash script
> 
> for Linux, any hardware,
> but I doubt Linux argument handling suffered the same problems as the Windows
> URL protocol handler registration.
> 

was linux vulnerable even before the patch?
according to my tests it shows a single argv[1] according to 'cat /proc/PID/cmdline | od -c" ?

Comment 62

[georgi - hopefully not receiving bugspam]

is there working linux exploit involving external protocol handlers?

Comment 63

[Karl Tomlinson (:karlt)]

(In reply to comment #61)
> was linux vulnerable even before the patch?

I don't believe so, as no shell was used to launch the other application, so there was always only one argv.
The issue on Linux is probably just that applications are registered to receive a url, so, if we use them, we should give them a "legal" url.

Comment 64

[JakartaDean]

I'm new around here; please excuse me if this is the wrong place or a previously beaten-to-death issue.  I'm also far from knowledgeable about this stuff; I did search bugzilla a few different ways without success.

I'm wondering about handling of the at-sign "@" included in the query portion of a URI.  One site I like, the site www.cuetable.com used to work fine with a long query, which often included this symbol.  It has stopped when the at-sign is included, from at least 2.0.0.5 but likely earlier.  My reading of RFC3986 is that the at-sign is allowed:

query       = *( pchar / "/" / "?" ) ; and
pchar         = unreserved / pct-encoded / sub-delims / ":" / "@"

yet Firefox now percent-encodes the at-sign, which has broken the argument handler of the above Shockwave application.  Am I wrong, or has someone perhaps overzealously limited the use of reserved characters?

Comment 65

[Harry Johnston]

(In reply to comment #64)

> I'm wondering about handling of the at-sign "@" included in the query portion
> of a URI.  One site I like, the site www.cuetable.com used to work fine with a
> long query, which often included this symbol.  It has stopped when the at-sign
> is included, from at least 2.0.0.5 but likely earlier.

Works for me on 2.0.0.5.  Do you have any plugins or extensions installed that might be responsible?  Or perhaps a proxy server?

Comment 66

[JakartaDean]

I've got the following extensions (time for some house cleaning, I see) none of which look like they could be responsible:
All-In-One Sidebar
ColorfulTabs
CustomizeGoogle
Duplicate Tab
infoRSS
NoScript
OpenDownload
Tab Clicking Options
Tabbrowser Preferences

To clarify, if I click on the following link:
http://cuetable.com/P/?@4AdOx3BBKy3CYjd4DHgd4EEIq4FWFl1GHeJ2HdYA1PWEn4QVWl4RTll3WaAd3Wape3WYSc4lVWl3lbRS3lPpp4lbXc4lVxu4lbNq1lWMk1lWMj4mTll3mbEG3mbUH@

Firefox tries to open the following:
http://cuetable.com/P/?%404AdOx3BBKy3CYjd4DHgd4EEIq4FWFl1GHeJ2HdYA1PWEn4QVWl4RTll3WaAd3Wape3WYSc4lVWl3lbRS3lPpp4lbXc4lVxu4lbNq1lWMk1lWMj4mTll3mbEG3mbUH%40

i.e. the at-signs have been converted to %40, which breaks the format the query expects.

Comment 67

[Harry Johnston]

(In reply to comment #66)

> I've got the following extensions (time for some house cleaning, I see) 

Please try disabling them, restarting Firefox and repeating the test.

Try the following (much simpler) link which works for me:

http://www.scms.waikato.ac.nz/~harry/[e-mail]

Comment 68

[JakartaDean]

It was NoScript.  It aggressively blocks XSS stuff, either disabling NoScript or enabling it but disabling the Anti-XSS option clears the problem up.  I didn't even know it was there.  Thanks very much for your help Harry.

BTW, your link worked fine, even with all add-ons enabled including the Anti-XSS feature of NoScript.

Regards,
Dean

Comment 69

[Harry Johnston]

(In reply to comment #68)

> BTW, your link worked fine, even with all add-ons enabled including the
> Anti-XSS feature of NoScript.

Yep, my mistake - of course a regular file will work either way because the web server will decode the URL as necessary.  Glad the problem is resolved.

Comment 70

[Stephen Donner [:stephend] Not actively reading bugmail]

I ran Rob's NSIS installer script to register some protocols listed on https://bugzilla.mozilla.org/attachment.cgi?id=274255, as well as the testurl.exe command-line shell, to verify that the first URI in the bugzilla attachment triggers the testurl.exe to run, but not the 2nd and 3rd ones, which usually have "%00" or "%[invalid char]".  Verifying this as fixed, then, using a Thunderbird version 1.5.0.13 (20070809) (release candidate) build.

Replacing fixed1.8.0.13 with verified1.8.0.13.

Comment 71

[Stephen Donner [:stephend] Not actively reading bugmail]

Sorry, the above is actually meant for bug 389580; reverting verified1.8.0.13 back to fixed1.8.0.13; I'll come back and list the steps I used to verify this bug.  Sorry about that...

Comment 72

[Stephen Donner [:stephend] Not actively reading bugmail]

Using Thunderbird 1.5.0.13 release candidate 1 (build ID: version 1.5.0.13 (20070809)) on Windows XP, I tested:

1) Making HTML testcases out of the three testcase snippets from dveditz in comment 26.  I then:
2) Sent myself three emails, each with its own "foo 1", "foo 2", and "foo 3" testcases.
3) Next, I read each email and clicked on the <a href> in the testcase, which in all cases launched the "External Protocol Request", each of whose strings I compared, making sure that the back-tick (`) and the double-quotes (") were escaped by %60 and %22, respectively.

From my talks with biesi and juanb (and reading the bug), I think this is enough to verify that we're doing the right thing.  This VM image has the setup from comment 70, but the steps to verify this particular bug are different, of course.

Replacing fixed1.8.0.13 keyword with verified1.8.0.13.

Comment 73

[Stephen Donner [:stephend] Not actively reading bugmail]

Oh, and I forgot to mention that the single quote in the second testcase wasn't escaped (seems that's intended, from comment 12 and comment 13).

Comment 74

[Giorgio Maone [:ma1]]

Looks like fixing this bug made some difference, after all:
http://www.gnucitizen.org/blog/ie-pwns-secondlife