Bug (CVE-2012-3696) – window.location.href and others needlessly decodes URI-encoded characters

• Component: DOM
• Status: RESOLVED FIXED

[Andrei Popescu]

Navigate to a URL that contains URI escape sequences in its path, e.g. http://example.com/foo%3dbar and query window.location.href. This property should return the URI unmodified in any way. What actually happens is that the property contains the URI with its path component being URI-decoded, i.e. http://example.com/foo=bar. 

IE, Mozilla FF, Chrome all return the path unmodified. Safari 4.x and the Android browser decode the path. This problem was actually meant to be addressed after the patch in the bug below landed:

https://bugs.webkit.org/show_bug.cgi?id=23546

but it never happened.

Also note the following bug filed against Android:

http://code.google.com/p/android/issues/detail?id=3861

Uploading patch soon.

[Andrei Popescu]

Created attachment 40895 [details]
Make KURL::prettyUrl (used by window.location.href) not URI-decode the URI path.

[Andrei Popescu]

Created attachment 40896 [details]
Make KURL::prettyUrl (used by window.location.href) not URI-decode the URI path.

Forgot to mark the previous upload as 'patch'.

[Eric Seidel (no email)]

Comment on attachment 40896 [details]
Make KURL::prettyUrl (used by window.location.href) not URI-decode the URI path.

You ChangeLog is duplicated.

I don't really like pathRaw(), maybe encodedPath()?  Or should path be decodedPath()?

fast/dom/Window/Location/location-path-has-is-unmodified.htm is sorta a js test, except you didn't generate the tempate using make-script-test-wrappers.

Why does this need to be run from onload?  It needs to wait for the iframe to load?  Will body onload do that?

(If this needs to wait for onload, then it won't work quite right as a js test, so maybe that's why you ddid it this way.)

+        correctValue = "path%2Dwith-escape%2Dcharacters.html";
+        shouldBe("result", "correctValue");

shouldBeEqualToString("normalizeURL(String(window.frames[0].location.href))", "path%2Dwith-escape%2Dcharacters.html");
would have been better.

Does this actually need to load a valid resource to work?

[Darin Adler]

This should not change the behavior of prettyUrl. You can add a new function with the desired behavior and possibly remove prettyUrl if no one uses it. It doesn't make sense to have a “pretty” URL function that does not decode the path!

You should look at what window.location.href needs and think about what the right name is for it.

[Andrei Popescu]

