Skip to content

Java : Add Log Injection Vulnerability - #5099

Merged
aschackmull merged 5 commits into
mainfrom
unknown repository
Mar 24, 2021
Merged

Java : Add Log Injection Vulnerability#5099
aschackmull merged 5 commits into
mainfrom
unknown repository

Conversation

@ghost

@ghostghost commented Feb 4, 2021

Copy link
Copy Markdown

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

CC: @smowton@Marcono1234@intrigus-lgtm

@ghost

ghost commented Feb 4, 2021

Copy link
Copy Markdown
Author

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

@intrigus-lgtmintrigus-lgtm left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadjava/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qhelp Outdated
Comment threadjava/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qhelp Outdated
Comment threadjava/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qhelp Outdated
Comment threadjava/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qhelp Outdated
Comment threadjava/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qhelp Outdated
Comment threadjava/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qhelp Outdated
Comment threadjava/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qhelp Outdated
Comment threadjava/ql/src/experimental/Security/CWE/CWE-117/LogInjection.ql Outdated
Comment threadjava/ql/src/experimental/semmle/code/java/Logging.qll Outdated
owen-mc
owen-mc previously requested changes Feb 5, 2021

@owen-mcowen-mc left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadjava/ql/src/experimental/Security/CWE/CWE-117/LogInjection.java Outdated
Comment threadjava/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qhelp Outdated
Comment threadjava/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qhelp Outdated
Comment threadjava/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qhelp Outdated
Comment threadjava/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qhelp Outdated
Comment threadjava/ql/src/experimental/Security/CWE/CWE-117/LogInjection.ql Outdated
Comment threadjava/ql/src/experimental/Security/CWE/CWE-117/LogInjection.ql Outdated
Comment threadjava/ql/src/experimental/semmle/code/java/Logging.qll
Comment threadjava/ql/src/experimental/Security/CWE/CWE-117/LogInjection.ql Outdated
Comment threadjava/ql/src/experimental/Security/CWE/CWE-117/LogInjection.java Outdated
Comment threadjava/ql/src/experimental/Security/CWE/CWE-117/LogInjection.qhelp Outdated
Comment threadjava/ql/src/experimental/semmle/code/java/Logging.qll Outdated
Comment threadjava/ql/src/experimental/semmle/code/java/Logging.qll Outdated
Comment threadjava/ql/src/experimental/semmle/code/java/Logging.qll
Comment threadjava/ql/src/experimental/semmle/code/java/Logging.qll Outdated
Comment threadjava/ql/src/experimental/semmle/code/java/Logging.qll Outdated
@ghost

ghost commented Mar 2, 2021

Copy link
Copy Markdown
Author

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

Comment threadjava/ql/src/experimental/semmle/code/java/Logging.qll Outdated
Comment threadjava/ql/src/experimental/Security/CWE/CWE-117/LogInjection.ql Outdated
Comment threadjava/ql/src/experimental/semmle/code/java/Logging.qll Outdated
Comment threadjava/ql/src/experimental/semmle/code/java/Logging.qll Outdated

@owen-mcowen-mc left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadjava/ql/src/experimental/Security/CWE/CWE-117/LogInjection.ql Outdated
Comment threadjava/ql/src/experimental/Security/CWE/CWE-117/LogInjection.ql Outdated
@owen-mc

Copy link
Copy Markdown
Contributor

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.

@owen-mc

Copy link
Copy Markdown
Contributor

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

@ghost

Copy link
Copy Markdown
Author

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

@owen-mcowen-mc assigned aschackmull and unassigned owen-mcMar 18, 2021
@owen-mc
owen-mc dismissed their stale reviewMarch 18, 2021 14:27

Changes have been made

@aschackmull
aschackmull merged commit 63831cc into github:mainMar 24, 2021
@ghost
ghost deleted the javaLogInjection branch March 24, 2021 18:52
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@owen-mc@Marcono1234@aschackmull@intrigus-lgtm