More missing callgraph edges involving destructors

RESOLVED FIXED in Firefox 39, Firefox OS v2.1

Status

()

Core
JavaScript: GC
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

(Blocks: 1 bug, {sec-high})

unspecified
mozilla40
x86_64
Linux
sec-high
Points:
---

Firefox Tracking Flags

(firefox33 wontfix, firefox34 wontfix, firefox35 wontfix, firefox36 wontfix, firefox37 wontfix, firefox38 wontfix, firefox38.0.5 wontfix, firefox39 fixed, firefox40 fixed, firefox-esr31 unaffected, firefox-esr3839+ fixed, b2g-v1.4 unaffected, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 fixed, b2g-v2.1S fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [b2g-adv-main2.2+][adv-main39+][adv-esr38.1+])

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

3 years ago
Bug 1047696 looks to have revealed *another* gap in the rooting hazard analysis.

Before MOZ_FINAL, PrepareForWrapping called '_ZN14XPCCallContextD1Ev' aka |void XPCCallContext::~XPCCallContext()|, and the analysis didn't think that called anything. The analysis is aware of 4 different destructors:

  (#1) _ZN14XPCCallContextD1Ev|void XPCCallContext::~XPCCallContext()
  (#2) _ZN14XPCCallContextD1Ev *INTERNAL* |void XPCCallContext::~XPCCallContext(int32)
  (#3) _ZN14XPCCallContextD1Ev|void XPCCallContext::~XPCCallContext(int32)
  (#4) _ZN14XPCCallContextD0Ev|void XPCCallContext::~XPCCallContext()

which blows my mind -- notice how the mangled name _ZN14XPCCallContextD1Ev resolves to two different unmangled names, and the unmangled name 'void XPCCallContext::~XPCCallContext()' is associated with two different mangled names. *groan* Anyway, #3 above calls all kinds of stuff, including ~nsRefPtr which is what happens with MOZ_FINAL added as seen in the hazard output at https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/trev.saunders@gmail.com-f9d622c91bed/try-linux64-br-haz/hazards.txt.gz

To continue, the *INTERNAL* destructors I already knew about -- they are "not-in-charge" destructors for use with virtual inheritance. When they are identical to the "in-charge" destructors, gcc aliases them to the same thing in some weird way that is not visible in the output provided to the analysis plugin. I previously fixed that problem by pretending that one of them calls the other, since the analysis plugin would not otherwise see a body for one of them at all. (To be honest, I forget which is which.)

But now we seem to have progressed to an even higher degree of insanity. The plugin sees bodies generated for #2 and #4 above, but not for #1 and #3. Without MOZ_FINAL, it sees an edge from PrepareForWrapping to #1. The existing aliasing workaround adds an artificial edge from #3 to #2. There is a natural edge from #4 to #1: the D0 in the mangled name means "deleting destructor", which is a destructor that finalizes and additional frees the actual memory. So gcc generates a body that basically calls #1 and then invokes operator delete.

So without MOZ_FINAL, we have:

  PrepareForWrapping -> #1
  #4 -> #1
  #3 -> #2
  #2 -> (important stuff)

It looks like we're missing an edge from #1 -> #3. Which are the two suspicious ones -- they have the same mangled name, but differ by an int32 parameter.
(Assignee)

Comment 1

3 years ago
Created attachment 8477042 [details] [diff] [review]
Add edge between internal destructors
(Assignee)

Updated

3 years ago
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Keywords: sec-high
Steve, is this ready to move forward?
status-firefox33: --- → affected
status-firefox34: --- → affected
status-firefox35: --- → affected
(Assignee)

Comment 3

3 years ago
I don't have a complete fix yet. I added in one edge, which fills in a bunch of the callgraph, but I'm still missing a destructor-related edge in a weird case that I haven't managed to reproduce in a minimal test case. I just need to spend a bit more time on it; I can use the full build as the test case.
Any progress here?  This is two months old now.  Thanks.
status-firefox33: affected → wontfix
status-firefox36: --- → affected
Flags: needinfo?(sphink)
Group: javascript-core-security
(Assignee)

Comment 5

3 years ago
Update: I still hope to fix this, but there are other things the analysis is missing that I'm in the middle of fixing and backporting right now, and there are other missing edges that I am also working on fixing. So this problem is real, it still exists, but it's really just an example of a pile of things the analysis is missing currently. I have no reason to suspect there are more or less vulnerabilities from this bug than from the other gaps.

I will file a bug for the other main source of missing edges. It should be a little easier to fix, and fill in a few more of the missing edges than this bug.
Flags: needinfo?(sphink)
(Assignee)

Updated

3 years ago
Blocks: 1143786
(Assignee)

Comment 6

3 years ago
Comment on attachment 8477042 [details] [diff] [review]
Add edge between internal destructors

Review of attachment 8477042 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure what a review here means, but adding edges can only be good, right?
Attachment #8477042 - Flags: review?(terrence)
Comment on attachment 8477042 [details] [diff] [review]
Add edge between internal destructors

Review of attachment 8477042 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, let's get it in since we know it finds at least one important path.
Attachment #8477042 - Flags: review?(terrence) → review+
(Assignee)

Comment 8

3 years ago
Ok, finally tracked this down. The edges added in the above patch are good and were missing before. However, they do not reveal any hazards (at least not now; some were fixed.)

The confusion arises from bug 1056410, which marks a whole bunch of classes as final. If nsXPConnect is marked final, then the analysis starts reporting a hazard. I mistakenly lumped this in with the missing destructors issue, since the new GC function in the hazard passes through a couple differently-named destructors. I was thinking that the new |final| designation simplified things enough for the analysis to see an edge through a destructor that it could not see previously.

But it turns out that the |final| designation is only serving to evade an analysis heuristic. Specifically, the analysis ignores all AddRef() and Release() virtual methods of any class inheriting from nsISupports. nsXPConnect implements its own Release() method, and if it is marked |final|, the compile devirtualizes it and the analysis heuristic no longer applies.

I could either spot-fix this by special-casing nsXPConnect::Release, or fix the slightly more general problem by applying the AddRef/Release heuristic for any function call, not just virtual ones.

Alternatively, I could see what it would take for the analysis to figure out by itself that Release calls don't GC. But from skimming those, that looks... challenging.
(Assignee)

Comment 9

3 years ago
Oops, the bug # in the last comment should have been bug 1047696, as it says in comment 0.
(Assignee)

Comment 10

3 years ago
Created attachment 8585700 [details] [diff] [review]
Mark more classes final

Bug 1047696 was blocked by the hazard here, so it ended up not marking problematic classes final. Mark them final.
Attachment #8585700 - Flags: review?(terrence)
(Assignee)

Comment 11

3 years ago
Created attachment 8585706 [details] [diff] [review]
GetNameShared cannot GC

This cleans up a false positive from the Array handling. It prunes back the beginning of the live range for an XPCNativeMember[] variable, and annotates away a trivial accessor function.
Attachment #8585706 - Flags: review?(terrence)
(Assignee)

Comment 12

3 years ago
Comment on attachment 8585706 [details] [diff] [review]
GetNameShared cannot GC

Nope, just this one annotation isn't enough. There are 2 other calls involved.
Attachment #8585706 - Flags: review?(terrence)
(Assignee)

Comment 13

3 years ago
Created attachment 8585830 [details] [diff] [review]
GetNameShared cannot GC

Trying again with https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd8d9f7aea44
Attachment #8585830 - Flags: review?(terrence)
(Assignee)

Updated

3 years ago
Attachment #8585706 - Attachment is obsolete: true
(Assignee)

Comment 14

3 years ago
Ugh. So that patch does no harm, but the analysis obnoxiously points out that GetConstant *can* GC for real:

GC Function: _ZN18xptiInterfaceEntry11GetConstantEtN2JS13MutableHandleINS0_5ValueEEEPPc|uint32 xptiInterfaceEntry::GetConstant(uint16, class JS::MutableHandle<JS::Value>, int8**)
    void mozilla::AutoJSContext::~AutoJSContext(int32)
    void mozilla::AutoJSContext::~AutoJSContext(int32)
    void mozilla::dom::AutoJSAPI::~AutoJSAPI()
    void mozilla::dom::AutoJSAPI::~AutoJSAPI(int32)
    void xpc::ErrorReport::LogToConsole()
    FieldCall: nsIScriptError.InitWithWindowID

So this isn't exactly a false positive, since we don't move strings yet. But it's lying in wait. (Well, I don't know if this path really can GC or not, but error reporting stuff typically can.)

I'll have to do something else here. Maybe even root things for real. With an external rooter? An overlaid stack-only class? Something.
Attachment #8585700 - Flags: review?(terrence) → review+
Attachment #8585830 - Flags: review?(terrence) → review+
(Assignee)

Comment 15

3 years ago
Created attachment 8586520 [details] [diff] [review]
Root XPCNativeMember.name

Sadly, I don't see a way around just rooting this thing. GetConstant can legitimately GC, or at least it's really tough to prove otherwise.
Attachment #8586520 - Flags: review?(bobbyholley)
https://hg.mozilla.org/mozilla-central/rev/a6db8094e78f
https://hg.mozilla.org/mozilla-central/rev/fffed2b68c8e

Leaving this open since it appears there are still patches pending.
status-b2g-v2.1: --- → affected
status-b2g-v2.1S: --- → affected
status-b2g-v2.2: --- → affected
status-b2g-master: --- → affected
status-firefox34: affected → wontfix
status-firefox35: affected → wontfix
status-firefox36: affected → wontfix
status-firefox37: --- → wontfix
status-firefox38: --- → affected
status-firefox39: --- → affected
status-firefox40: --- → affected
Target Milestone: --- → mozilla40
Comment on attachment 8586520 [details] [diff] [review]
Root XPCNativeMember.name

Review of attachment 8586520 [details] [diff] [review]:
-----------------------------------------------------------------

mName is interned, right? Given that, I don't see why we need to root it. See XPCNativeInterface::NewInstance.
Attachment #8586520 - Flags: review?(bobbyholley)
(Assignee)

Comment 18

3 years ago
Nothing more is needed here.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
I assume things need uplift here? :)
status-b2g-v1.4: --- → unaffected
status-b2g-v2.0: --- → unaffected
status-b2g-v2.0M: --- → unaffected
status-firefox38.0.5: --- → affected
status-firefox40: affected → fixed
status-firefox-esr31: --- → unaffected
status-firefox-esr38: --- → affected
Flags: needinfo?(sphink)
Group: javascript-core-security
(Assignee)

Updated

3 years ago
Attachment #8586520 - Attachment is obsolete: true
(Assignee)

Comment 20

3 years ago
Comment on attachment 8477042 [details] [diff] [review]
Add edge between internal destructors

Approval Request Comment
[Feature/regressing bug #]: A bit of a mixture. Bug 963738 exposed some problems.

[User impact if declined]: None. The hazard analysis tests will be slightly less accurate for older branches. Backporting later fixes may be a little harder.

[Describe test coverage new/current, TreeHerder]: This has been running as part of the analysis for a month.

[Risks and why]: The only risk is that we will end up with a mismatched set of backports, which would probably require further work to make the analysis happy again on each branch, and in the worst case would cause actual exploitable hazards to be briefly visible in the TH view of these branches. The hazards this bug is dealing with haven't been of that flavor, though.

[String/UUID change made/needed]: None
Flags: needinfo?(sphink)
Attachment #8477042 - Flags: approval-mozilla-esr38?
Attachment #8477042 - Flags: approval-mozilla-beta?
Attachment #8477042 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 21

3 years ago
Comment on attachment 8585830 [details] [diff] [review]
GetNameShared cannot GC

Approval Request Comment
[Feature/regressing bug #]: see previous approval request, but this specific patch doesn't fix any real issues at all. It's just for ease of backporting.

[User impact if declined]: None. Approval requested to make backports easier.

[Risks and why]: very low
Attachment #8585830 - Flags: approval-mozilla-esr38?
Attachment #8585830 - Flags: approval-mozilla-beta?
Attachment #8585830 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 22

3 years ago
Comment on attachment 8585700 [details] [diff] [review]
Mark more classes final

Approval Request Comment
[Feature/regressing bug #]: see above. This patch is also not fixing any real issue.

[User impact if declined]: Probably none, some possibility of a very slight perf hit though I doubt it. Approval is requested for ease of backports.

[Risks and why]: Very low. Changes code generation very very slightly, so I guess anything is theoretically possible.
Attachment #8585700 - Flags: approval-mozilla-esr38?
Attachment #8585700 - Flags: approval-mozilla-beta?
Attachment #8585700 - Flags: approval-mozilla-aurora?
Comment on attachment 8585700 [details] [diff] [review]
Mark more classes final

Approving for uplift to aurora and ESR38, since this is sec-high and may help with future backporting.    We have already built ESR 38.0 so this would be released in ESR 38.1.0.
Attachment #8585700 - Flags: approval-mozilla-esr38?
Attachment #8585700 - Flags: approval-mozilla-esr38+
Attachment #8585700 - Flags: approval-mozilla-aurora?
Attachment #8585700 - Flags: approval-mozilla-aurora+
Steve, I just notice that this doesn't have sec approval. Could you please request it? 

Kyle can you hold off on uplift to aurora until we have sec approval? Thanks.
Flags: needinfo?(sphink)
Why did I just say "Kyle"? Argh. I was thinking of Kwierso. Also, too many tabs open at once. 

Wes, can you hold off on uplift for these patches till we have sec approval?
Flags: needinfo?(wkocher)
Can do.
Flags: needinfo?(wkocher)
(Assignee)

Comment 27

3 years ago
Comment on attachment 8477042 [details] [diff] [review]
Add edge between internal destructors

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Highly unlikely

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No

Which older supported branches are affected by this flaw? back to esr38, I guess?

If not all supported branches, which bug introduced the flaw? This bug is for a category of problems, at this point none of which are serious (due to other fixes previously landing). Tracking this down to specific flaws would be possible, but annoying, and I don't think it really buys us anything here.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? I think this patch should apply to all affected branches.

How likely is this patch to cause regressions; how much testing does it need? Given that this only adds edges, it should only be able to cause more hazards to be reported, so the existing hazard analysis tests should be fine for detecting regressions.
Flags: needinfo?(sphink)
Attachment #8477042 - Flags: sec-approval?
(Assignee)

Comment 28

3 years ago
Comment on attachment 8585700 [details] [diff] [review]
Mark more classes final

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Highly unlikely

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No

Which older supported branches are affected by this flaw? None. This would be backported only to make other backporting easier.

If not all supported branches, which bug introduced the flaw? N/A. This patch does not fix any flaws (though it does introduce something that the previous code would not have caught; it's just that the same patch fixes that too.)

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Should backport cleanly.

How likely is this patch to cause regressions; how much testing does it need? I can't think of a way for this to regress anything, barring PGO wackiness.
Attachment #8585700 - Flags: sec-approval?
(Assignee)

Comment 29

3 years ago
Comment on attachment 8585830 [details] [diff] [review]
GetNameShared cannot GC

[Security approval request comment]
How easily could an exploit be constructed based on the patch? N/A; this patch does not fix anything.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No

Which older supported branches are affected by this flaw? All of them or none of them, depending on your perspective. I think the relevant code is about the same in all supported branches.

If not all supported branches, which bug introduced the flaw? N/A

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? I think it should work without change.

How likely is this patch to cause regressions; how much testing does it need? If this messes up, it will break the compile, not introduce subtle problems.

The risk with the hazard analysis change here is that we would cover up real hazards. That is very unlikely; it would have to be a flaw introduced by a binary extension that implemented nsIInterfaceInfo.GetMethodInfo or .GetConstant or .GetNameShared with something that can GC, which would be pretty weird. (If in-tree code implements any of them with code that can GC, the analysis won't be fooled by this annotation and will accurately report any hazards thus introduced.)
Attachment #8585830 - Flags: sec-approval?
Looking at comment 16, is this one of those situations where I'm being asked for sec-approval+ *after* things have been checked in? Because, if it is, I might as well give it since the point of such approval is to avoid early disclosure and we've already done that.

Otherwise, you'll get approval to check in on 5/26, two weeks into the next dev cycle.
(Assignee)

Comment 31

3 years ago
(In reply to Al Billings [:abillings] from comment #30)
> Looking at comment 16, is this one of those situations where I'm being asked
> for sec-approval+ *after* things have been checked in? Because, if it is, I
> might as well give it since the point of such approval is to avoid early
> disclosure and we've already done that.

Doh! Yes, sorry. And because I keep doing this, I filed bug 1164063.
Attachment #8477042 - Flags: sec-approval? → sec-approval+
Attachment #8585700 - Flags: sec-approval? → sec-approval+
Attachment #8585830 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 32

3 years ago
Comment on attachment 8585830 [details] [diff] [review]
GetNameShared cannot GC

On second thought, this patch never landed on m-c and no longer matters. So it's not going to help backporting. I'll just drop it.
Attachment #8585830 - Attachment is obsolete: true
Attachment #8585830 - Flags: sec-approval+
Attachment #8585830 - Flags: approval-mozilla-esr38?
Attachment #8585830 - Flags: approval-mozilla-beta?
Attachment #8585830 - Flags: approval-mozilla-aurora?
Comment on attachment 8477042 [details] [diff] [review]
Add edge between internal destructors

Approved for uplift to beta (39). This already landed on 40 so it no longer needs uplift to aurora.
Attachment #8477042 - Flags: approval-mozilla-beta?
Attachment #8477042 - Flags: approval-mozilla-beta+
Attachment #8477042 - Flags: approval-mozilla-aurora?
Comment on attachment 8585700 [details] [diff] [review]
Mark more classes final

Approved for uplift to beta (39). This already landed on 40 so it no longer needs uplift to aurora.
Attachment #8585700 - Flags: approval-mozilla-beta?
Attachment #8585700 - Flags: approval-mozilla-beta+
Attachment #8585700 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-beta/rev/0296484f4f84
https://hg.mozilla.org/releases/mozilla-beta/rev/b964c317dbff
status-b2g-master: affected → fixed
status-firefox38: affected → wontfix
status-firefox38.0.5: affected → wontfix
status-firefox39: affected → fixed
https://hg.mozilla.org/releases/mozilla-esr38/rev/6a9a086db35d
https://hg.mozilla.org/releases/mozilla-esr38/rev/7fd4fa3ef5c1
status-firefox-esr38: affected → fixed
tracking-firefox-esr38: --- → ?
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/af7478dd5b99
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/bb9c9ed29fdd

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/49873a7cf403
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/ed0520adaf8c

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/49873a7cf403
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/ed0520adaf8c
status-b2g-v2.1: affected → fixed
status-b2g-v2.1S: affected → fixed
status-b2g-v2.2: affected → fixed
Attachment #8477042 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
tracking-firefox-esr38: ? → 39+
Whiteboard: [b2g-adv-main2.2+]
Whiteboard: [b2g-adv-main2.2+] → [b2g-adv-main2.2+][adv-main39+][adv-esr38.1+]

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.