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.

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: Cover CVE-2021-45046 in the Log4jJndiInjection query #7423

Merged
merged 3 commits into from
Dec 16, 2021

Conversation

atorralba
Copy link
Contributor

This PR adds sinks and taint steps for covering the new attack vectors discovered in CVE-2021-45046.

Tests were added too.

@smowton
Copy link
Contributor

smowton commented Dec 16, 2021

What's going on with the removed model lines? Most seem to end with a Throwable parameter, though some Throwables remain in the revised models.

I'd strongly recommend using an inline-expectation test rather than committing a 5,000 line expectation.

@atorralba
Copy link
Contributor Author

What's going on with the removed model lines? Most seem to end with a Throwable parameter, though some Throwables remain in the revised models.

No lines were removed, it's just that I renamed the sinks from logging to log4j and that changes the formatting. Should I make a first commit only with that change, so that it's easier to see the new additions? (the new things are at the bottom of the models in any case)

I'd strongly recommend using an inline-expectation test rather than committing a 5,000 line expectation.

Yes, that was my intention, but the problem is that we can't reuse anything from the query in the test because it's in experimental, and I didn't want to duplicate everything in the test 😞

@smowton
Copy link
Contributor

smowton commented Dec 16, 2021

No lines were removed, it's just that I renamed the sinks from logging to log4j and that changes the formatting. Should I make a first commit only with that change, so that it's easier to see the new additions? (the new things are at the bottom of the models in any case)

Are you sure? The first block for example adds 5 less lines than it removes?

@atorralba
Copy link
Contributor Author

Are you sure? The first block for example adds 5 less lines than it removes?

Note that, e.g. line 57 contains two rows in one line because of the change in formatting: https://github.com/github/codeql/pull/7423/files#diff-c87c9c5a74ba94cb105e161c4d11190f6ea16f301da6e98c2779209abb3196ccR57

@smowton
Copy link
Contributor

smowton commented Dec 16, 2021

Doh right, so it does! I guess we can take care of fixing the test on promotion if that ever comes...

@smowton smowton merged commit e3b2eed into main Dec 16, 2021
@smowton smowton deleted the atorralba/log4j-CVE-2021-45046 branch December 16, 2021 16:00
"org.apache.logging.log4j;LogBuilder;true;log;(String,Object,Object,Object,Object,Object,Object,Object,Object,Object,Object);;Argument[0..10];log4j",
"org.apache.logging.log4j;LogBuilder;true;log;(String,Supplier[]);;Argument[0..1];log4j",
"org.apache.logging.log4j;LogBuilder;true;log;(Supplier);;Argument[0];log4j",
// org.apache.logging.log4j.ThreadContet
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are also CloseableThreadContext and CloseableThreadContext.Instance, in case you want to cover them as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch @Marcono1234, thanks. See #7435.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants