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

removeDeadNode Function Issues with Conditional Statements #90

Closed
lanvent opened this issue Sep 4, 2023 · 8 comments
Closed

removeDeadNode Function Issues with Conditional Statements #90

lanvent opened this issue Sep 4, 2023 · 8 comments
Assignees
Labels
bug Something isn't working

Comments

@lanvent
Copy link
Contributor

lanvent commented Sep 4, 2023

Description:

When running the removeDeadNode function on specific code snippets that contain conditional statements, two issues arise:

  1. For the snippet:

    var J;
    if (J==0) 
      var x,z;
    else
      var y;
    console.log("hello");

    An error is thrown indicating a failure in generating code because consequent branch of the if statement is undefined.

  2. For the snippet:

    var J;
    if (J==0) {
      var x,z;
    }
    else{
      var y;
    }
    console.log("hello");

    The output contains empty if and else blocks:

    var J;
    if (J == 0) {
    } else {
    }
    console.log('hello');

Possible Solutions:

  1. Enhance removeDeadNode to better handle conditional statements without braces.

  2. Add a new module to remove empty if and else blocks when deobfuscating, if that is considered ideal behavior.

Again, I'm not certain which approach is best, but I would appreciate your expertise on how to best solve this issue. Thank you for your continuous efforts to improve this invaluable tool.

@BenBaryoPX BenBaryoPX added the bug Something isn't working label Sep 10, 2023
@BenBaryoPX BenBaryoPX self-assigned this Sep 10, 2023
@BenBaryoPX
Copy link
Collaborator

I'm pushing a fix for this in flAST. I'll update when it's out.
Good catch, thanks!

@BenBaryoPX
Copy link
Collaborator

Btw, output containing empty if-else blocks are the intended behavior. Consider the following:

var J;
if (J = true) {}
else {}
console.log(J ? 'success' : 'failure');

The test clause can contain any type of code that might have side effects, while returning either a truthy or falsy values, so it's hard to determine if it can be safely removed. I this the deobfuscation should err on the side of caution.

@BenBaryoPX
Copy link
Collaborator

I believed this is solved by #91

@lanvent
Copy link
Contributor Author

lanvent commented Sep 11, 2023

Thank you so much for your swift response, and for your relentless efforts in refining this important tool.

I completely understand the concerns you raised about the potential side effects that could occur in the test clause. My apologies if my previous explanation was not as clear as it could be. What I was attempting to suggest was that in certain, well-defined scenarios, the conditional blocks can be safely simplified without altering the program's behavior. For example:

  • if (J == 0) {} else {} can potentially be reduced to J == 0;
  • if (J == 0) {} else { //elsebody } can be transformed to if (!(J == 0)) { //elsebody }

These changes ensure that no side-effects could occur, thus preserving the original intent of the code while also making it more succinct.

@BenBaryoPX
Copy link
Collaborator

I haven't thought of just keeping the test clause - that's a good idea!
Also, negating the if test clause to keep the else is also a good idea! I'll adopt both!.
I'm reopening this issue.
Thanks again for your attention and your great suggestions!

@BenBaryoPX BenBaryoPX reopened this Sep 11, 2023
@BenBaryoPX
Copy link
Collaborator

I believe that simplifying these if statements should be its own module, and not part of the removeDeadNodes module

@BenBaryoPX
Copy link
Collaborator

Hopefully this was solved with the new simplifyIfStatements module (#92)

@lanvent
Copy link
Contributor Author

lanvent commented Sep 11, 2023

Yes, I agree that this should be a separate new module. Thank you for your assistance. In dealing with obfuscated js files, I've also employed two approaches to simplify if statements.

For example, transforming nested if-else blocks:

if (condition1) {
  // ...
} else {
  if (condition2) {
    // ...
  } else {
    if (condition3) {
      // ...
    }
  }
}

into if-else if constructs:

if (condition1) {
  // ...
} else if (condition2) {
  // ...
} else if (condition3) {
  // ...
}

And, turning nested if conditions:

if (condition1) {
  if (condition2) {
    if (condition3) {
      // ...
    }
  }
}

into combined conditions:

if (condition1 && condition2 && condition3) {
  // ...
}

Both techniques reduce awkward indentation and improve code readability in my result. I believe these can also be incorporated into the new module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants