Bug 230391 - DFG strength reduction on % operator should handle an INT_MIN divisor.
Summary: DFG strength reduction on % operator should handle an INT_MIN divisor.
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
Keywords: InRadar
Depends on:
Reported: 2021-09-17 00:20 PDT by wjllz
Modified: 2021-09-29 22:27 PDT (History)
10 users (show)

See Also:

proposed patch. (3.29 KB, patch)
2021-09-29 14:40 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (3.29 KB, patch)
2021-09-29 15:58 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (3.25 KB, patch)
2021-09-29 16:00 PDT, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description wjllz 2021-09-17 00:20:57 PDT
Hi, I want to report a bug to u. This is the first time I try to report a bug. So if anything is wrong, I will sorry about this.

The bug at |DFGStrengthReductionPhase|, when it deal ArithMod instruction(https://github.com/WebKit/WebKit/blob/main/Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp#L191):

        case ArithMod:
            // On Integers
            // In: ArithMod(ArithMod(x, const1), const2)
            // Out: Identity(ArithMod(x, const1))
            //     if const1 <= const2.
            if (m_node->binaryUseKind() == Int32Use
                && m_node->child2()->isInt32Constant()
                && m_node->child1()->op() == ArithMod
                && m_node->child1()->binaryUseKind() == Int32Use
                && m_node->child1()->child2()->isInt32Constant()
                && std::abs(m_node->child1()->child2()->asInt32()) <= std::abs(m_node->child2()->asInt32())) {

the optimize is so simple, like this:

let x1 = n % 10;   // [+] @a
let x2 = x1 % 14;  // [+] @b

at here, @a we could ensure x1 < 10, then we know 10 < 14, so we think we could remove @b.

However, the wrong part is this:

assume |m_node->child1()->child2()->asInt32()| value is |INT_MIN|, |@x: "-2147483648"|, we should got |@y: "2147483648"|, @y > INT_MAX. so interger overflow will happend.

## poc part

My poc like this, It could trigger in safari newest version(14.1.2). 

``` js
let hot_count = 0x1000000;

function jit(num){
    num |= 0;   //  [+] without this, num will be (SpecNonBoolInt32 | SpecNonIntAsDouble), which won't got this
    let x1 = num % -2147483648;
    let x2 = x1 %  5;
    //  [+] here is emmmmm...
    if(x2 > 9){   //  [+] shold be here...
        print("[+] magic happend, sir!");
        return x2;  //  [+] I am already calc
    return x1 + x2;

for(let i = 0; i < hot_count; i++){
    jit(-(i % 10));

let MAGIC_INT = 10;    //  [+] if |0x80000000| be thought as a INT, which should be INT_MIN
let res = jit(MAGIC_INT);
print("[+] the result is: " + res);    //    [+] should be 0x80000000, error output -2147483648

if u run the poc, you could see x2 is |10|, obviously it should be 0(10 % 5 == 0).

## exploit part

I am so sorry, I don't archive exploit part. I try to use this bug to eliminate boundCheck, but Finally I don't find a way.

The DFGInterger range analysis part use |executeNode| to determine value's range, but ArithMod aren't be here. I did some source code review, and found seems, if we have a code like this:
let x = n % 100;

Seems jsc can't aware that x will be: (x > -100 %% x < 100).

But the bug seems as this two bug(and another v8's bug), So I decide to report it as a security vulnerablity:

-   @1: https://googleprojectzero.blogspot.com/2020/09/jitsploitation-one.html
-   @2: https://bugs.webkit.org/show_bug.cgi?id=229869

@2 seems is found by apple self, seems It is hard to found it by fuzz, I have no idea How can Convert it to a security issue, maybe the bug report know that. But I have not ability to access it.

## another

I am sorry for my bad English, sorry....
Comment 1 Radar WebKit Bug Importer 2021-09-17 00:21:13 PDT
Comment 2 wjllz 2021-09-17 00:28:49 PDT
The js code has some commit in it... which is wrong, I just changed another bug's poc, and forget delete it. I am so sorry for that part. But the bug is so simple(maybe...), hope it won't confuse u.
Comment 3 wjllz 2021-09-17 02:32:29 PDT
By the way, the patch for the bug is simple, add a check like this(https://github.com/WebKit/WebKit/blob/main/Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp#L166).

if(m_node->child1()->child2()->asInt32() == INT_MIN){
       break;  // [+] don't optimize it, otherwise it will trigger integer overflow
Comment 4 wjllz 2021-09-22 19:07:42 PDT
hi. nobody care about this?
Comment 5 Brent Fulgham 2021-09-22 19:23:21 PDT
(In reply to wjllz from comment #4)
> hi. nobody care about this?

We are looking into it and will respond soon. I apologize for the delay.
Comment 6 wjllz 2021-09-22 22:15:03 PDT
(In reply to Brent Fulgham from comment #5)
> (In reply to wjllz from comment #4)
> > hi. nobody care about this?
> We are looking into it and will respond soon. I apologize for the delay.

It's ok, It's my wrong. 

This is my first time try to report bug to safari, so I don't know I find the right way to report it. So I ask this question(comment 4)...

I don't find any useful information about webkit's bounty system... so It make me confusion.

Thanks for your reply.
Comment 7 Mark Lam 2021-09-28 12:20:46 PDT
I'm currently investigating this issue.  I'm not sure it's really a security issue yet, but will find out soon.
Comment 8 Mark Lam 2021-09-29 11:12:43 PDT
This is not a security issue because there's no way to use the resultant integer to access memory.  Any memory access based on the resultant integer thereafter will still do the needed bounds checks.
Comment 9 Mark Lam 2021-09-29 14:33:15 PDT
Thanks for the bug report.  Fix coming soon.
Comment 10 Mark Lam 2021-09-29 14:40:26 PDT
Created attachment 439662 [details]
proposed patch.
Comment 11 Mark Lam 2021-09-29 15:58:51 PDT
Created attachment 439674 [details]
proposed patch.
Comment 12 Mark Lam 2021-09-29 16:00:37 PDT
Created attachment 439675 [details]
proposed patch.
Comment 13 Robin Morisset 2021-09-29 16:02:58 PDT
Comment on attachment 439675 [details]
proposed patch.

Comment 14 Mark Lam 2021-09-29 22:11:23 PDT
Comment on attachment 439675 [details]
proposed patch.

Thanks for the review.  Landing now.
Comment 15 EWS 2021-09-29 22:27:54 PDT
Committed r283300 (242325@main): <https://commits.webkit.org/242325@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 439675 [details].