Closed Bug 1248851 Opened 4 years ago Closed 4 years ago

Add static analysis for UniquePtr::release() to ensure the result is always used

Categories

(Core :: MFBT, defect)

40 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox44 --- wontfix
firefox45 + fixed
firefox46 + fixed
firefox47 + fixed
firefox-esr38 45+ fixed
firefox-esr45 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

(Keywords: regression, sec-high, Whiteboard: [post-critsmash-triage][adv-main45-][adv-esr38.7-])

Attachments

(5 files, 1 obsolete file)

The name of UniquePtr::release() doesn't quite reflect what it actually does intuitively to me, and I believe I'm not alone here.

Given this name is inherited from the STL's unique_ptr, we probably shouldn't rename it. But we should at least add a static analysis to ensure its return value is always used so that no trivial leak happens.

(Probably ASAN build is already able to detect this kind of trivial leak? But static analysis is probably more reliable than ASAN I suppose.)
There's a warn-unused-result macro somewhere that would give build warnings if the result is ignored.
(In reply to :Ms2ger from comment #1)
> There's a warn-unused-result macro somewhere that would give build warnings
> if the result is ignored.

MOZ_WARN_UNUSED_RESULT. Note that this does not do anything on MSVC.
(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #0)
> (Probably ASAN build is already able to detect this kind of trivial leak?
> But static analysis is probably more reliable than ASAN I suppose.)

Yes, LSan can detect leaks like that. Of course, it requires a test to actually hit that case.
(In reply to Andrew McCreight [:mccr8] from comment #2)
> (In reply to :Ms2ger from comment #1)
> > There's a warn-unused-result macro somewhere that would give build warnings
> > if the result is ignored.
> 
> MOZ_WARN_UNUSED_RESULT. Note that this does not do anything on MSVC.

Yeah, that works, thanks! I though we have such macro, but only found MOZ_MUST_USE.

And after applying this macro, I immediately found an issue in our code :)
Not actually an issue, though.
It seems the ownership management in the js code is pretty complicated... Many functions take the ownership in some condition but do not take it otherwise... Making a bunch of explicit release() call without using the result.
Comment on attachment 8720637 [details]
MozReview Request: Bug 1248851 part 2 - Remove redundant release() calls in indexedDB code.

https://reviewboard.mozilla.org/r/35397/#review32059
Comment on attachment 8720637 [details]
MozReview Request: Bug 1248851 part 2 - Remove redundant release() calls in indexedDB code.

https://reviewboard.mozilla.org/r/35397/#review32061
Attachment #8720637 - Flags: review+
Comment on attachment 8720638 [details]
MozReview Request: Bug 1248851 part 3 - Fix a potential double-free issue in indexedDB.

https://reviewboard.mozilla.org/r/35399/#review32063

::: dom/indexedDB/ActorsParent.cpp:18847
(Diff revision 1)
>      rv = updateStmt->BindAdoptedBlobByName(indexDataValuesString,

Will BindAdoptedBlobByName really take ownership of the buffer even if the bind fails?
https://reviewboard.mozilla.org/r/35399/#review32063

> Will BindAdoptedBlobByName really take ownership of the buffer even if the bind fails?

It seems to me yes.

There is only one implementation of BindAdoptedBlobByName which is BindingParams::BindAdoptedBlobByName. It can be seen that this function creates an AdoptedBlobVariant which takes the ownership, which is managed by a nsCOMPtr. The variant object is then passed into BindByName via a weak interface pointer (nsIVariant*), which indicates its ownership is not transfered further, and that method is not going to touch the internal data directly.

If BindByName method would ever leak, the Variant object is leaked as well. Releasing only the data inside isn't the right solution in that case. Anyway, tracing down the code path shows that the ownership of the buffer is always taken. Please check the code and see whether I'm right here.
Attachment #8720638 - Flags: review?(jonas)
Comment on attachment 8720638 [details]
MozReview Request: Bug 1248851 part 3 - Fix a potential double-free issue in indexedDB.

https://reviewboard.mozilla.org/r/35399/#review32219
Attachment #8720638 - Flags: review?(jonas) → review+
Comment on attachment 8720636 [details]
MozReview Request: Bug 1248851 part 1 - Explicitly mark some release() calls result-unused.

https://reviewboard.mozilla.org/r/35393/#review32253
Attachment #8720636 - Flags: review?(jwalden+bmo) → review+
Attachment #8720639 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8720639 [details]
MozReview Request: Bug 1248851 part 4 - Mark UniquePtr::release() MOZ_WARN_UNUSED_RESULT.

https://reviewboard.mozilla.org/r/35401/#review32255
We really need to get some move-only typing added to those JS functions at some point, so we don't have to have explicit releases far away from the actual consumption of those values in the JS engine.  :-\
Double-frees are sec-critical, so this should be hidden.  I suppose it's a mitigation that the file name *suggests* it requires e10s to hit, but we ship with that on to a ton of developers, and surely we're not okay with subjecting them to such risk.
Group: core-security
Keywords: sec-critical
Comment on attachment 8720638 [details]
MozReview Request: Bug 1248851 part 3 - Fix a potential double-free issue in indexedDB.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not sure. :sicking, do you know how easily could a script make BindAdoptedBlobByName return failure?
Note that, since I wasn't aware that double-free is security-critical, the patch has actually been exposed (on reviewboard and try server), and this bug has been public for a while. Sorry about that.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
It mentions "double-free".

Which older supported branches are affected by this flaw?
If not all supported branches, which bug introduced the flaw?
It seems this code was introduced since bug 1131776 which landed on Firefox 40. So assumably all current branches.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This single patch can be uplifted directly.

How likely is this patch to cause regressions; how much testing does it need?
This patch would at most cause memory leak in case I was wrong. Not sure how serious could this leak be. This would also a question for :sicking.
Flags: needinfo?(jonas)
Attachment #8720638 - Flags: sec-approval?
The right way to fix bindAdoptedBlobByName is probably passing UniquePtr&& into it.
(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #22)
> The right way to fix bindAdoptedBlobByName is probably passing UniquePtr&&
> into it.

Errrr, but DatabaseOperationBase uses a special deleter which calls "free" rather than delete[]... Probably the current solution is the best, then.
The amount of data leaked will likely not be very large. But it could potentially add up quickly if a website does a lot of IndexedDB data modifications.

I don't know exactly under which circumstances this function fails. There three cases I can think of:
* If there's an IO error when trying to update an index. This would be very hard to reproduce, and only happen quite rarely in practice.
* If there's a quota error when trying to update an index. This would also be quite hard to trigger reliably, but more doable. We could create an objectstore and then populate it with a lot of data and commit this in a transaction. We would then measure the filesize and set the quota limit to a little higher than the current filesize (say 1MB higher). We could then create a separate transaction which creates a new index. If the objectstore contains enough data, and we could make sure that this grows the database by significantly more than 1MB, which should trigger a quota error.
* When trying to insert data which violates a unique index. This should be trivial to do, and in fact there are already tests which do this.

So if we want to test this, the easiest thing to do is likely to set a breakpoint and then run through all the indexedDB tests. That should verify if the last bullet actually triggers this error condition.

Beyond that I'm not sure what's worth doing.


One relevant question is, what do other IndexedDB calls of BindAdoptedBlobByName do? Do they assume that BindAdoptedBlobByName always take ownership of the passed in buffer?
(In reply to Jonas Sicking PTO until Feb 16th (:sicking) from comment #24)
> One relevant question is, what do other IndexedDB calls of
> BindAdoptedBlobByName do? Do they assume that BindAdoptedBlobByName always
> take ownership of the passed in buffer?

There are two callsites overall, both in ActorsParent. And the other one... It also frees the buffer on failure, which seems to be another double-free. [1] We probably want to fix this as well.

[1] https://dxr.mozilla.org/mozilla-central/rev/d5daf10d3b74b04e8fa63c4e5429de8a0adf79f8/dom/indexedDB/ActorsParent.cpp#24456
FWIW, in one case this will actually leak, which is when aValueSize > INT_MAX, so BindingParams::BindAdoptedBlobByName returns even earlier than the ownership transfering.

This is probably fine as I don't think it is expected to handle an array > 2GB correctly. This could be fixed if we pass UniquePtr to it anyway.
[Security approval request comment]
See comment 21 and comment 24.
Attachment #8721298 - Flags: sec-approval?
Attachment #8721298 - Flags: review?(jonas)
Attachment #8720638 - Attachment is obsolete: true
Attachment #8720638 - Flags: sec-approval?
Attachment #8720638 - Flags: review+
Comment on attachment 8721298 [details] [diff] [review]
part 3 - Fix a potential double-free issue in indexedDB.

sec-approval+.

We should take this on Aurora and Beta. Please nominate patches there before Monday so we can make the beta builds next week.

Marking ESR38 as unaffected based on comment 21.
Attachment #8721298 - Flags: sec-approval? → sec-approval+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #20)
> Double-frees are sec-critical, so this should be hidden.

They can be exploited, but usually require the attacker to be able to manipulate memory between the two frees. Without a testcase to show we can hit that window we usually call it sec-high.
Blocks: 1131776
Group: core-security → core-security-release
(In reply to Daniel Veditz [:dveditz] from comment #29)
> They can be exploited, but usually require the attacker to be able to
> manipulate memory between the two frees. Without a testcase to show we can
> hit that window we usually call it sec-high.

Really?  That seems unfortunate.  I thought we adhered to the principle that we would assume memory safety violations were exploitable until proven otherwise, and so we would cautiously assume the worst unless it was very easy to demonstrate execution would safely end (with, say, a constant-near-null deref or so).  :-\
I cannot access my patches on MozReview anymore... Are they removed or made forbidden for public access?
Assignee: nobody → quanxunzhen
Comment on attachment 8721298 [details] [diff] [review]
part 3 - Fix a potential double-free issue in indexedDB.

Approval Request Comment
[Feature/regressing bug #]: bug 1131776
[User impact if declined]: potential double-free
[Describe test coverage new/current, TreeHerder]: unknown
[Risks and why]: if the fix is wrong, it could lead to memory leak for some indexedDB manipulation
[String/UUID change made/needed]: n/a
Attachment #8721298 - Flags: approval-mozilla-beta?
Attachment #8721298 - Flags: approval-mozilla-aurora?
Wait, the issue fixed by the second half of the patch was introduced in bug 994190, which was landed in 35. It indicates 38 ESR may be affected as well.
Comment on attachment 8721298 [details] [diff] [review]
part 3 - Fix a potential double-free issue in indexedDB.

No, it was bug 979445 (in mozilla30) since the introduction of BindAdoptedBlobByName.

:khuey, I think it's worth an additional review from you to part 3, since you wrote both parts of the code.
Attachment #8721298 - Flags: review?(khuey)
(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #31)
> I cannot access my patches on MozReview anymore... Are they removed or made
> forbidden for public access?

There's an automated warning system that tells IT when a bug with MozReviews on it is closed, and they go and delete everything at that point:

[2016-02-19 14:32:29] <Waldo> gps: does mozreview have any ability to enforce access controls of any sort?  if not, is such planned soon?
[2016-02-19 14:33:46] <gps> Waldo: if you are talking about bugzilla-like secure bug access, that is out of scope for a long time
[2016-02-19 14:33:58] <Waldo> booooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo
[2016-02-19 14:34:03] <Waldo> ...ooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo
[2016-02-19 14:34:31] <gps> there are checks that prevent you from publishing to non-public bugs
[2016-02-19 14:35:01] <Waldo> gps: you know that's going to come back to bite us eventually, when someone doesn't know/think to mark a bug s-s and then posts patches
[2016-02-19 14:35:40] <gps> there is a mechanism for this. we get notified when a bug becomes secure and mozreview+hg data is deleted
[2016-02-19 14:35:49] <Waldo> hm
[2016-02-19 14:37:08] <Waldo> I guess that's at least not horrific

Not ideal, but at least it's not public forever in that case.
Attachment #8721298 - Flags: review?(khuey) → feedback?(khuey)
Comment on attachment 8721298 [details] [diff] [review]
part 3 - Fix a potential double-free issue in indexedDB.

sec-high, taking it.
Attachment #8721298 - Flags: approval-mozilla-beta?
Attachment #8721298 - Flags: approval-mozilla-beta+
Attachment #8721298 - Flags: approval-mozilla-aurora?
Attachment #8721298 - Flags: approval-mozilla-aurora+
Given comment 35, we probably want to uplift part of this patch to ESR 38 as well. I'd like to hear from khuey first.
Kyle, should we uplift this to ESR38? Xidorn is also wondering in comment 40.
Flags: needinfo?(khuey)
Comment on attachment 8721298 [details] [diff] [review]
part 3 - Fix a potential double-free issue in indexedDB.

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

Ah, I see.  We execute the statements before the unique_ptr expires which is why these work.
Attachment #8721298 - Flags: feedback?(khuey) → feedback+
Yeah, we should take it.
Flags: needinfo?(khuey)
(In reply to Ritu Kothari (:ritu) from comment #42)
> Kyle, should we uplift this to ESR38? Xidorn is also wondering in comment 40.

ESR needs a different patch. I'll submit it.
Flags: needinfo?(quanxunzhen)
[Approval Request Comment]
sec-high, other fields see comment 33.

// This is actually less different than I thought. Oh, the big code movement happened in mozilla35, so...
Flags: needinfo?(quanxunzhen)
Attachment #8722329 - Flags: approval-mozilla-esr38?
Comment on attachment 8722329 [details] [diff] [review]
patch for esr, r=sicking

sec-high that also needs to be uplifted to esr38.7
Attachment #8722329 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Carsten, Wes: Could you please uplift this to esr38 branch? Thanks!
Flags: needinfo?(wkocher)
Flags: needinfo?(cbook)
has problems uplifting to esr38 

grafting 329958:8441fa1a493b "Bug 1248851 - r=sicking, a=sylvestre"
merging dom/indexedDB/ActorsParent.cpp
warning: conflicts while merging dom/indexedDB/ActorsParent.cpp! (edit, then use 'hg resolve --mark')


xidorn, could you take a look, thanks!
Flags: needinfo?(cbook) → needinfo?(quanxunzhen)
(In reply to Carsten Book [:Tomcat] from comment #49)
> xidorn, could you take a look, thanks!

Please use the patch I requested uplift to esr38 on, which is attachment 8722329 [details] [diff] [review], rather than picking it from beta.
Flags: needinfo?(quanxunzhen)
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main45-][adv-esr38.7-]
Flags: in-testsuite?
Version: unspecified → 40 Branch
Group: core-security-release
Duplicate of this bug: 1237712
You need to log in before you can comment on or make changes to this bug.