Closed Bug 1378374 Opened 7 years ago Closed 7 years ago

AddressSanitizer: heap-use-after-free ProtocolUtils.h:177:33 in Id

Categories

(Core :: IPC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed

People

(Reporter: cbook, Assigned: billm)

References

(Blocks 1 open bug, )

Details

(4 keywords)

Attachments

(2 files)

noticed this on bughunter for the url http://www.hk2love.com/cr/kw/k8.htm - that site itself is strange and windows defender warns of malicious things.

On Linux visting this site result in a crash - however it seems bughunter found also a heap-use-after-free issue like:



SUMMARY: AddressSanitizer: heap-use-after-free /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ipc/ProtocolUtils.h:177:33 in Id
Shadow bytes around the buggy address:
  0x0c08800252f0: fa fa 00 00 00 00 00 00 fa fa 00 00 00 00 00 fa
  0x0c0880025300: fa fa 00 00 00 00 00 fa fa fa fd fd fd fd fd fd
  0x0c0880025310: fa fa 00 00 00 00 00 00 fa fa 00 00 00 00 00 fa
  0x0c0880025320: fa fa 00 00 00 00 04 fa fa fa fd fd fd fd fd fd
  0x0c0880025330: fa fa 00 00 00 00 00 fa fa fa fd fd fd fd fd fd
=>0x0c0880025340: fa fa fd fd fd fd fd fd fa fa fd[fd]fd fd fd fa
  0x0c0880025350: fa fa fd fd fd fd fd fd fa fa 00 00 00 00 00 fa
  0x0c0880025360: fa fa 00 00 00 00 00 00 fa fa fd fd fd fd fd fa
  0x0c0880025370: fa fa fd fd fd fd fd fa fa fa 00 00 00 00 00 fa
  0x0c0880025380: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fa
  0x0c0880025390: fa fa 00 00 00 00 00 fa fa fa fd fd fd fd fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
Tomcat, is there a more complete report? You didn't include any stacks here.
Flags: needinfo?(cbook)
Group: core-security → dom-core-security
Attached file stack
Flags: needinfo?(cbook)
(In reply to Andrew McCreight [:mccr8] from comment #1)
> Tomcat, is there a more complete report? You didn't include any stacks here.

yeah sorry forgot to attach here :(
Thanks for the stacks. From the stacks, it looks like we're in mozilla::dom::ContentChild::ProvideWindowCommon, then we allocate a AllocPRenderFrameChild, spin the event loop (which destroys the PRenderFrameChild), then call PRenderFrameChild::Send__delete__.

It looks like the event loop spinning was added in bug 1343728. Could you take a look, Michael? Thanks.
Flags: needinfo?(michael)
(In reply to Andrew McCreight [:mccr8] from comment #4)
> Thanks for the stacks. From the stacks, it looks like we're in
> mozilla::dom::ContentChild::ProvideWindowCommon, then we allocate a
> AllocPRenderFrameChild, spin the event loop (which destroys the
> PRenderFrameChild), then call PRenderFrameChild::Send__delete__.
> 
> It looks like the event loop spinning was added in bug 1343728. Could you
> take a look, Michael? Thanks.

It looks like the problem here is caused by us assuming that the PRenderFrameChild is still alive after the event loop has been spun, which is a bad assumption because we passed the object over IPC to the other side.

Only way I can think of to handle this off of the top of my head would be to have some way for the RenderFrameChild to notify ProvideWindowCommon that it is being torn down, and then not calling Send__delete__ in that case. There might be some better way to do it but I can't think of it off of the top of my head. Perhaps billm will have a better idea for how to tell if someone else destroyed your IPC actor while you were spinning a nested event loop?
Flags: needinfo?(michael) → needinfo?(wmccloskey)
These PRenderFrameChild::Send__delete__ calls are incorrect. The PRenderFrame actor is a child of PBrowser. So killing the PBrowser will automatically kill the PRenderFrame. I think that the RecvCreateWindow code takes care of shutting down the new <browser> element in case of failure (which is probably why this error happens).

I'm a little unsure of the |if (layersId == 0)| case. That one may also not need PRenderFrameChild::Send__delete__, but I'm not sure. needinfo dvander for that.
Flags: needinfo?(wmccloskey) → needinfo?(dvander)
BTW, there's a reduced testcase (520 bytes) over on dupe-bug 1378112, currently labeled as "CRASH Testcase #2".
(In reply to Bill McCloskey (:billm) from comment #6)
> I'm a little unsure of the |if (layersId == 0)| case. That one may also not
> need PRenderFrameChild::Send__delete__, but I'm not sure. needinfo dvander
> for that.

It looks like it can be 0 with an allocated PRenderFrame if we get here [1], where |rv| is explicitly assigned and then ignored. But I'm not sure why we need to clean up PRenderFrame immediately. It should be okay to let it happen automatically, and then if that exposes other problems maybe we should just fix those.

[1] http://searchfox.org/mozilla-central/source/dom/ipc/ContentParent.cpp#4735
Blocks: 1378112
Assignee: nobody → wmccloskey
Attachment #8891112 - Flags: review?(dvander)
Attachment #8891112 - Flags: review?(dvander) → review+
https://hg.mozilla.org/mozilla-central/rev/2fbfb1b926ef

Looking at the relevant blame, it looks like this issue affects more than just 56?
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(wmccloskey)
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Group: dom-core-security → core-security-release
I think the event loop spinning in bug 1343728 is what triggered this. So I don't think this needs a backport.
Flags: needinfo?(wmccloskey)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.