-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Java : Add Log Injection Vulnerability #5099
Conversation
There is already a bounty application open with GHSL. See github/securitylab#144 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some comments.
The .qhelp
file has only been checked for missing <code>
tags and nothing else.
java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qhelp
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qhelp
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qhelp
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qhelp
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qhelp
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qhelp
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qhelp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to have some tests.
java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.java
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qhelp
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qhelp
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qhelp
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qhelp
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.java
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qhelp
Outdated
Show resolved
Hide resolved
0190d2a
to
9478bd7
Compare
@owen-mc I have the latest changes here. As for the tests, let this PR be merged as experimental. I keep running it issues with stubbing Java dependencies again and again. So I have decided to write a simple tool to generate the stubs for me. Until that is fully functional, I won't be adding any tests to any of my Java PR's. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have set two LGTM runs going, one with the sanitizer guards and one without. I sympathise about stubbing. For codeql for Go there is a tool called Depstubber to do stubbing.
I did two lgtm runs. There were a lot of results, and not much difference between the two runs. There were results in 7066 projects for the run with sanitizers and 7073 for the run without. Some projects also had different numbers of results. Unfortunately lgtm doesn't make it particularly easy to diff the two outputs. I stand by my suggestion about which sanitizer guards to remove. |
@porcupineyhairs Do you intend to update the sanitizer guards? I will then move this to the next stage of the process. |
Co-authored-by: Marcono1234 <Marcono1234@users.noreply.github.com>
22dca75
to
a88c368
Compare
@owen-mc I have removed the sanitizers and rebased the PR to the latest main. |
This is a continuation of @dellalibera's #3882.
CC: @smowton @Marcono1234 @intrigus-lgtm