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

Upgrade jackson-databind to 2.13.4.2 to address CVEs #13244

Closed
wants to merge 3 commits into from

Conversation

kfaraz
Copy link
Contributor

@kfaraz kfaraz commented Oct 20, 2022

CVEs:
CVE-2022-42004
CVE-2022-42003

Changes:

  • Upgrade jackson-bom to 2.13.4.20221013
  • Remove usage of methods deleted from latest version of jackson-databind

  • been self-reviewed.
  • added or updated version, license, or notice information in licenses.yaml

@kfaraz kfaraz marked this pull request as draft October 20, 2022 18:30
@FrankChen021
Copy link
Member

FYI

There were two related PRs on this, we can close them once this is done.
#12411
#12373

And a PR before suppressed the CVE of jackson which I think we need to remove the suppression if this PR addresses the CVE:
#12535

@kfaraz
Copy link
Contributor Author

kfaraz commented Oct 22, 2022

Thanks for the tip, @FrankChen021 !
There are some backward incompatibility issues with this upgrade. I will try to get those resolved and close the older PRs.

@CookieAroundTheBend
Copy link

Hey @kfaraz I am also interested in getting past these CVE's.
What are some of the backward incompatibility issues you're seeing?

Thanks for your work on this!

@kfaraz
Copy link
Contributor Author

kfaraz commented Nov 18, 2022

Hi, @CookieAroundTheBend !
The latest version of jackson-databind has removed the method AnnotateMember.getGenericType() which was being used in the Druid code in the GuiceIntrospector. I need to find a way to make the introspector work with getRawType(), or getType(). Haven't spent much time on it though.

Please let me know if you have any ideas.

@CookieAroundTheBend
Copy link

@kfaraz We had Druid 0.20.2 working with newer Jackson where we did have to change that GuiceAnnotationIntrospector to use getRawType().

Unfortunately we are trying to apply the same patch to 0.22.1 and are seeing issues most likely stemming from that change.

Does this patch work for the latest Druid? Or are you seeing issues with what's in this branch as well?

Comment on lines +63 to 65
return Key.get(m.getType(), guiceAnnotation);
}

Choose a reason for hiding this comment

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

Suggested change
return Key.get(m.getType(), guiceAnnotation);
}
return Key.get(getParamType(m), guiceAnnotation);
}
private Type getParamType(AnnotatedMember m) {
if(m.getType().isContainerType()){
return Types.newParameterizedType(m.getType().getRawClass(), m.getType().getContentType().getRawClass());
}
return m.getRawType();
}

This seems to work for me (and of course changing the Key.get(m.getType()); to Key.get(getParamType(m)); on line 61.

But this was on version 0.22.1 and I'm not sure if this is as robust handling that could be needed.

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, @CookieAroundTheBend ! I will try to spend some time on this and get it resolved soon.

@xvrl
Copy link
Member

xvrl commented Aug 1, 2023

@kfaraz do you have any plans to push this over the finish line?

@kfaraz
Copy link
Contributor Author

kfaraz commented Aug 1, 2023

Not at the moment, @xvrl . I have been occupied with a few other tasks. I am okay if you would like to take it up.

@kfaraz
Copy link
Contributor Author

kfaraz commented Aug 8, 2023

There is a new PR #14770 which should address this.

@xvrl
Copy link
Member

xvrl commented Dec 15, 2023

@kfaraz should we close this PR?

@kfaraz
Copy link
Contributor Author

kfaraz commented Dec 16, 2023

Yes, @xvrl , we can close this for now.

@kfaraz kfaraz closed this Dec 16, 2023
@kfaraz kfaraz deleted the fix_jackson_cve branch December 16, 2023 04:06
Sign up for free to 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