Skip to content

Fixes for CVEs#4383

Closed
nookala wants to merge 4 commits intocloudfoundry:mainfrom
nookala:tnz-may-cves
Closed

Fixes for CVEs#4383
nookala wants to merge 4 commits intocloudfoundry:mainfrom
nookala:tnz-may-cves

Conversation

@nookala
Copy link
Copy Markdown
Contributor

@nookala nookala commented May 30, 2025

GHSA-vmwr-mc7x-5vc3
GHSA-gjh7-p2fx-99vx
GHSA-2gw2-8q9w-cw8p

Thanks for contributing to cloud_controller_ng. To speed up the process of reviewing your pull request please provide us with:

  • A short explanation of the proposed change:

  • An explanation of the use cases your change solves

  • Links to any other associated PRs

  • I have reviewed the contributing guide

  • I have viewed, signed, and submitted the Contributor License Agreement

  • I have made this pull request to the main branch

  • I have run all the unit tests using bundle exec rake

  • I have run CF Acceptance Tests

Copy link
Copy Markdown
Member

@moleske moleske left a comment

Choose a reason for hiding this comment

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

I'm confused. Two of these dependencies are being added to the gemfile and aren't used anywhere. And rack is at 2.2.15 which is higher than the cve you're referencing

@nookala
Copy link
Copy Markdown
Contributor Author

nookala commented Jun 2, 2025

I'm confused. Two of these dependencies are being added to the gemfile and aren't used anywhere. And rack is at 2.2.15 which is higher than the cve you're referencing

ffi already exists in the Gemfile.lock as a dependency ffi (1.16.3)

ffi (1.16.3)

rexml is also an existing dependency, but is 3.3.9 so I could pin it to >= 3.3.9
rexml (>= 3.3.9)

I'm confused. Two of these dependencies are being added to the gemfile and aren't used anywhere. And rack is at 2.2.15 which is higher than the cve you're referencing
Agreed on rack, could explicitly pin to >= 2.2.15

@moleske
Copy link
Copy Markdown
Member

moleske commented Jun 3, 2025

ffi already exists in the Gemfile.lock as a dependency ffi (1.16.3)

This is why I'm confused, it is already brought in transitively and is updated. Adding it to the gemfile I don't believe helps cause the transitive dependency will bring in whatever version it needs. Same for the other dependencies. I also see no issue in the github security checks.

I'll let someone who knows ruby dependency management better than I (hi @sethboyles!) chime in to correct me if I'm misunderstanding

@nookala
Copy link
Copy Markdown
Contributor Author

nookala commented Jun 3, 2025

ffi already exists in the Gemfile.lock as a dependency ffi (1.16.3)

This is why I'm confused, it is already brought in transitively and is updated. Adding it to the gemfile I don't believe helps cause the transitive dependency will bring in whatever version it needs. Same for the other dependencies. I also see no issue in the github security checks.

I'll let someone who knows ruby dependency management better than I (hi @sethboyles!) chime in to correct me if I'm misunderstanding
As per my understanding the gem version is to be added in the Gemfile to override a transitive dependency https://docs.veracode.com/r/Fix_Example_Transitive_Vulnerability_for_Ruby
bundle show after the change returns

  • rack (2.2.16)
  • rexml (3.4.1)
  • ffi (1.16.3)

@sethboyles
Copy link
Copy Markdown
Member

I think you can run bundle update --conservative rexml ffi rack to update just those gems without adding to the Gemfile.

it won't update rexml because it's already at 3.4.1. Seems like a false alarm from the CVE scanner

@sethboyles
Copy link
Copy Markdown
Member

I misread your comment @nookala since the formatting makes it look like a quote. It's unfortunate that Veracode (is that the scanner Bcom uses?) recommends adding the gems to the gemfile directly, because it makes it less clear what gems are actually being used directly by CCNG.

I'd suggest trying the validate with the --conservative option I suggested above. If that doesn't work, and we actually need to add to the Gemfile, maybe we can put transitive security fixes in their own section lower in the file with a comment explaining they are only explicitly set to satisfy a CVE scanner.

@moleske
Copy link
Copy Markdown
Member

moleske commented Jun 4, 2025

Though I don't know the report being referenced, I think it is all false positives

Some of dependencies that bring in ffi, rexml, and rack say that lower versions of those dependencies are acceptable, but the lock file is not bringing in those lower versions

here are the links to the dependabot security updates that fixed some of the issues referenced in PR. You'll only be able to see these if you are an approver in the capi project though. I couldn't find one for ffi, but the ffi cve referenced is from 2018 and has been fixed for awhile

edit - found the ffi bump to resolve this cve from 2018. It was done before the advisory was released, so no dependabot security note for it

@nookala
Copy link
Copy Markdown
Contributor Author

nookala commented Jun 4, 2025

Though I don't know the report being referenced, I think it is all false positives

Some of dependencies that bring in ffi, rexml, and rack say that lower versions of those dependencies are acceptable, but the lock file is not bringing in those lower versions

here are the links to the dependabot security updates that fixed some of the issues referenced in PR. You'll only be able to see these if you are an approver in the capi project though. I couldn't find one for ffi, but the ffi cve referenced is from 2018 and has been fixed for awhile

edit - found the ffi bump to resolve this cve from 2018. It was done before the advisory was released, so no dependabot security note for it

Thanks @moleske and @sethboyles I've marked the CVEs as false positives.

@nookala nookala closed this Jun 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants