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 hashbrown to fix security vulnerability #13622

Merged
merged 2 commits into from
Jan 8, 2025

Conversation

chookity-pokk
Copy link
Contributor

@chookity-pokk chookity-pokk commented Jan 7, 2025

Summary

indexmap versions 2.7.0 and 2.6.0 pull in hashbrown 0.15.0 which has a high severity vulnerability. This just downgrades to a version that doesn't have that vulnerability.

Details and comments

Just a simple security fix PR.

@chookity-pokk chookity-pokk requested a review from a team as a code owner January 7, 2025 22:11
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Jan 7, 2025
@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the following people are relevant to this code:

  • @Qiskit/terra-core

@CLAassistant
Copy link

CLAassistant commented Jan 7, 2025

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@jakelishman
Copy link
Member

Thanks for doing this. Dependabot should have done it for us, but it's having a minor problem because of some package constraints we've put on it. I had a half-hearted attempt to fix Dependabot earlier today, but it was the end of my work day, and I was just leaving it for tomorrow.

Can we not update the locked version of hashbrown to 0.15.1, which resolves the problem?

@jakelishman
Copy link
Member

I'm not at my computer, but iirc cargo update -p [email protected] is the right command.

@chookity-pokk
Copy link
Contributor Author

I'm not at my computer, but iirc cargo update -p [email protected] is the right command.

I tried that originally but the newest hashbrown has breaking changes for qiskit that didn't seem simple to solve so I chose to downgrade indexmap.

@jakelishman
Copy link
Member

Even just bumping the 0.15.0 versions in the lock files we don't use directly? Bumping the direct requirement on 0.14.5 will cause problems because it's used in interfaces with PyO3 and rustworkx (iirc), but I'm surprised if the non-interface versions used elsewhere are an issue.

@jakelishman
Copy link
Member

Also, fwiw, we don't use Borsch serialisation or deserialisation of anything, so I'd be very surprised if we're actually exposed to any related bugs from hashbrown 0.15.0.

@jakelishman
Copy link
Member

You have the PR locked so I can't push changes to your branch, but I tried locally, and we can just update the 0.15.0 dep to 0.15.2. If you reset this branch to the state of main, then run cargo update -p [email protected], it should update only the problematic dependency and leave the fixed one intact.

I can open a separate PR if you prefer.

@chookity-pokk
Copy link
Contributor Author

You have the PR locked so I can't push changes to your branch, but I tried locally, and we can just update the 0.15.0 dep to 0.15.2. If you reset this branch to the state of main, then run cargo update -p [email protected], it should update only the problematic dependency and leave the fixed one intact.

I can open a separate PR if you prefer.

This worked. I think when I tried I must have just done a regular cargo update hashbrown which caused the issues with building the package. Thanks for the help! This should be ready now!

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

No worries, thanks for helping out! Glad to see you guys from q-ctrl here - hopefully there's places we can work together.

@jakelishman jakelishman enabled auto-merge January 8, 2025 16:46
@jakelishman jakelishman added type: qa Issues and PRs that relate to testing and code quality stable backport potential The bug might be minimal and/or import enough to be port to stable Changelog: None Do not include in changelog labels Jan 8, 2025
@jakelishman jakelishman added this to the 1.3.2 milestone Jan 8, 2025
@chookity-pokk
Copy link
Contributor Author

No worries, thanks for helping out! Glad to see you guys from q-ctrl here - hopefully there's places we can work together.

Glad to be here! Hopefully we have plenty of more things to contribute in the future.

@chookity-pokk chookity-pokk changed the title Downgrade indexmap to get rid of hashbrown security vulnerability Upgrade hashbrown to fix security vulnerability Jan 8, 2025
@jakelishman jakelishman added this pull request to the merge queue Jan 8, 2025
Merged via the queue into Qiskit:main with commit 682cf3b Jan 8, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog Community PR PRs from contributors that are not 'members' of the Qiskit repo stable backport potential The bug might be minimal and/or import enough to be port to stable type: qa Issues and PRs that relate to testing and code quality
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants