Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue with soundness of HLIL control flow structuring #5201

Closed
plafosse opened this issue Mar 20, 2024 · 3 comments
Closed

Issue with soundness of HLIL control flow structuring #5201

plafosse opened this issue Mar 20, 2024 · 3 comments
Assignees
Labels
Component: Core Issue needs changes to the core Core: HLIL Issue involves High Level IL Effort: High Issue should take > 1 month Impact: High Issue adds or blocks important functionality Type: Bug Issue is a non-crashing bug with repro steps
Milestone

Comments

@plafosse
Copy link
Member

plafosse commented Mar 20, 2024

HLIL can produce unsound control flow structuring in some conditions.

Consider this MLIL code:

orig_state_exec.zip

In MLIL everything looks correct:
image

Consider the case when 'i = 0the path meansiis assigned to5and then ultimately goes to instruction21`

Now in HLIL:
image
The control flow is a series of if statements rather than if-else statements. In the case of i == 0 it meets the first condition and sets i = 5 and then can also satisfy the second condition too incorrectly setting var_20 = 1

Special Thanks to: Zao Yang and Stefan Nagy for their research in Decompiler Fuzzing for reporting this issue.

@plafosse plafosse added Type: Bug Issue is a non-crashing bug with repro steps Component: Core Issue needs changes to the core Core: HLIL Issue involves High Level IL Impact: High Issue adds or blocks important functionality labels Mar 20, 2024
@plafosse plafosse added this to the Elysium milestone Mar 20, 2024
@D0ntPanic D0ntPanic added the Effort: High Issue should take > 1 month label Apr 10, 2024
@D0ntPanic
Copy link
Member

This is exposing a fundamental issue in control flow restructuring. We have desire to revisit the control flow restructuring algorithm to significantly reduce the number of conditions that get out of hand and become unreadable, which is also an issue with the current algorithm. This bug will probably have to be fixed alongside a refactor of the control flow restructuring itself.

@xusheng6
Copy link
Member

xusheng6 commented Apr 30, 2024

A user reported a similar case:

  • BN version: 4.0.4958 Windows
  • Compiler: MSVC 2023 x64

Compile the following C code using cl /c /O2 test.c and disassemble the resulting binary in Binary Ninja:

bool test(int *input)
{
    bool result;
    if(input[0] == 1) {
        result = true;
        input[0] = 2;
        if(input[1] == 1)
            goto other;
    } else {
other:
        result = false;
    }
    return result;
}

This results in the following High Level IL output:

 int64_t test(int32_t* arg1) {
1    int64_t rax;
2    if (*arg1 == 1)
3        rax = 1;
4        *arg1 = 2;
5    if (*arg1 != 1 || (*arg1 == 1 && arg1[1] == 1))
6        rax.b = 0;
7    return rax;
 }

However, this output is not actually correct: if the function's input is { 1, 0 }, at line 4 in the HLIL, arg1[0] will be set to 2. Then, at line 5, the condition in the if statement evaluates to true (since arg1[0] now evaluates to 2), which in turn leads to line 6 being executed, and the whole function returning false. This differs from the actual behavior of the program (which should be returning true).

I see the fundamental issue is the body of the if statement is modifying the variables used in the condition, so it alters the result

@D0ntPanic
Copy link
Member

Fixed in 4.1.5547

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Core Issue needs changes to the core Core: HLIL Issue involves High Level IL Effort: High Issue should take > 1 month Impact: High Issue adds or blocks important functionality Type: Bug Issue is a non-crashing bug with repro steps
Projects
None yet
Development

No branches or pull requests

3 participants