377049 - nsBaseURLParser::ParsePath doesn't handle strings that are not NULL terminated

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Brett Wilson]

The URL parsing code is written such that the input may or may not be NULL terminated. However, the first for loop in nsBaseURLParser::ParsePath uses a NULL for the termination condition, which will cause us to read random memory if the input isn't NULL terminated.

In practice, this code seems to be only be run over the output of PromiseFlatCString which guarantees a NULL terminator. However, this may be a problem in the future, especially since the parsers go pretty far out of their way to allow non-NULL terminated input.

Comment 1

[Ryan Jones-Ward [:sciguyryan]]

One simple fix here would be to create a new |PromiseFlatCString| string from the character string passes as the argument then re-convert this back too a |const char*| array and use that thus ensuring we always end with a string but that seems inefficient.

I'm not sure if the above method is viable though.

Comment 2

[Ryan Jones-Ward [:sciguyryan]]

Attachment: patch v1.0

Patch v1.0

This is probably nicer than the method above because we don't need to duplicate code there and we simply check against the |pathLen| parameter instead.

Comment 3

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

Comment on attachment 261465 [details] [diff] [review]
patch v1.0

p != path + pathLen. did you test this patch?

Comment 4

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1.1

Patch v1.1

Oops!

Comment 5

[Nickolay_Ponomarev]

mozilla/netwerk/base/src/nsURLParsers.cpp  1.27

Comment 6

[Nickolay_Ponomarev]

Please set in-testsuite to either "-" or "?", depending on whether this needs a regression test.

Comment 7

[Nickolay_Ponomarev]

Comment on attachment 261484 [details] [diff] [review]
Patch v1.1

>-    for (p = path; *p; ++p) {
>+    for (p = path; p < path + pathLen; ++p)

This fails to compile, backed out.

Comment 8

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1.2

Sorry for the trouble everyone - I missed this one because my build file strangely has the missing |{| already so I'm not sure how its missing from the patch.

I'm going to ask biesi to re-review this patch just to be on the safe side.

Comment 9

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

this should get a testcase, though we don't have a way to create one yet.

Comment 10

[Nickolay_Ponomarev]

Re-landed:
mozilla/netwerk/base/src/nsURLParsers.cpp  1.29

Sorry I didn't try to fix the bustage the last time - didn't really have time to experiment on the tinderbox :)