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

Java : Add Log Injection Vulnerability #5099

Merged

aschackmull merged 5 commits into github:main from porcupineyhairs:javaLogInjection

Mar 24, 2021

Contributor

porcupineyhairs commented Feb 4, 2021

This is a continuation of @dellalibera's #3882.

CC: @smowton @Marcono1234 @intrigus-lgtm

porcupineyhairs requested a review from a team as a code owner

February 4, 2021 18:21

github-actions bot added documentation Java labels

smowton assigned owen-mc

Contributor Author

porcupineyhairs commented Feb 4, 2021

There is already a bounty application open with GHSL. See github/securitylab#144

This was referenced Feb 4, 2021

[Java] CWE-117: CodeQL query to detect Log Injection github/securitylab#144

Open

Java : Add query for detecting Log Injection vulenrabilities github/securitylab#265

Closed

intrigus-lgtm reviewed

View reviewed changes

Contributor

intrigus-lgtm left a comment

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

java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.ql Outdated Show resolved Hide resolved

java/ql/src/experimental/semmle/code/java/Logging.qll Outdated Show resolved Hide resolved

owen-mc previously requested changes

View reviewed changes

Contributor

owen-mc left a comment

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.ql Outdated Show resolved Hide resolved

java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.ql Outdated Show resolved Hide resolved

java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.ql Show resolved Hide resolved

java/ql/src/experimental/semmle/code/java/Logging.qll Show resolved Hide resolved

java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.ql Outdated Show resolved Hide resolved

Marcono1234 reviewed

View reviewed changes

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/semmle/code/java/Logging.qll Outdated Show resolved Hide resolved

java/ql/src/experimental/semmle/code/java/Logging.qll Outdated Show resolved Hide resolved

Marcono1234 reviewed

View reviewed changes

java/ql/src/experimental/semmle/code/java/Logging.qll Show resolved Hide resolved

java/ql/src/experimental/semmle/code/java/Logging.qll Outdated Show resolved Hide resolved

java/ql/src/experimental/semmle/code/java/Logging.qll Outdated Show resolved Hide resolved

porcupineyhairs force-pushed the javaLogInjection branch from 0190d2a to 9478bd7 Compare

March 2, 2021 18:44

Contributor Author

porcupineyhairs commented Mar 2, 2021

@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.

Marcono1234 reviewed

View reviewed changes

java/ql/src/experimental/semmle/code/java/Logging.qll Outdated Show resolved Hide resolved

Marcono1234 reviewed

View reviewed changes

java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.ql Outdated Show resolved Hide resolved

Marcono1234 reviewed

View reviewed changes

java/ql/src/experimental/semmle/code/java/Logging.qll Outdated Show resolved Hide resolved

java/ql/src/experimental/semmle/code/java/Logging.qll Outdated Show resolved Hide resolved

owen-mc reviewed

View reviewed changes

Contributor

owen-mc left a comment

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.

java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.ql Outdated Show resolved Hide resolved

java/ql/src/experimental/Security/CWE/CWE-117/LogInjection.ql Outdated Show resolved Hide resolved

Contributor

owen-mc commented Mar 10, 2021

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. CruxFramework/crux is one of the projects that had results in the second run but not the first. I looked at one of its results and it seems to be inappropriately sanitizer-guarded by endswith. The rest of the results for that repo are very similar, so I think the same must be the case.

I stand by my suggestion about which sanitizer guards to remove.

Contributor

owen-mc commented Mar 15, 2021

@porcupineyhairs Do you intend to update the sanitizer guards? I will then move this to the next stage of the process.

porcupineyhairs and others added 5 commits

March 18, 2021 16:12


          Add Log Injection Vulnerability

17d7ba8


          Add flogger and android logging support

d0c82d3


          Update java/ql/src/experimental/semmle/code/java/Logging.qll

f27d2bd

Co-authored-by: Marcono1234 <[email protected]>


          Include suggestions from review

84c9137


          remove sanitiserGuards

a88c368

porcupineyhairs force-pushed the javaLogInjection branch from 22dca75 to a88c368 Compare

March 18, 2021 10:42

Contributor Author

porcupineyhairs commented Mar 18, 2021

@owen-mc I have removed the sanitizers and rebased the PR to the latest main.

owen-mc assigned aschackmull and unassigned owen-mc

owen-mc dismissed their stale review

March 18, 2021 14:27

Changes have been made

aschackmull approved these changes

View reviewed changes

aschackmull merged commit 63831cc into github:main

porcupineyhairs deleted the javaLogInjection branch

March 24, 2021 18:52

This was referenced Nov 4, 2021

[Java] CodeQL query to detect Log Injection #3882

Closed

Java: Promote Log Injection from experimental #7054

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Java