371473 - Stack overflow when URLs (jar:, view-source:, etc) are nested too deeply

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Andrew Miller]

Attachment: A testcase which shows the problem

URLs like jar: can be nested an arbitrary number of levels deep. However, there is no limit on the number of stack frames which the code will enter in to parse such a URL. This can result in a stack overflow.

It is not clear whether this can be exploited by methods such as making the stack and heap meet.

This crashes both trunk and 1.8 branch, although it seems to allocate a lot more heap on trunk builds (so be warned, and set your quotas before trying the testcase).

Comment 1

[Jesse Ruderman]

There are lots of public stack overflow bugs.  I don't think this bug needs to be hidden unless you think the functions involved here are especially likely to do the kinds of things that could cause the stack and heap to meet.

Comment 2

[Andrew Miller]

(In reply to comment #1)
> There are lots of public stack overflow bugs.  I don't think this bug needs to
> be hidden unless you think the functions involved here are especially likely to
> do the kinds of things that could cause the stack and heap to meet.
> 
If the policy is to make such bugs public, then this should be done.

I note that you have changed the summary to specifically mention jar:. It is worth noting that any nested URI scheme (at least view-source:, I presume about: is safe, as the user doesn't control the inner URI) will also suffer a stack overflow, either at the parsing stage, or in NS_DoImplGetInnermostURI when the script security manager tries to fetch the inner URI.

Comment 3

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1

Patch v1

* Idea proposed by biesi. With this code implemented the testcase no longer hangs nor crashes.

Comment 4

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

Comment on attachment 257002 [details] [diff] [review]
Patch v1

please write a unit test for this too

Comment 5

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

also, the IO service part is basically written by me, I don't think I should be the reviewer for that. (hm... should probably use PRUint32 instead of unsigned, too)

Comment 6

[kitchin]

This part is not needed:
+        if (!nestedInner) {
+            break;
+        }

Comment 7

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #6)
> This part is not needed:
> +        if (!nestedInner) {
> +            break;
> +        }
> 

As soon as we no longer have |nestedInner| the loop will fail in any case. Its just quicker to break from there than to start the cycle over (even if it won't be  run next time).

Comment 8

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

...why would that be faster?

Comment 9

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #8)
> ...why would that be faster?
> 

Because we don't need to go back to the start of the while loop and it makes more logical sence that way I think.

Comment 10

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

When it comes to that, why not remove the condition from the loop? But, unconditional jumps are fast, and it's not certain that you do have to back to the start of the while loop anyway (depends how the compiler generates the code).

Comment 11

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1.1

Patch v1.1

* Added a unit test and fixed up a few errors.

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

>Index: netwerk/base/public/nsNetUtil.h
>+        nestedInner = do_QueryInterface(inner, &rv);
>+        NS_ENSURE_SUCCESS(rv, rv);

This will make the function fail any time a nested URI is passed in.  Which makes me question whether this was even tested.

>Index: netwerk/base/src/nsIOService.cpp
> nsIOService::NewURI(const nsACString &aSpec, const char *aCharset, nsIURI *aBaseURI, nsIURI **result)

Darin or biesi really need to look at this change.  I'm not convinced that you're allowed to make this main-thread-only.

>Index: netwerk/test/unit/test_bug371473.js

At the very least you need to also test that a not-too-long jar: URI does NOT throw.  As written, a newURI impl that always throws would pass this test.

Comment 13

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #12)
> (From update of attachment 257042 [details] [diff] [review])
> >Index: netwerk/base/public/nsNetUtil.h
> >+        nestedInner = do_QueryInterface(inner, &rv);
> >+        NS_ENSURE_SUCCESS(rv, rv);
> 
> This will make the function fail any time a nested URI is passed in.  Which
> makes me question whether this was even tested.
> 
I'll fix that.

> >Index: netwerk/base/src/nsIOService.cpp
> > nsIOService::NewURI(const nsACString &aSpec, const char *aCharset, nsIURI *aBaseURI, nsIURI **result)
> 
> Darin or biesi really need to look at this change.  I'm not convinced that
> you're allowed to make this main-thread-only.
> 
Biesi suggested the code (and wrote most of it) but I'll check its OK.

> >Index: netwerk/test/unit/test_bug371473.js
> 
> At the very least you need to also test that a not-too-long jar: URI does NOT
> throw.  As written, a newURI impl that always throws would pass this test.
> 

I'll do that. Thanks for the quick review! I'll upload a new patch shortly.

Comment 14

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

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

+  do_throw("newURI didn't throw when it was passed a large nested URI?");

don't think you should do that. if creating the URI works, so much the better! it only shouldn't crash.

Comment 15

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

(In reply to comment #12)
> Darin or biesi really need to look at this change.  I'm not convinced that
> you're allowed to make this main-thread-only.

Oh yeah, the IO service is main-thread-only. That change is fine.

Comment 16

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1.2

Patch v1.2

Hopefully addresses the comments for Patch v1.1 :)

Comment 17

[Andrew Miller]

Shouldn't we also fix:

nsDocNavStartProgressListener::IsSpurious(nsIURI* aURI, PRBool* isSpurious)

which also uses recursion to find the inner URI.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Probably, but that's a different product, with different module owners, etc.  We'll need a separate bug/patch for that...

Ryan, I'd like the thread issue resolved so we're sure the approach is ok before I really get into checking over the tests and such.

Comment 19

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #18)
> Ryan, I'd like the thread issue resolved so we're sure the approach is ok
> before I really get into checking over the tests and such.
> 

I'm afraid I'm not really sure of how to solve the thread issue here without making the solution more complex (by making the counter run on its own thread or the like).

Boris, Christian: Any better ideas?

Comment 20

[Ryan Jones-Ward [:sciguyryan]]

We could probably solved this by directly fixing the affected function(s) that calls |nsIOService::NewURI| actually (that would mean jar for sure and maybe view-source).
That would avoid the thread problem I believe because we could simply work within the function callers of |nsIOService::NewURI|. We could say work with the inner spec for the URI in question instead of trying to loop to find the inner URI as we do here.

Comment 21

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

bz, did you see comment 15?

Comment 22

[Boris Zbarsky [:bzbarsky]]

> bz, did you see comment 15?

Nope.  Not cced on the bug and all, and you didn't mention that in your updated patch post...

If it's ok with biesi, it's fine with me.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 257057 [details] [diff] [review]
Patch v1.2

>Index: netwerk/test/unit/test_bug371473.js
>+    do_throw("newURI threw even thought it wasn't passed a large nested URI?");

s/thought/though/

r=bzbarsky with that

Comment 24

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

(In reply to comment #22)
> Nope.  Not cced on the bug and all, and you didn't mention that in your updated
> patch post...

Ah, sorry... didn't realize you weren't cc'd.

Comment 25

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

Comment on attachment 257057 [details] [diff] [review]
Patch v1.2

perhaps you should add a comment near the newURI call in test_too_long() that all you're testing is that it doesn't crash (as-is, it's not immediately obvious what this function is testing)

Comment 26

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1.3 (For checkin)

Patch v1.3 (For checkin)

Added a comment too the unit test as requested by biesi.

Comment 27

[Nickolay_Ponomarev]

mozilla/netwerk/base/public/nsNetUtil.h      1.106
mozilla/netwerk/base/src/nsIOService.cpp     1.193
mozilla/netwerk/test/unit/test_bug371473.js  1.1