(In reply to comment #3)
> (From update of attachment 40896 [details])
> You ChangeLog is duplicated.
> 
> I don't really like pathRaw(), maybe encodedPath()?  Or should path be
> decodedPath()?
> 

Actually, I think that using the conversion to String operator is enough. This returns the string that was built in the KURL::parse() method.

> fast/dom/Window/Location/location-path-has-is-unmodified.htm is sorta a js
> test, except you didn't generate the tempate using make-script-test-wrappers.
> 
> Why does this need to be run from onload?  It needs to wait for the iframe to
> load?  Will body onload do that?
> 

Yes.

> (If this needs to wait for onload, then it won't work quite right as a js test,
> so maybe that's why you ddid it this way.)
>

That's right. In fact, I just used the pattern I found in the fast/dom/HTMLObjectElement/object-as-frame.html test that I had to modify. Note that that test also uses an onload handler to test the window.location.href property of an iframe.


 
> +        correctValue = "path%2Dwith-escape%2Dcharacters.html";
> +        shouldBe("result", "correctValue");
> 
> shouldBeEqualToString("normalizeURL(String(window.frames[0].location.href))",
> "path%2Dwith-escape%2Dcharacters.html");
> would have been better.
> 

Done.

> Does this actually need to load a valid resource to work?

Actually it doesn't if I use a data URL. Done.

[Andrei Popescu]

Hi Darin,

(In reply to comment #4)
> This should not change the behavior of prettyUrl. You can add a new function
> with the desired behavior and possibly remove prettyUrl if no one uses it. It
> doesn't make sense to have a “pretty” URL function that does not decode the
> path!
> 


You're right, it would not be "pretty" anymore. prettyURL() is used by the Console so I cannot remove it.

> You should look at what window.location.href needs and think about what the
> right name is for it.

I looked and, if I am right, then the String conversion operator of KURL should be enough, so I do not need to add anything new. This operator calls KURL::string(), which just returns the string that was the output of the parse method. So simply removing the call to prettyURL in Location.cpp seems to do the job and all the layout tests pass.

Thanks,
Andrei

[Darin Adler]

(In reply to comment #6)
> I looked and, if I am right, then the String conversion operator of KURL should
> be enough, so I do not need to add anything new. This operator calls
> KURL::string(), which just returns the string that was the output of the parse
> method. So simply removing the call to prettyURL in Location.cpp seems to do
> the job and all the layout tests pass.

Sounds good.

The way to prove this is with tests. Think about cases where you think this might fail, and write a new test or tests that covers as many cases as possible. At least one test for each distinct thing prettyURL used to do, and also tests based on guesses you might have about what could go wrong. And then make sure you run the tests in other browsers too.

[Andrei Popescu]

(In reply to comment #7)
> (In reply to comment #6)
> > I looked and, if I am right, then the String conversion operator of KURL should
> > be enough, so I do not need to add anything new. This operator calls
> > KURL::string(), which just returns the string that was the output of the parse
> > method. So simply removing the call to prettyURL in Location.cpp seems to do
> > the job and all the layout tests pass.
> 
> Sounds good.
> 
> The way to prove this is with tests. Think about cases where you think this
> might fail, and write a new test or tests that covers as many cases as
> possible. At least one test for each distinct thing prettyURL used to do, and
> also tests based on guesses you might have about what could go wrong. And then
> make sure you run the tests in other browsers too.

I was just about to upload a new patch but I will hold off for now and add more tests. Thanks for the comments!

Andrei

[Thomas Steinacher]

Please fix this bug as it makes it impossible to distinguish URLs like /%25 and /%2525 in a cross-browser manner.

[Darin Adler]

Adam, have you looked into this issue while planning your URL work?

[Adam Barth]

(In reply to comment #10)
> Adam, have you looked into this issue while planning your URL work?

I haven't.  I've been studying the behavior of <a href>.  In general, it makes sense to canonicalize URLs before returning them to web content.  For example, returning % unescaped is clearly problematic because % is a control character in URLs.

[Erik Arvidsson]

For what it is worth HTMLAnchorElement href and other URL DOM bindings use KURL::string which seems to do the right thing. Changing Location::href to use KURL::string as well seems to solve this particular bug without having to update KURL.

Chiming in with another request to please fix this issue -- it has the side effect of effectively making the new pushState() API unusable in Safari. If your URL happens to have encoded data in it, like say, the name of a user-entered field -- it's impossible to distinguish their "/" in the title from a true "/', because the auto-decoding makes them appear the same to window.location.pathname.

[Adam Barth]

Thanks.  I'll look at this in connection with the prettyURL audit.

[Adam Barth]

My investigation in Bug 61201 seems to indicate that all callers of prettyURL are wrong.  I'm going to wrap that up first and then continue on to this bug.

Many thanks, Adam. -- I very much appreciate you taking a look at this issue.

[Erik Arvidsson]

Taking... My intention is to make Safari match the other browsers here

[Erik Arvidsson]

Created attachment 104257 [details]
Patch

[Erik Arvidsson]

This makes Safari a whole lot more compatible with IE, Firefox and Opera which all return the encoded form for these things.

Sorry about the incomplete ChangeLog files, I forgot that webkit-patch does not pick up changes to these things.

[Adam Barth]

Comment on attachment 104257 [details]
Patch

This patch is great.

[Adam Barth]

 > Sorry about the incomplete ChangeLog files, I forgot that webkit-patch does not pick up changes to these things.

(I should also say that I'm assuming you wrote a nice ChangeLog entry that you'll include when landing the patch.)

[Darin Adler]

Comment on attachment 104257 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104257&action=review

> Source/WebCore/page/Location.cpp:68
> -    const KURL& url = this->url();
> -    // FIXME: Stop using deprecatedString(): https://bugs.webkit.org/show_bug.cgi?id=30225
> -    return url.hasPath() ? url.deprecatedString() : url.deprecatedString() + "/";
> +    return this->url().string();

I don’t see any coverage of "http://foo.com", which is a case that I’d expect to be affected by the hasPath check. Is that covered and I just missed it?

The old code also might do things like omit a "@" or a ":" where the new code would keep it. Is that covered?

> Source/WebCore/platform/KURL.cpp:604
> -    return decodeURLEscapeSequences(m_string.substring(start, m_hostEnd - start));
> +    return m_string.substring(start, m_hostEnd - start);

This change seems entirely separate from the Location.href/toString one. Is there sufficient test coverage? I don’t see tests where hostnames are seen URL-encoded above.

> Source/WebCore/platform/KURL.cpp:729
> -    return decodeURLEscapeSequences(m_string.substring(m_portEnd, m_pathEnd - m_portEnd)); 
> +    return m_string.substring(m_portEnd, m_pathEnd - m_portEnd); 

Are you sure that all callers of path want the URL-escaped form? For example, if I was going to take a file URL’s path, encode as UTF-8 and use it to access a file, I would not want it URL-escaped. Is there any code that does that?

[WebKit Review Bot]

Comment on attachment 104257 [details]
Patch

Attachment 104257 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9419246

New failing tests:
fast/dom/javascript-backslash.html
fast/dom/HTMLObjectElement/object-as-frame.html

[Darin Adler]

Comment on attachment 104257 [details]
Patch

Looks like this will break DOMFileSystemBase::crackFileSystemURL

[Darin Adler]

Fixing the location object seems fine, but the KURL changes are likely to have effects elsewhere. Please thoroughly investigate this before landing the change.

[Darin Adler]

Looks like this will also break createGlobalHDropContent in ClipboardWin.cpp

[Darin Adler]

Looks like this breaks KURL::fileSystemPath in KURLQt.cpp

[Darin Adler]

This might break FrameLoader::defaultObjectContentType when the path extension has characters that are URL-encoded in it.

[Adam Barth]

Comment on attachment 104257 [details]
Patch

Darin's comment make me less enthusiastic about this patch.  :)

[Adam Barth]

Comment on attachment 104257 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104257&action=review

>> Source/WebCore/page/Location.cpp:68
>> +    return this->url().string();
> 
> I don’t see any coverage of "http://foo.com", which is a case that I’d expect to be affected by the hasPath check. Is that covered and I just missed it?
> 
> The old code also might do things like omit a "@" or a ":" where the new code would keep it. Is that covered?

http://trac.webkit.org/browser/trunk/LayoutTests/fast/url/script-tests/standard-url.js is a good place to add this test.  It is covered there, but in an oblique way.  A direct test would be better.

[Erik Arvidsson]

Thanks Darin. I did expect this to need more work.

I'll look into these issues.

Another option is to introduce KURL::encodedPath() and KURL::encodedHost()

[Erik Arvidsson]

Darin, for the record Chromium has used encoded host() and path() for KURL since 1.0 without any currently known issues so I'm hoping I can get this to be the case for KURL in non Chromium code as well.

[Adam Barth]

ClipboardWin.cpp and KURLQt.cpp aren't used by Chromium, so it's important to make sure we don't break these.  I'm less worried about FrameLoader::defaultObjectContentType, although it would be nice to have a test.

DOMFileSystemBase::crackFileSystemURL is used by Chromium, so if this change breaks that function, there must be something going on that I don't fully understand.

[Darin Adler]

(In reply to comment #33)
> DOMFileSystemBase::crackFileSystemURL is used by Chromium, so if this change breaks that function, there must be something going on that I don't fully understand.

I believe this patch changes the behavior of that function so that file extensions that are percent-escaped are be handled differently from the same file extensions when not percent-escaped. I don’t know whether file extensions that are percent-escaped are legal or not.

So, for example if the extension was ".%6Apeg" instead of ".jpeg".

[Eric U.]

(In reply to comment #34)
> (In reply to comment #33)
> > DOMFileSystemBase::crackFileSystemURL is used by Chromium, so if this change breaks that function, there must be something going on that I don't fully understand.
> 
> I believe this patch changes the behavior of that function so that file extensions that are percent-escaped are be handled differently from the same file extensions when not percent-escaped. I don’t know whether file extensions that are percent-escaped are legal or not.
> 
> So, for example if the extension was ".%6Apeg" instead of ".jpeg".

The filesystem should support files named .%6Apeg and .jpeg, and should see them as distinct.  It currently has some bugs in the area, which I'm working on with https://bugs.webkit.org/show_bug.cgi?id=62811.

If you call toURL() on a FileEntry called ".%6Apeg", you should get "filesystem:http://your.origin.com/temporary/.%256Apeg".

[Darin Adler]

My comment is not about whether the old or new behavior is correct. My comment is about the fact that this patch changes behavior, and is not fixing a bug with a test case.

I think a patch targeted at window.location should fix specifically window.location. Fixing the underlying function without testing all the call sites is unnecessarily dangerous.

[Adam Barth]

Yes, definitely.  I asked EricU to look at this bug to help us understand what the correct behavior is and to see how it related to the other bug he's working on.

[Darin Adler]

(In reply to comment #37)
> I asked EricU to look at this bug to help us understand what the correct behavior is and to see how it related to the other bug he's working on.

Thanks EricU. I appreciate having the information!

[Erik Arvidsson]

Created attachment 108568 [details]
Patch

[Erik Arvidsson]

Sorry for not getting back to this sooner.

I ended up decoding the path in the places where it was expecting decoded values like before.

I only tested this on Safari/Mac so I expect some Chromium failures. I'll look at the bots when they come in.

[WebKit Review Bot]

Comment on attachment 108568 [details]
Patch

Attachment 108568 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9842119

New failing tests:
fast/loader/subframe-navigate-during-main-frame-load.html
fast/history/history-back-initial-vs-final-url.html
fast/url/standard-url.html

[Erik Arvidsson]

Created attachment 109710 [details]
Patch

[Erik Arvidsson]

(In reply to comment #42)
> Created an attachment (id=109710) [details]
> Patch

There are still KURL issues that this patch does not fix but (fragments encodes non ascii characters for example) but it does fix the path issue and cleans up the non hierarchical URLs too.

[WebKit Review Bot]

Comment on attachment 109710 [details]
Patch

Attachment 109710 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9958092

New failing tests:
fast/url/standard-url.html

[Erik Arvidsson]

Adam, the bot output does not include "standard-url" and it passing locally (built and tested with --chromium). Any advice?

[Adam Barth]

You're probably testing on a Mac.  I suspect there are different expectations for standard URL on Windows and then Linux needs to further override the windows expectations.

[Adam Barth]

Notice that the fallback path for chromium-linux goes through chromium-win:

https://docs.google.com/drawings/d/1z65SKkWrD4Slm6jobIphHwwRADyUtjOAxwGBVKBY8Kc/edit?hl=en_US

[Darin Adler]

Comment on attachment 109710 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109710&action=review

> LayoutTests/fast/dom/anchor-origin-expected.txt:12
> +data:text/html,foo => null

This shows the test is now broken. The URL has <b> tags in it, but now they disappear. We should fix the test so the <b> tags don’t disappear. I understand they should not be %-escaped, but it’s bad to leave this test in a broken state.

> LayoutTests/fast/url/script-tests/file-http-base.js:39
> -    ["file:///C:/asdf#\\xc2", "file:///C:/asdf#\\xef\\xbf\\xbd"],
> +    ["file:///C:/asdf#\xc2", "file:///C:/asdf#\xc2"],

I don’t understand why you changed what’s being tested here. Changing the result is one thing, but you changed the test input URL as well. Don’t we still want to test that case? Maybe we should test both?

> LayoutTests/fast/url/script-tests/standard-url.js:64
> +  ["javascript:alert(\\t 1 \\n\\r)", "javascript:alert( 1 )"],

Is this really an expected, good test result? Why are the spaces being collapsed here?

> LayoutTests/fast/url/segments.html:4
> -<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
> +<!DOCTYPE html>
>  <html>
>  <head>
> +<meta charset="utf-8">

Why this change? Change log doesn’t mention it.

> Source/WebCore/fileapi/DOMFileSystemBase.cpp:46
> +#include "KURL.h"

This file already uses KURL. No need to add a new include.

> Source/WebCore/loader/FrameLoader.cpp:75
> +#include "KURL.h"

This file already uses KURL. No need to add a new include.

> Source/WebCore/page/Location.cpp:154
>  String Location::toString() const
>  {
> -    if (!m_frame)
> -        return String();
> -
> -    const KURL& url = this->url();
> -    // FIXME: Stop using deprecatedString(): https://bugs.webkit.org/show_bug.cgi?id=30225
> -    return url.hasPath() ? url.deprecatedString() : url.deprecatedString() + "/";
> +    return href();
>  }

If this is just turning around and calling another function, then lets inline this in the header.

> Source/WebCore/platform/win/ClipboardWin.cpp:43
> +#include "KURL.h"

No need to add this include. The file already used KURL member functions so it must already have been including this.

> Source/WebCore/workers/WorkerLocation.cpp:80
>  String WorkerLocation::toString() const
>  {
> -    // FIXME: Stop using deprecatedString(): https://bugs.webkit.org/show_bug.cgi?id=30225
> -    return m_url.hasPath() ? m_url.deprecatedString() : m_url.deprecatedString() + "/";
> +    return href();
>  }

Same here.

[Erik Arvidsson]

Comment on attachment 109710 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109710&action=review

>> LayoutTests/fast/dom/anchor-origin-expected.txt:12
>> +data:text/html,foo => null
> 
> This shows the test is now broken. The URL has <b> tags in it, but now they disappear. We should fix the test so the <b> tags don’t disappear. I understand they should not be %-escaped, but it’s bad to leave this test in a broken state.

Fixed.

>> LayoutTests/fast/url/script-tests/file-http-base.js:39
>> +    ["file:///C:/asdf#\xc2", "file:///C:/asdf#\xc2"],
> 
> I don’t understand why you changed what’s being tested here. Changing the result is one thing, but you changed the test input URL as well. Don’t we still want to test that case? Maybe we should test both?

Fixed. I added a new test line here too.

>> LayoutTests/fast/url/script-tests/standard-url.js:64
>> +  ["javascript:alert(\\t 1 \\n\\r)", "javascript:alert( 1 )"],
> 
> Is this really an expected, good test result? Why are the spaces being collapsed here?

This is how all other browsers behave.

>> LayoutTests/fast/url/segments.html:4
>> +<meta charset="utf-8">
> 
> Why this change? Change log doesn’t mention it.

The test script is now UTF-8. I added a comment to the ChangeLog.

>> Source/WebCore/page/Location.cpp:154
>>  }
> 
> If this is just turning around and calling another function, then lets inline this in the header.

Fixed.

[Erik Arvidsson]

Created attachment 109887 [details]
Patch

[Darin Adler]

Comment on attachment 109887 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109887&action=review

> LayoutTests/ChangeLog:23
> +        * fast/url/segments.html: Use UT-8

Typo

[Erik Arvidsson]

Created attachment 109895 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 109895 [details]
Patch for landing

Clearing flags on attachment: 109895

Committed r96779: <http://trac.webkit.org/changeset/96779>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

It seems like this patch caused fast/history/history-back-initial-vs-final-url.html to fail on Qt:
http://build.webkit.org/results/Qt%20Linux%20Release/r96779%20(38208)/results.html

[Erik Arvidsson]

(In reply to comment #55)
> It seems like this patch caused fast/history/history-back-initial-vs-final-url.html to fail on Qt:
> http://build.webkit.org/results/Qt%20Linux%20Release/r96779%20(38208)/results.html

http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/qt/DumpRenderTreeQt.cpp#L834

Needs to be changed to

QString url = item.url().toString();

since the extra encoding is no longer needed to match KURL.

I can make this change tomorrow when I get in to the office.

[Csaba Osztrogonác]

Fix landed in http://trac.webkit.org/changeset/96789.
But unfortunately there is one more bug: https://bugs.webkit.org/show_bug.cgi?id=69511

[Erik Arvidsson]

*** Bug 22899 has been marked as a duplicate of this bug. ***

[Darin Adler]

*** Bug 76320 has been marked as a duplicate of this bug. ***

[Randy Hudson]

Has this fix made its way into Safari 5.1.5?

Using that version of Safari, if I go to this page:
http://limpet.net/mbrubeck/temp/chromium-location-test.html/foo%20bar?foo%20bar

The non-query part of the location is still being decoded.

[Adam Barth]

*** Bug 83485 has been marked as a duplicate of this bug. ***

[Brent Fulgham]

*** Bug 66836 has been marked as a duplicate of this bug. ***