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

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Nathan Froyd [:froydnj]]

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?

Comment 1

[Nathan Froyd [:froydnj]]

Really ni?'ing kanru this time...Kan-Ru, can you fix this?

Comment 2

[Kan-Ru Chen [:kanru] (UTC+9)]

(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?

Comment 3

[Nathan Froyd [:froydnj]]

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.

Comment 4

[Kan-Ru Chen [:kanru] (UTC+9)]

(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?

Comment 5

[Nathan Froyd [:froydnj]]

(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.

Comment 6

[Kan-Ru Chen [:kanru] (UTC+9)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/abaedc34e7e9

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/abaedc34e7e9

Comment 8

[Al Billings [:abillings - ex-MoCo]]

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?

Comment 9

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: patch

Comment 10

[Kan-Ru Chen [:kanru] (UTC+9)]

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?

Comment 11

[Al Billings [:abillings - ex-MoCo]]

I agree on reading it more closely.