"Trojan Source" vulnerability checker

[Stephan202]

The new Error Prone 2.10 release includes the UnicodeEscape bug pattern, which may guard against maliciously crafted source code containing escaped unicode characters. At Picnic this started a discussion in the context of the recently released Trojan Source research, which is instead about unescaped non-printable unicode characters. The question is: would it make sense to introduce a bug checker which flags the problematic characters mentioned in table 1 of the paper?

(Alternatively perhaps a CheckStyle rule could be more appropriate/efficient, but perhaps it's better for the Java ecosystem if both tools provided such a check.)

CC @nickboucher

---

NB: Before filing this issue I verified that Error Prone currently does not (clearly) flag the example code provided by the paper's authors. This can be seen by running the following command (note that I enabled all checks, then excluded three unrelated checks for output brevity):

javac \
  -XDcompilePolicy=simple \
  -processorpath dataflow-errorprone-3.15.0.jar:error_prone_core-2.10.0-with-dependencies.jar:jFormatString-3.0.0.jar \
  '-Xplugin:ErrorProne -XepAllDisabledChecksAsWarnings -Xep:DefaultPackage:OFF -Xep:PrivateConstructorForUtilityClass:OFF -Xep:SystemOut:OFF' \
  CommentingOut.java HomoglyphFunction.java StretchedString.java

This outputs:

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by com.google.protobuf.UnsafeUtil (file:/tmp/repro/error_prone_core-2.10.0-with-dependencies.jar) to field java.nio.Buffer.address
WARNING: Please consider reporting this to the maintainers of com.google.protobuf.UnsafeUtil
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
CommentingOut.java:4: warning: [UnusedVariable] The local variable 'isAdmin' is never read.
        boolean isAdmin = false;
                ^
    (see https://errorprone.info/bugpattern/UnusedVariable)
  Did you mean to remove this line?
StretchedString.java:5: warning: [ReferenceEquality] Comparison using reference equality instead of value equality
        if (accessLevel != "user‮ ⁦// Check if admin⁩ ⁦") {
                        ^
    (see https://errorprone.info/bugpattern/ReferenceEquality)
  Did you mean 'if (!"user‮ ⁦'?
StretchedString.java:5: warning: [StringEquality] String comparison using reference equality instead of value equality
        if (accessLevel != "user‮ ⁦// Check if admin⁩ ⁦") {
                        ^
    (see https://errorprone.info/bugpattern/StringEquality)
  Did you mean 'if (!"user‮ ⁦'?
3 warnings

As we can see:

• CommentingOut.java yields an UnusedVariable warning, but a crafty attacker can easily avoid this by introducing a legit usage.
• HomoglyphFunction.java doesn't yield a warning at all.
• StretchedString.java yields ReferenceEquality and StringEquality warnings (the latter is a special case of the former), but the attack would also work if the adversary had simply properly used .equals.

[graememorgan]

I think there are multiple things we need to do:

• Ban the scary direction-changing characters everywhere, and make that non-suppressible. (If it's suppressible, you could hide the suppression with scary characters.)
• Ban Unicode characters outside string literals and comments. That prevents homoglyph attacks.

[Stephan202]

I see you filed #2665 🚀

[Stephan202]

And part two is #2670 🚀

Tnx for the quick resolution! I guess that concludes this issue; will close it :)