PerimeterX/restringer

`removeDeadNode` Function Issues with Conditional Statements

Closed this issue · 8 comments

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.

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

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.

I believed this is solved by #91

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.

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!

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

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

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.