622981 - UTF-8 URLs ending with ? fail to load when a relative URL tries to change the query

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[viriketo]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; ca-ES; rv:1.9.2.13) Gecko/20101220 Firefox/3.6.3
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; ca-ES; rv:1.9.2.13) Gecko/20101220 Firefox/3.6.13

There appears to be a bad string manipulation regarding the URL, if the URL contains utf-8 characters, and a link in the webpage simply starts with a "?" (to only alter the query string, but not the URL path)

Reproducible: Always

Steps to Reproduce:


Let's say I have the browser looking at "http://localhost:8090/distribuïnt?"  (notice the unicode ï).

If the page has a link like: <a href="?local_images=0">[Use remote images]</a>
firefox renders the destination URL as: http://localhost:8090/distribu%C3%?local_images=0
When it should be: http://localhost:8090/distribu%C3%AFnt?local_images=0

It looks like the ?... is appended at the number of bytes determined by the number of utf-8 characters of the URL, instead of the number of bytes of the encoded URL.

Maybe this bad handling of strings can potentially end in an exploitable buffer overflow, additional to the inconvenience of failing "?blabla" links.
Actual Results:  
http://localhost:8090/distribu%C3%?local_images=0

Expected Results:  
http://localhost:8090/distribu%C3%AFnt?local_images=0
or showing directly:
http://localhost:8090/distribuïnt?local_images=0

Comment 1

[u279076]

Would you be able to set up a test site as a proof of concept?

Comment 2

[viriketo]

Here you have a URL. The ? at the end seems to trigger the problem, as without it, all works.
http://vicerveza.homeunix.net/~viric/tmp/xènia?

Comment 3

[u279076]

Going to http://vicerveza.homeunix.net/~viric/tmp/xènia and clicking TEST seems to work for me.

Going to http://vicerveza.homeunix.net/~viric/tmp/xènia? and clicking TEST gives me an Error 400 webserver error.

The same test under Safari results in no error.  I'm not sure what the issue is, but marking as NEW.

Comment 4

[viriketo]

After clicking the link to "xènia?" check the URL firefox goes to. The webserver error comes from the wrong URL firefox used for the link.

Comment 5

[viriketo]

Sorry, I meant that the case you should loook at is that giving 'webserver error' to you, due to the wrong url. As you changed the subject of the bug report, I imagine you understood all well.

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: xpcshell testcase showing that URI resolution against a UTF-8-based based URI is wrong

Navigating the link via clicking (e.g. by clicking on the link in this bug's "URL" field) and then clicking the "Test" link on the resultant page works correctly. But, copy+pasting the URL and then clicking the "Test" link on the resultant page gives the described erroneous behavior. The behavior is the same in 3.5.16 so this is not a regression and thus shouldn't be a blocker.

The attached xpcshell testcase demonstrates the behavior.

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: xpcshell testcase showing that URI resolution against a UTF-8-based based URI is wrong

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Taking bug

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

The cause is ::ParsePath() finding the query in "native" form (i.e. with the characters as single bytes), and then the actual spec being stored in encoded (%XX) form.  This causes the offsets for the query (and probably param or ref) to be wrong.  I believe there's no chance of buffer overflow, since it will always indicate a position and length (when it's wrong) that's too early/short, not too late/long.

Looking at the best way to reorder things to fix this.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch to fix this bug and greatly expand the test coverage of test_URIs.js

The test coverage is largely based on the RFC 3986 examples.  Some tests don't work easily in the testing framework and have been disabled for now.  The tests should hit all the major cases and many edge cases.

One very minor bug was caught by this; the test for that is disabled and I'll submit something for that later.

The bug 622981 test (two simple relative URLs) could be spun into a separate bug file instead of mixing it with all the basic functionality tests.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Make the title reflect the actual bug

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 545042 [details] [diff] [review]
Patch to fix this bug and greatly expand the test coverage of test_URIs.js

r=me.  I really wish the invariants for this code were better documented.... :(

Thanks for the patch!

If you want someone else to push this, please don't forget to post a patch with a From or User line and a commit message.

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Checked in: http://hg.mozilla.org/mozilla-central/rev/9fe4289527fc