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.

Sign up for GitHub

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

Jump to bottom

Cannot build find-sec-bugs from local, CrlfLogInjectionDetectorTest fail #736

Open
bsellier opened this issue May 31, 2024 · 3 comments
Open

Cannot build find-sec-bugs from local, CrlfLogInjectionDetectorTest fail #736

bsellier opened this issue May 31, 2024 · 3 comments

Comments

@bsellier
Copy link

bsellier commented May 31, 2024 •
edited
Loading

Environment

Trying to build branch master of find-sec-bugs. Edit : Got same error with version-13.0.0 but version-12.0.0 works fine.

Apache Maven 3.6.3
Maven home: /usr/share/maven
Java version: 11.0.22, vendor: Ubuntu, runtime: /usr/lib/jvm/java-11-openjdk-amd64
Default locale: en, platform encoding: UTF-8
OS name: "linux", version: "5.15.133.1-microsoft-standard-wsl2", arch: "amd64", family: "unix"

Cannot build find-sec-bugs from local, CrlfLogInjectionDetectorTest fail

Similar to issue (https://github.com/find-sec-bugs/find-sec-bugs/issues/379)[#issue379].

mvn clean test-compile works fine but mvn clean install fails.

Stacktrace :

Failures:
[ERROR]   CrlfLogInjectionDetectorTest.detectResponseSplittingKotlin:66
securityReporter.doReportBug(
    BugInstance with:
bugType="CRLF_INJECTION_LOGS",
);
Wanted 29 times:
-> at com.h3xstream.findsecbugs.injection.crlf.CrlfLogInjectionDetectorTest.detectResponseSplittingKotlin(CrlfLogInjectionDetectorTest.java:66)
But was 34 times:
-> at edu.umd.cs.findbugs.AbstractBugReporter.reportBug(AbstractBugReporter.java:199)

[ERROR] Tests run: 350, Failures: 1, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for OWASP Find Security Bugs root 1.14.0-SNAPSHOT:
[INFO]
[INFO] OWASP Find Security Bugs root ...................... SUCCESS [  0.613 s]
[INFO] FindSecBugs Test Utility ........................... SUCCESS [  2.040 s]
[INFO] Find Security Bugs Samples Dependencies ............ SUCCESS [  1.132 s]
[INFO] Find Security Bugs Samples Kotlin .................. SUCCESS [  6.292 s]
[INFO] Find Security Bugs Samples Java .................... SUCCESS [  1.353 s]
[INFO] Find Security Bugs Samples Java 11 ................. SUCCESS [  0.189 s]
[INFO] Find Security Bugs Samples JSP ..................... SUCCESS [  1.352 s]
[INFO] OWASP Find Security Bugs Plugin .................... FAILURE [07:27 min]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  07:40 min
[INFO] Finished at: 2024-05-31T12:48:37 02:00
[INFO] ------------------------------------------------------------------------
@JuditKnoll
Copy link
Contributor

JuditKnoll commented Jul 3, 2024 •
edited
Loading

I also encountered this problem with a fresh workspace. It looks like it was introduced with the Kotlin version update when releasing FindSecBugs 13.0. Modifying the Kotlin version back to 1.3.72 on the current master, the build is successful on my local machine (if the changes from #740 are also applied).

@JuditKnoll
Copy link
Contributor

It still works using Kotlin 1.4.10, from Kotlin version 1.4.20 to 1.4.32 another type of build fail surfaces, and from Kotlin 1.5.0 to 2.0.0 this problem persists.
These bugreports changed (the priority changed from Medium to Low):

------------------------------------------------------
New Bug Instance: [19]
  message=SECCRLFLOG: This use of java/util/logging/Logger.logp(Ljava/util/logging/Level;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/Throwable;)V might be used to include CRLF characters into log messages
  bugType=CRLF_INJECTION_LOGS  priority=Low  category=S
  class=com.h3xstream.findsecbugs.injection.KotlinLogging  method=javaUtilLogging  line=37
------------------------------------------------------
New Bug Instance: [23]
  message=SECCRLFLOG: This use of java/util/logging/Logger.logrb(Ljava/util/logging/Level;Ljava/lang/String;Ljava/lang/String;Ljava/util/ResourceBundle;Ljava/lang/String;Ljava/lang/Throwable;)V might be used to include CRLF characters into log messages
  bugType=CRLF_INJECTION_LOGS  priority=Low  category=S
  class=com.h3xstream.findsecbugs.injection.KotlinLogging  method=javaUtilLogging  line=41
------------------------------------------------------
New Bug Instance: [24]
  message=SECCRLFLOG: This use of java/util/logging/Logger.logrb(Ljava/util/logging/Level;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;)V might be used to include CRLF characters into log messages
  bugType=CRLF_INJECTION_LOGS  priority=Low  category=S
  class=com.h3xstream.findsecbugs.injection.KotlinLogging  method=javaUtilLogging  line=42
------------------------------------------------------
New Bug Instance: [25]
  message=SECCRLFLOG: This use of java/util/logging/Logger.logrb(Ljava/util/logging/Level;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/Object;)V might be used to include CRLF characters into log messages
  bugType=CRLF_INJECTION_LOGS  priority=Low  category=S
  class=com.h3xstream.findsecbugs.injection.KotlinLogging  method=javaUtilLogging  line=43
------------------------------------------------------
New Bug Instance: [26]
  message=SECCRLFLOG: This use of java/util/logging/Logger.logrb(Ljava/util/logging/Level;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;[Ljava/lang/Object;)V might be used to include CRLF characters into log messages
  bugType=CRLF_INJECTION_LOGS  priority=Low  category=S
  class=com.h3xstream.findsecbugs.injection.KotlinLogging  method=javaUtilLogging  line=44
------------------------------------------------------

These are the new bugreports, which should not be reported:

------------------------------------------------------
New Bug Instance: [31]
  message=SECCRLFLOG: This use of java/util/logging/Logger.fine(Ljava/lang/String;)V might be used to include CRLF characters into log messages
  bugType=CRLF_INJECTION_LOGS  priority=Low  category=S
  class=com.h3xstream.findsecbugs.injection.KotlinLogging  method=javaUtilLogging  line=51
------------------------------------------------------
New Bug Instance: [32]
  message=SECCRLFLOG: This use of java/util/logging/Logger.log(Ljava/util/logging/Level;Ljava/lang/String;Ljava/lang/Object;)V might be used to include CRLF characters into log messages
  bugType=CRLF_INJECTION_LOGS  priority=Low  category=S
  class=com.h3xstream.findsecbugs.injection.KotlinLogging  method=javaUtilLogging  line=52
------------------------------------------------------
New Bug Instance: [33]
  message=SECCRLFLOG: This use of java/util/logging/Logger.logp(Ljava/util/logging/Level;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;[Ljava/lang/Object;)V might be used to include CRLF characters into log messages
  bugType=CRLF_INJECTION_LOGS  priority=Low  category=S
  class=com.h3xstream.findsecbugs.injection.KotlinLogging  method=javaUtilLogging  line=53
------------------------------------------------------
New Bug Instance: [34]
  message=SECCRLFLOG: This use of java/util/logging/Logger.logrb(Ljava/util/logging/Level;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;)V might be used to include CRLF characters into log messages
  bugType=CRLF_INJECTION_LOGS  priority=Low  category=S
  class=com.h3xstream.findsecbugs.injection.KotlinLogging  method=javaUtilLogging  line=54
------------------------------------------------------
New Bug Instance: [35]
  message=SECCRLFLOG: This use of java/util/logging/Logger.throwing(Ljava/lang/String;Ljava/lang/String;Ljava/lang/Throwable;)V might be used to include CRLF characters into log messages
  bugType=CRLF_INJECTION_LOGS  priority=Low  category=S
  class=com.h3xstream.findsecbugs.injection.KotlinLogging  method=javaUtilLogging  line=55
------------------------------------------------------

The strange thing is, that when I commented out the lines of other bugreports, the bugs were the expected ones. Tried to separate the sample code into several functions, but this didn't help solving the issue.
It looks like some taint - which were safe earlier - are now categorized as unknown, and this causes the issue.
I'm not so familiar with FindSecBugs, so if someone could take a look at it also, I would really appreciate it. If anyone has any idea, what the solution or even a good workaround might be, feel free to go ahead.

@jbindel
Copy link
Contributor

jbindel commented Nov 12, 2024 •
edited
Loading

If the checking is using the FSB taint tracing, then perhaps the newer Kotlin libraries are not compiling to the same Java methods that we would expect. The java.lang.String.toLowerCase() method passes the taint flags from the source string to the return value, and it leave the taint of the source string alone. In this Kotlin example, the taint flags of the original string are changing after the call to the Kotlin version of toLowerCase().

    val safer = "safer"
    // this is not reported
    logger.fine(safer)
    val alsoSafe = safer.toLowerCase();
    // this is not reported
    logger.fine(alsoSafe)
    // this IS reported!
    logger.fine(safer)

The disassembled code from [my modified] KotlinLogging.class shows that the newer version Kotlin inserts an extra method call into the bytecode after the call to String.toLowerCase():

  683: invokestatic  #93                 // Method kotlin/jvm/internal/Intrinsics.checkNotNullExpressionValue:(Ljava/lang/Object;Ljava/lang/String;)V

This method is not currently in any of the taint configuration files, and the taint engine doesn't know that this method doesn't change the first parameter, which is our string. We can declare this checkNotNullExpressionValue method as safe by adding this line to the taint configuration. It makes sense to add a kotlin.txt file:

- Kotlin checks objects for null. A String is immutable, but the signature is java.lang.Object, so we have to tell the engine.
kotlin/jvm/internal/Intrinsics.checkNotNullExpressionValue(Ljava/lang/Object;Ljava/lang/String;)V:UNKNOWN

(edit, I think the correct taint setting is UNKNOWN in this case though explicitly passing param 1's taint back to itself also seemed to work. when I use :1#1 instead of UNKNOWN. I'll review the taint code again before making a pull request.)

Unless someone beats me to it, I'll make a pull request that adds this configuration as soon as I get a chance. All of the other methods in that kotlin.jvm.internal.Intrinsics class should be included in the taint configuration as appropriate as well to avoid false positives from Kotlin code. The methods that check java.lang.Object should get the correct taint configurations.
https://github.com/JetBrains/kotlin/blob/master/libraries/stdlib/jvm/runtime/kotlin/jvm/internal/Intrinsics.java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@jbindel @bsellier @JuditKnoll