|unescaped| in jsregexp.cpp:EscapeNakedForwardSlashes needs an anchor and some OOM-checking

RESOLVED FIXED in mozilla2.0

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

unspecified
mozilla2.0
Points:
---

Firefox Tracking Flags

(status1.9.2 unaffected, status1.9.1 unaffected)

Details

(Whiteboard: [sg:moderate][fixed-in-tracemonkey])

Attachments

(1 attachment)

Created attachment 516023 [details] [diff] [review]
Patch

If the string contains any forward slashes, we start appending to a new string.  If an append fails, the chars we got from |unescaped| might be killed, or even reallocated by something else entirely, if the compiler can see we did an append, ergo |unescaped| no longer need be kept alive.  So we should be checking for OOM here.  And probably for sanity we should anchor |unescaped|, too, because this code is arguably not transparently, obviously, intuitively correct with the OOM-checks in place.
Attachment #516023 - Flags: review?(brendan)
I think the worst that happens here is disclosure of memory if the unescaped chars pointer memory is reallocated to something sensitive.  But I'm not certain what the side effects of an ignored OOM report are; if that sets something in a context or whatever such that the new string isn't actually exposed (say an error check detects an error was thrown even tho success was reporte), it might be only a theoretical security concern.  (Although it would still generally be a crash, so certainly worth fixing even if not security-sensitive.)

Older releases manually root the string and don't use a Vector at all, so no worries for them.
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected
Whiteboard: [sg:moderate]
Comment on attachment 516023 [details] [diff] [review]
Patch

hg annotate says:

changeset:   49289:597254d97174
user:        Chris Leary <cdleary@mozilla.com>
date:        Wed Aug 11 13:30:07 2010 -0700
summary:     Bug 564953: Port YARR! Lands macroassembler. (r=gal)

Cc'ing folks too.

/be
Attachment #516023 - Flags: review?(brendan) → review?(cdleary)
(In reply to comment #2)

Waldo and I had already talked about it. We wanted to r? you because we were unsure of whether you wanted the anchor in the caller or if it was okay in the callee.
(In reply to comment #3)
> (In reply to comment #2)
> 
> Waldo and I had already talked about it. We wanted to r? you because we were
> unsure of whether you wanted the anchor in the caller or if it was okay in the
> callee.

Oh -- thanks for clarifying.

I'm not the Anchor guru (jimb?) but caller roots is the usual rule, because a callee-root may leave a window of vulnerability, which is hard to see, wide open among the callers.

Any reason not to anchor in the caller?

/be

Comment 5

7 years ago
(In reply to comment #4)
> I'm not the Anchor guru (jimb?) but caller roots is the usual rule, because a
> callee-root may leave a window of vulnerability, which is hard to see, wide
> open among the callers.

No, the Anchor should go in EscapeNakedForwardSlashes, because the derived value (the character pointer) is local to that function. We don't care if the string goes away once the character pointer is dead.
Attachment #516023 - Flags: review?(cdleary) → review+
Comment on attachment 516023 [details] [diff] [review]
Patch

Small-ish security problem, shouldn't be put off until 5, but not a biggie if it doesn't happen strictly by 4.0 release time, as the concern depends on compilers being *just* *that* *smart*, which they might not be.  If you think there's still time to take a straightforward and well-understood change, you should take this.  Otherwise I can wait for a 4.0.1 release (and for TM/m-c to reopen).
Attachment #516023 - Flags: approval2.0?
(In reply to comment #5)
> (In reply to comment #4)
> > I'm not the Anchor guru (jimb?) but caller roots is the usual rule, because a
> > callee-root may leave a window of vulnerability, which is hard to see, wide
> > open among the callers.
> 
> No, the Anchor should go in EscapeNakedForwardSlashes, because the derived
> value (the character pointer) is local to that function. We don't care if the
> string goes away once the character pointer is dead.

Cool, I was giving the general rule if the rooted/anchored thing flows back to the caller. If it doesn't, then there's no caller to care about (or the callee is the caller). Thanks for looking at this.

/be

Updated

7 years ago
Attachment #516023 - Flags: approval2.0? → approval2.0+

Comment 8

7 years ago
http://hg.mozilla.org/mozilla-central/rev/290712e55ade
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0
http://hg.mozilla.org/tracemonkey/rev/423a8e7df455
Whiteboard: [sg:moderate] → [sg:moderate][fixed-in-tracemonkey]
Group: core-security
You need to log in before you can comment on or make changes to this bug.