Bad-casting from PRCListStr to nsSHistory

RESOLVED FIXED in Firefox 36

Status

()

Core
Document Navigation
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: lifeasageek, Assigned: smaug)

Tracking

({sec-other})

Trunk
mozilla36
x86_64
Linux
sec-other
Points:
---

Firefox Tracking Flags

(firefox34 wontfix, firefox35 wontfix, firefox36 fixed, firefox-esr31 wontfix)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/37.0.2062.120 Safari/537.36

Steps to reproduce:





Actual results:

Hi Firefox!

Please refer bug1074280 for this report.

// From docshell/shistory/src/nsSHistory.cpp:1145

    transactions.AppendElements(shTransactions);
    shist = static_cast<nsSHistory*>(PR_NEXT_LINK(shist));

At the end of the loop, PR_NEXT_LINK(shist) points to the static global variable, "static PRCList gSHistoryList". Thus, the object pointed by PR_NEXT_LINK(shist) is not a sub-object of nsSHistory, and triggers undefined-behavior.

---------------------------------------------

/home/blee/project/firefox/firefox/docshell/shistory/src/nsSHistory.cpp:1145:13: Casting from 'PRCListStr' to 'nsSHistory'
         Pointer         0x7f66feee6be0                                                                                                                                      
         Alloc base      0x000000000000
         User base       0x000000000000
         Offset          0x7f66feee6be0
         TypeTable       0x000000000000
    #0 0x404e19 in __csan_handle_cast /home/blee/project/cast_sanitizer/llvm/projects/compiler-rt/lib/csan/csan_common.cc:353
    #1 0x7f66fbc5e907 in GloballyEvictContentViewers /home/blee/project/firefox/firefox/docshell/shistory/src/nsSHistory.cpp:1145 (discriminator 4)
    #2 0x7f66fbc6073e in EvictOutOfRangeContentViewers /home/blee/project/firefox/firefox/docshell/shistory/src/nsSHistory.cpp:806
    #3 0x7f66fb8674ac in Show /home/blee/project/firefox/firefox/layout/base/nsDocumentViewer.cpp:2017
    #4 0x7f66fb90a77b in EnsureVisible /home/blee/project/firefox/firefox/layout/base/nsPresContext.cpp:2015
    #5 0x7f66fb875d17 in UnsuppressAndInvalidate /home/blee/project/firefox/firefox/layout/base/nsPresShell.cpp:3928 (discriminator 2)
    #6 0x7f66fb864f40 in LoadComplete /home/blee/project/firefox/firefox/layout/base/nsDocumentViewer.cpp:1043

------------------------------
Name    struct PRCListStr
Size    16
NumVar  2
NumBase 0
------------------------------
   0 | struct PRCListStr                                                                                                                                                     
   0 |   PRCList * next
   8 |   PRCList * prev
     | [sizeof=16, dsize=16, align=8
     |  nvsize=16, nvalign=8]

------------------------------
Name    class nsSHistory
Size    128
NumVar  8
NumBase 4
        0       class nsISHistory ['(primary base)']
        8       struct PRCListStr ['(base)']
	24      class nsISHistoryInternal ['(base)']
	32      class nsIWebNavigation ['(base)']

------------------------------
   0 | class nsSHistory
   0 |   class nsISHistory (primary base)
   0 |     class nsISupports (primary base)
   0 |       (nsISupports vtable pointer)
   8 |   struct PRCListStr (base)
   8 |     PRCList * next
  16 |     PRCList * prev
  24 |   class nsISHistoryInternal (base)
  24 |     class nsISupports (primary base)
  24 |       (nsISupports vtable pointer)
  32 |   class nsIWebNavigation (base)
...

Comment 1

3 years ago
hg blame has bug 683777 r=smaug, so starting with NEEDINFO there.
Component: Untriaged → Document Navigation
Flags: needinfo?(bugs)
Product: Firefox → Core
(Assignee)

Comment 2

3 years ago
The code is actually ancient, coming from bug 292965.
And as far as I see we just end up using that pointer in a pointer comparison so nothing bad should happen.
(Assignee)

Updated

3 years ago
Assignee: nobody → bugs
Flags: needinfo?(bugs)
(Assignee)

Comment 3

3 years ago
Created attachment 8512089 [details] [diff] [review]
v1

I guess this should silence the warning.
Attachment #8512089 - Flags: review?(jst)
> Please refer bug1074280 for this report.

Thanks for the report.  In general, it would be good to just copy and paste your brief explanation of what is happening when you file these bugs, because somebody who has access to one bug may not have access to the other.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

3 years ago
Attachment #8512089 - Flags: review?(jst) → review+
(Assignee)

Comment 5

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfa520bee5fc
https://hg.mozilla.org/mozilla-central/rev/cfa520bee5fc
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox36: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Can we get a security rating for this issue? If this is serious, we need to backport this to Aurora, Beta, and ESR31.

This shouldn't have landed without one (and sec-approval+ if it is high or critical) unless it is trunk only. Since this seems to go back to 2011, I assume everything is affected. See https://wiki.mozilla.org/Security/Bug_Approval_Process
Flags: needinfo?(bugs)
(Assignee)

Comment 8

3 years ago
I don't see anything critical/high here. We would have either crashed (in a bad way) or ended up to a
endless loop if something had gone wrong. The code is ancient and we haven't seen issues with it.
Flags: needinfo?(bugs)
Keywords: sec-other
status-firefox34: --- → wontfix
status-firefox35: --- → wontfix
status-firefox-esr31: --- → wontfix

Updated

2 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.