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

RESOLVED FIXED

Status

()

Core
Networking
--
critical
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: Andrew Miller, Assigned: sciguyryan)

Tracking

({crash, testcase})

Trunk
x86
All
crash, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

11 years ago
Created attachment 256248 [details]
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

11 years ago
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.
Component: Networking → Networking: JAR
QA Contact: networking → networking.jar
Summary: Stack overflow when URLs nested too deeply → Stack overflow when jar: URLs are nested too deeply
(Reporter)

Comment 2

11 years ago
(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.
Summary: Stack overflow when jar: URLs are nested too deeply → Stack overflow when URLs (jar:, view-source:, etc) are nested too deeply

Updated

11 years ago
Group: security
Severity: normal → critical
Component: Networking: JAR → Networking
Keywords: testcase
QA Contact: networking.jar → networking
(Assignee)

Updated

11 years ago
Assignee: nobody → sciguyryan
(Assignee)

Updated

11 years ago
OS: Linux → All
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 3

11 years ago
Created attachment 257002 [details] [diff] [review]
Patch v1

Patch v1

* Idea proposed by biesi. With this code implemented the testcase no longer hangs nor crashes.
Attachment #257002 - Flags: superreview?(cbiesinger)
Attachment #257002 - Flags: review?(cbiesinger)
Comment on attachment 257002 [details] [diff] [review]
Patch v1

please write a unit test for this too
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

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

Comment 7

11 years ago
(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).
...why would that be faster?
(Assignee)

Comment 9

11 years ago
(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.
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).
(Assignee)

Comment 11

11 years ago
Created attachment 257042 [details] [diff] [review]
Patch v1.1

Patch v1.1

* Added a unit test and fixed up a few errors.
Attachment #257002 - Attachment is obsolete: true
Attachment #257042 - Flags: superreview?(cbiesinger)
Attachment #257042 - Flags: review?(bzbarsky)
Attachment #257002 - Flags: superreview?(cbiesinger)
Attachment #257002 - Flags: review?(cbiesinger)
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.
Attachment #257042 - Flags: review?(bzbarsky) → review-
(Assignee)

Updated

11 years ago
Attachment #257042 - Flags: superreview?(cbiesinger)
(Assignee)

Comment 13

11 years ago
(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 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.
Attachment #257042 - Flags: superreview-
(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.
(Assignee)

Comment 16

11 years ago
Created attachment 257057 [details] [diff] [review]
Patch v1.2

Patch v1.2

Hopefully addresses the comments for Patch v1.1 :)
Attachment #257042 - Attachment is obsolete: true
Attachment #257057 - Flags: superreview?(cbiesinger)
Attachment #257057 - Flags: review?(bzbarsky)
(Reporter)

Comment 17

11 years ago
Shouldn't we also fix:

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

which also uses recursion to find the inner URI.
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.
(Assignee)

Comment 19

11 years ago
(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?
(Assignee)

Updated

11 years ago
Attachment #257057 - Flags: superreview?(cbiesinger)
Attachment #257057 - Flags: review?(bzbarsky)
(Assignee)

Comment 20

11 years ago
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.
> 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.
(Assignee)

Updated

11 years ago
Attachment #257057 - Flags: superreview?(cbiesinger)
Attachment #257057 - Flags: review?(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
Attachment #257057 - Flags: review?(bzbarsky) → review+
(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 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)
Attachment #257057 - Flags: superreview?(cbiesinger) → superreview+
(Assignee)

Comment 26

11 years ago
Created attachment 258105 [details] [diff] [review]
Patch v1.3 (For checkin)

Patch v1.3 (For checkin)

Added a comment too the unit test as requested by biesi.
Attachment #257057 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Whiteboard: [checkin needed]

Comment 27

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [checkin needed]

Updated

9 years ago
Depends on: 529150
You need to log in before you can comment on or make changes to this bug.