Bug 230391 – DFG strength reduction on % operator should handle an INT_MIN divisor.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[wjllz]

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())) {
                    convertToIdentityOverChild1();
            }
            break;


the optimize is so simple, like this:

```c++
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:
``
std::abs(m_node->child1()->child2()->asInt32())
```

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:
```js
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....

[Radar WebKit Bug Importer]

<rdar://problem/83229740>

[wjllz]

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.

[wjllz]

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
}

[wjllz]

hi. nobody care about this?

[Brent Fulgham]

(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.

[wjllz]

(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.

[Mark Lam]

I'm currently investigating this issue.  I'm not sure it's really a security issue yet, but will find out soon.

[Mark Lam]

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.

[Mark Lam]

Thanks for the bug report.  Fix coming soon.

[Mark Lam]

Created attachment 439662 [details]
proposed patch.

[Mark Lam]

Created attachment 439674 [details]
proposed patch.

[Mark Lam]

Created attachment 439675 [details]
proposed patch.

[Robin Morisset]

Comment on attachment 439675 [details]
proposed patch.

r=me

[Mark Lam]

Comment on attachment 439675 [details]
proposed patch.

Thanks for the review.  Landing now.

[EWS]

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].