Bug 389106 (CVE-2007-3845)

Escape URIs (especially quotes) when passing them to external protocol handlers

RESOLVED FIXED

Status

()

Core
Security
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: mconnor, Assigned: Biesinger)

Tracking

(Depends on: 2 bugs, {fixed1.8.1.6, fixed1.8.1.8, verified1.8.0.13})

unspecified
fixed1.8.1.6, fixed1.8.1.8, verified1.8.0.13
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.6 +
blocking1.8.1.8 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical], URL)

Attachments

(8 attachments, 4 obsolete attachments)

1.47 KB, patch
dveditz
: superreview+
Details | Diff | Splinter Review
1.69 KB, patch
Details | Diff | Splinter Review
40.00 KB, application/octet-stream
Details
1.18 KB, patch
dveditz
: superreview+
Details | Diff | Splinter Review
1005 bytes, patch
dveditz
: superreview+
Details | Diff | Splinter Review
46.57 KB, application/octet-stream
Details
48.85 KB, text/html
Details
106 bytes, text/x-sh
Details
(Reporter)

Description

10 years ago
different takes on this from different people, biesi has a patch for nsOSHelperAppService.cpp
Flags: blocking1.8.1.6+
Created attachment 273260 [details] [diff] [review]
patch
Attachment #273260 - Flags: superreview?(dveditz)
Attachment #273260 - Flags: review?
Attachment #273260 - Flags: review? → review?(bzbarsky)
I should note... this patch patches windows only and it doesn't affect the URL in the dialog, just what's executed.
Is there an XP location we could patch?
Comment on attachment 273260 [details] [diff] [review]
patch

This does look ok for now, though...
Attachment #273260 - Flags: review?(bzbarsky) → review+
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 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...
Created attachment 273284 [details] [diff] [review]
alternative patch

something like this then?
Attachment #273284 - Flags: superreview?(dveditz)
Attachment #273284 - Flags: review?(bzbarsky)
Created attachment 273285 [details] [diff] [review]
alternative patch (branch version)

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

Comment 9

10 years ago
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);
On at least some platforms nsProcessCommon just pastes the args together into a string.  Or at least it used to,,,,
Comment on attachment 273284 [details] [diff] [review]
alternative patch

Indeed.
Attachment #273284 - Flags: review?(bzbarsky) → review+
(Reporter)

Updated

10 years ago
Flags: blocking1.8.1.6+
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."
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?
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 on attachment 273260 [details] [diff] [review]
patch

sr=dveditz without the %27 line
Attachment #273260 - Flags: superreview?(dveditz) → superreview+
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.
Attachment #273284 - Flags: superreview?(dveditz) → superreview+

Comment 17

10 years ago
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.
spaces are already escaped.
(Reporter)

Comment 19

10 years ago
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
Attachment #273285 - Flags: approval1.8.1.7+
Attachment #273285 - Flags: approval1.8.1.6+
Attachment #273285 - Flags: approval1.8.0.13+
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
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Adding tracking keywords fixed1.8.0.13, fixed1.8.1.6, fixed1.8.1.7 per above
Keywords: fixed1.8.0.13, fixed1.8.1.6, fixed1.8.1.7
QA could use some help identifying which areas we should examine more closely for possible regressions. Thanks.
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
Hm, actually, let me verify that statement about spaces...
OK, I'm not sure now what made me think that we escaped spaces, but we don't
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 273424 [details]
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.
Created attachment 273434 [details] [diff] [review]
escape spaces

Escape spaces. I'll have to test javascript: and data: URLs too once this build finishes...
Attachment #273260 - Attachment is obsolete: true
Attachment #273434 - Flags: superreview?(dveditz)
Attachment #273434 - Flags: review?(bzbarsky)
ok, spaces in javascript: and data: seem to work fine even with the patch.
Comment on attachment 273434 [details] [diff] [review]
escape spaces

r=bzbarsky
Attachment #273434 - Flags: review?(bzbarsky) → review+
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

Updated

10 years ago
Group: security
Created attachment 273501 [details] [diff] [review]
escape spaces, branch version

this one just adds a ReplaceSubstring call, should be safer than touching NS_EscapeURL
Attachment #273501 - Flags: superreview?(dveditz)
Attachment #273501 - Flags: review?(bzbarsky)
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
Attachment #273501 - Flags: superreview?(dveditz)
Attachment #273501 - Flags: superreview+
Attachment #273501 - Flags: approval1.8.1.6?
Attachment #273501 - Flags: approval1.8.0.13?
Keywords: fixed1.8.0.13, fixed1.8.1.6, fixed1.8.1.7
Comment on attachment 273434 [details] [diff] [review]
escape spaces

sr=dveditz for trunk and possibly 1.8 branch.
Attachment #273434 - Flags: superreview?(dveditz)
Attachment #273434 - Flags: superreview+
Attachment #273434 - Flags: approval1.8.1.7?
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
Attachment #273501 - Flags: review?(bzbarsky) → review+
(Reporter)

Comment 35

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

please land on all of the same branches as the previous patch
Attachment #273501 - Flags: approval1.8.1.6?
Attachment #273501 - Flags: approval1.8.1.6+
Attachment #273501 - Flags: approval1.8.0.13?
Attachment #273501 - Flags: approval1.8.0.13+
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
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
Fixes checked into branches, adding fix-tracking keywords
Keywords: fixed1.8.0.13, fixed1.8.1.6, fixed1.8.1.7
Created attachment 273528 [details]
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 on attachment 273528 [details]
Handlers in html format

Actually, a lot slipped through but it is quite a bit cleaner than it was. ;)
Created attachment 273560 [details]
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.
Attachment #273528 - Attachment is obsolete: true
Created attachment 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.

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.
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.  ;)
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.
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.
> 
Created attachment 273645 [details]
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

10 years ago
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).
Attachment #273645 - Attachment is obsolete: true
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)
Created attachment 273656 [details]
 updated handlers in html format
Attachment #273560 - Attachment is obsolete: true
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/
Depends on: 389469
'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

10 years ago
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?
(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.)
Created attachment 273738 [details]
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.
Attachment #273738 - Attachment description: equivalant test bash script → equivalent test bash script
Attachment #273738 - Attachment mime type: application/x-sh → text/x-sh
(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
(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.
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

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

Comment 58

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

10 years ago
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

10 years ago
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:  


(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" ?
is there working linux exploit involving external protocol handlers?
(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.

Updated

10 years ago
Depends on: 390126
Alias: CVE-2007-3845

Comment 64

10 years ago
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

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

10 years ago
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

10 years ago
(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/test@hi.html

Comment 68

10 years ago
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

10 years ago
(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.
Whiteboard: [sg:critical]

Updated

10 years ago
Summary: firefox may not escape quotes everywhere → Escape URIs (especially quotes) when passing them to external protocol handlers

Updated

10 years ago
Depends on: 392240
Flags: in-testsuite?
Attachment #273434 - Flags: approval1.8.1.7?
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.
Keywords: fixed1.8.0.13 → verified1.8.0.13
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...
Keywords: verified1.8.0.13 → fixed1.8.0.13
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.
Keywords: fixed1.8.0.13 → verified1.8.0.13
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

10 years ago
Looks like fixing this bug made some difference, after all:
http://www.gnucitizen.org/blog/ie-pwns-secondlife
Depends on: 422609
You need to log in before you can comment on or make changes to this bug.