Closed Bug 1216970 Opened 4 years ago Closed 4 years ago

ProfilerImpl::GetStacktrace passes out pointers to going-to-be-freed memory

Categories

(Core :: Gecko Profiler, defect)

43 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: froydnj, Assigned: kanru)

References

Details

(Whiteboard: [adv-main44-])

Attachments

(1 file)

ProfilerImpl::GetStacktrace has:

  nsTArray<nsCString> trace;
  nsAutoArrayPtr<char> output(new char[BACKTRACE_BUFFER_SIZE]);

  profiler_get_backtrace_noalloc(output, BACKTRACE_BUFFER_SIZE);
  for (const char* p = output; *p; p += strlen(p) + 1) {
    trace.AppendElement(nsDependentCString(p));
  }

  return trace;

The line:

    trace.AppendElement(nsDependentCString(p));

seems bogus, as nsDependentCString depends on the memory it references outliving the string itself.  But in this case, the memory it's referencing is going to be freed when GetStacktrace returns by nsAutoArrayPtr's destructor.  (It's possible there's some tricky stuff going on under the hood with how strings get returned in this case, but that seems unlikely.)

Marking as s-s for UAF.  kanru, can you fix this?
Flags: needinfo?
Really ni?'ing kanru this time...Kan-Ru, can you fix this?
Flags: needinfo? → needinfo?(kchen)
Assignee: nobody → kchen
Flags: needinfo?(kchen)
(In reply to Nathan Froyd [:froydnj] from comment #0)
> ProfilerImpl::GetStacktrace has:
> 
>   nsTArray<nsCString> trace;
>   nsAutoArrayPtr<char> output(new char[BACKTRACE_BUFFER_SIZE]);
> 
>   profiler_get_backtrace_noalloc(output, BACKTRACE_BUFFER_SIZE);
>   for (const char* p = output; *p; p += strlen(p) + 1) {
>     trace.AppendElement(nsDependentCString(p));
>   }

Since we are appending to a nsTArray<nsCString>, I think the nsCString constructor should always copy the data from the nsDependentCString instead of move it. My compiled result with clang++-3.5 does invoke the nsTString_CharT(const self_type& aStr) constructor and copies the data.

So it doesn't look like a UAF to me. Do you think this might not be true in some toolchain combination?
Flags: needinfo?(nfroyd)
Ah, indeed, copying an nsDependentCString does copy the underlying data.  So there's no bug here...currently.  Move construction would make this code dodgy, though, and it's arguably not obvious what's going on if one appeals to the compiled code. ;)

WDYT about rewriting the loop body like so:

  trace.AppendElement()->Assign(p, strlen(p));

I think that makes it more obvious that the copying is taking place.
Flags: needinfo?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #3)
> Ah, indeed, copying an nsDependentCString does copy the underlying data.  So
> there's no bug here...currently.  Move construction would make this code
> dodgy, though, and it's arguably not obvious what's going on if one appeals
> to the compiled code. ;)

I have to admit that the nsStringAPI code is somewhat hard to trace :)

> WDYT about rewriting the loop body like so:
> 
>   trace.AppendElement()->Assign(p, strlen(p));
> 
> I think that makes it more obvious that the copying is taking place.

Works for me. Could I make that change and r=you?
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #4)
> (In reply to Nathan Froyd [:froydnj] from comment #3)
> > Ah, indeed, copying an nsDependentCString does copy the underlying data.  So
> > there's no bug here...currently.  Move construction would make this code
> > dodgy, though, and it's arguably not obvious what's going on if one appeals
> > to the compiled code. ;)
> 
> I have to admit that the nsStringAPI code is somewhat hard to trace :)

That's fair!  It took me a while tracing through it to figure out what's going on, and I'm supposed to own the code! :)

> > WDYT about rewriting the loop body like so:
> > 
> >   trace.AppendElement()->Assign(p, strlen(p));
> > 
> > I think that makes it more obvious that the copying is taking place.
> 
> Works for me. Could I make that change and r=you?

Sure, thanks.  Either call Assign or Append, depending on which you think is clearer.
https://hg.mozilla.org/mozilla-central/rev/abaedc34e7e9
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Group: core-security → core-security-release
This shouldn't have been checked in without a security rating and sec-approval+ unless it is trunk only.

Can you please set sec-approval? on the patch and fill out the template?
Flags: needinfo?(kchen)
Whiteboard: [adv-main44+]
Attached patch patchSplinter Review
Attachment #8709807 - Flags: review+
As indicated in comment 2 and comment 3, this is not really an security issue. I'm inclined to transform this to a normal bug. What do you think?
Flags: needinfo?(kchen) → needinfo?(abillings)
I agree on reading it more closely.
Flags: needinfo?(abillings)
Whiteboard: [adv-main44+]
Whiteboard: [adv-main44-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.