Closed Bug 1595329 Opened 1 year ago Closed 1 year ago

Possible JIT bug: failure to set negative zero flag in 'ceil' operator

Categories

(Core :: JavaScript Engine: JIT, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla73
Tracking Status
firefox-esr68 --- wontfix
firefox71 --- wontfix
firefox72 --- wontfix
firefox73 --- fixed

People

(Reporter: mlfbrown, Assigned: nbp)

Details

(Keywords: sec-other, Whiteboard: [reporter-external] [web-bounty-form] [verif?][post-critsmash-triage][adv-main73-] )

Attachments

(2 files, 2 obsolete files)

We're verifying range analysis logic in IonMonkey and came across an inconsistency:
Math.ceil(-.5) = -0, but the ceil range analysis operator does not set the canBeNegativeZero flag for the result range in this case.
The relevant function is here:
https://searchfox.org/mozilla-central/source/js/src/jit/RangeAnalysis.cpp#1169

Please let us know if we're missing something, and thanks so much for your time!

Flags: sec-bounty?
Group: websites-security → core-security
Type: task → defect
Component: Other → JavaScript Engine: JIT
Product: Websites → Core
Version: unspecified → Trunk
function myCeil(x) {
    if(x >= -2) {
      if(x <= -0.5) {
        return Math.ceil(x);
      }
    }
  return 42;
}

var a = [ 10.0, 20.2, 30.3, 3.0, 0.2, -2 ];

for(var i = 0;i < 1000000; i++)
  for(var j = 0;j < a.length; j++)
    myCeil(a[j])

copy in Range::ceil has lower -2, upper 0, with canBeNegativeZero unset, so op also ends up with lower -2, upper 0, with canBeNegativeZero unset. I'm not sure if there is a bailout on -0, but even so it seems like ranges are incorrect and may be used assuming -0 is not possible. Are we missing something obvious?

Flags: needinfo?(luke)

(Forwarding)

Flags: needinfo?(luke) → needinfo?(nicolas.b.pierron)
Group: core-security → javascript-core-security

nbp: what are the security implications of this?

This is correct, there is a bug in Range::ceil which do not set the canBeNegativeZero flag.

However, this bug will not appear as a problem in MCeil code generation, as it bails on the range ]-1, -0], which includes -0.5.
Identically, this will not appear as a problem in MIntegerToInt32, as the upper bound contains 0 due to the current representation of Ranges.

In cases where the instruction is removed from the code generation, but its range was used to decide anything, such as MBeta computation and unreachable code elimination, then the ranges are computed using only the Integer ranges, which are including 0:
https://searchfox.org/mozilla-central/source/js/src/jit/RangeAnalysis.cpp#502-503,514

So far, I do not think this bug in Range Analysis leads to any bug which can be leveraged by a differential behavior of JavaScript. I will double check this last assumption by making a test case and fix this issue, as this indeed confusing and could have lead to a bug, if we had made further refinement.

Assignee: nobody → nicolas.b.pierron
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(nicolas.b.pierron)

Thanks! That makes sense. After playing around with the code my conclusion was that it can only affect other analyses. Am probably more paranoid about the redundant/bounds check elimination than DCE though.

Priority: -- → P1

Ran clang-format on this one. All js, jsapi, and jit tests pass.

Attachment #9112148 - Attachment is obsolete: true

I can't seem to modify the attachment to ask for review. (Sorry for not using moz-phab on this; can do it if ya'll want though.)

Flags: needinfo?(nicolas.b.pierron)

I should mention, I also ran this code though our verifier and it checks out.

Comment on attachment 9112154 [details] [diff] [review]
0001-Bug-1595329-Math.ceil-range-analysis-should-set-canB.patch

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

Thanks for making this patch, I will import it in phabricator and merge it (if phabricator accept that I push and land my-self)
Attachment #9112154 - Flags: review+
Flags: needinfo?(nicolas.b.pierron)
Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla73

Is there a user impact which justifies Beta backport consideration here or can this fix ride Fx73 to release (it seems pretty clearly not-needed for ESR)? Please nominate the patch for approval if so - it grafts cleanly as-landed.

Flags: needinfo?(nicolas.b.pierron)
Flags: in-testsuite+
Flags: sec-bounty? → sec-bounty-

This security bug is being publicly discussed on Twitter.
How much of a problem is this? Do we need to uplift?
https://mobile.twitter.com/tlk___/status/1202640545115463682

Sorry, I was on PTO for the last few days.

To answer comment 14 and comment 15, there should be no user affected by these changes, which is the conclusion of comment 4 and comment 5.
To paraphrase, the analysis is indeed incorrect but no execution nor optimization relies on this incorrect result in a way which would change JS execution or caused a security issue.

This bug remained hidden in case of uncertainty around the analysis.
I think we can safely open this bug, and there should not be any need for back-porting this patch.

Flags: needinfo?(nicolas.b.pierron)

I'm still a bit more paranoid. Nicolas, you're confident this can't affect DCE or RCE?

The Range::toIntegerInt32 path seems safe because of the copy->refineToExcludeNegativeZero();. I'm less confident about the MBeta case though. I'll try to look at this today.

Flags: qe-verify-
Whiteboard: [reporter-external] [web-bounty-form] [verif?] → [reporter-external] [web-bounty-form] [verif?][post-critsmash-triage]

Unless further comments indicate otherwise (in which case, ni me) I'm going to classify this as not exploitable and therefore doesn't need an advisory.

Whiteboard: [reporter-external] [web-bounty-form] [verif?][post-critsmash-triage] → [reporter-external] [web-bounty-form] [verif?][post-critsmash-triage][adv-main73-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.