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

[fix][sec] Bump commons-io version to 2.18.0 #23684

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

ZachChuba
Copy link
Contributor

@ZachChuba ZachChuba commented Dec 5, 2024

Fixes security risk

Motivation

Dependency common-io, shaded in pulsar-client, is susceptable to a reDos attack prior to version 2.15.0

Modifications

Bump commons-io version to 2.18.0

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: ZachChuba#2

CI only failed at upload stages

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Dec 5, 2024
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. The newest commons-io version is 2.18.0 (release notes). Please use the latest stable release.

What is the vulnerability in question?

@lhotari
Copy link
Member

lhotari commented Dec 5, 2024

My guess would be apache/commons-io@841b5fa . @ZachChuba Just curious how you spotted this? I don't think that we use commons-io for XML parsing in Pulsar, so perhaps there's no impact at all for Pulsar, however, obviously it's worthwhile to keep dependencies up-to-date.

@ZachChuba
Copy link
Contributor Author

My guess would be apache/commons-io@841b5fa . @ZachChuba Just curious how you spotted this? I don't think that we use commons-io for XML parsing in Pulsar, so perhaps there's no impact at all for Pulsar, however, obviously it's worthwhile to keep dependencies up-to-date.

@lhotari This vulnerability is indeed the one patched via apache/commons-io@841b5fa. It was identified through an internal analysis tool. It's unlikely that pulsar is susceptible to this, as you suggested, but it's always a good idea to in case an enhancement inadvertently opens an attack vector through that susceptible dependency. Will upgrade to 2.18.0.

Addresses a potential ReDos security vulnerability with commons-io
@ZachChuba ZachChuba changed the title [cleanup][build] Bump commons-io version to 2.15.0 [cleanup][build] Bump commons-io version to 2.18.0 Dec 6, 2024
@lhotari
Copy link
Member

lhotari commented Dec 6, 2024

@lhotari This vulnerability is indeed the one patched via apache/commons-io@841b5fa. It was identified through an internal analysis tool. It's unlikely that pulsar is susceptible to this, as you suggested, but it's always a good idea to in case an enhancement inadvertently opens an attack vector through that susceptible dependency. Will upgrade to 2.18.0.

@ZachChuba What internal analysis tool do you use? It seems that GitHub CodeQL had also flagged this. Just curious to know what are your specific security requirements for Apache Pulsar?

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM

@lhotari lhotari changed the title [cleanup][build] Bump commons-io version to 2.18.0 [fix][sec] Bump commons-io version to 2.18.0 Dec 6, 2024
@lhotari lhotari added this to the 4.1.0 milestone Dec 6, 2024
@ZachChuba
Copy link
Contributor Author

ZachChuba commented Dec 6, 2024

@lhotari Sonatype Lifecycle has flagged this as a high severity vulnerability. My organization's risk posture treats any vulnerabilities that can significantly hinder application performance as either high or severe. Our risk posture also assumes any shaded dependency with a vulnerability makes the application equally as vulnerable.

Sonatype has cataloged this as sonatype-2023-4396 and rated it high severity with a score of 8.7 based on our risk policy.

@lhotari
Copy link
Member

lhotari commented Dec 6, 2024

@lhotari Sonatype Lifecycle has flagged this as a high severity vulnerability. My organization's risk posture treats any vulnerabilities that can significantly hinder application performance as either high or severe.

Sonatype has cataloged this as sonatype-2023-4396 and rated it high severity with a score of 8.7 based on our risk policy.

Thanks for sharing the context!

@codecov-commenter
Copy link

codecov-commenter commented Dec 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.43%. Comparing base (bbc6224) to head (69ce162).
Report is 780 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23684      +/-   ##
============================================
+ Coverage     73.57%   74.43%   +0.85%     
- Complexity    32624    34575    +1951     
============================================
  Files          1877     1945      +68     
  Lines        139502   147448    +7946     
  Branches      15299    16270     +971     
============================================
+ Hits         102638   109747    +7109     
- Misses        28908    29233     +325     
- Partials       7956     8468     +512     
Flag Coverage Δ
inttests 27.26% <ø> (+2.67%) ⬆️
systests 24.36% <ø> (+0.04%) ⬆️
unittests 73.80% <ø> (+0.96%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 672 files with indirect coverage changes

@lhotari lhotari merged commit e6f421e into apache:master Dec 6, 2024
56 of 61 checks passed
@lhotari
Copy link
Member

lhotari commented Dec 6, 2024

Thanks for contributing @ZachChuba

lhotari pushed a commit that referenced this pull request Dec 6, 2024
Co-authored-by: Zach Chuba <[email protected]>
(cherry picked from commit e6f421e)
lhotari pushed a commit that referenced this pull request Dec 6, 2024
Co-authored-by: Zach Chuba <[email protected]>
(cherry picked from commit e6f421e)
lhotari pushed a commit that referenced this pull request Dec 6, 2024
Co-authored-by: Zach Chuba <[email protected]>
(cherry picked from commit e6f421e)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Dec 9, 2024
Co-authored-by: Zach Chuba <[email protected]>
(cherry picked from commit e6f421e)
(cherry picked from commit f16cf67)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Dec 10, 2024
Co-authored-by: Zach Chuba <[email protected]>
(cherry picked from commit e6f421e)
(cherry picked from commit f16cf67)
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.

3 participants