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

Guide Remediation Vuln Filtering not working (--vulns=OSV-ID) #913

Open
khai-tran opened this issue Apr 11, 2024 · 7 comments
Open

Guide Remediation Vuln Filtering not working (--vulns=OSV-ID) #913

khai-tran opened this issue Apr 11, 2024 · 7 comments
Assignees
Labels
bug Something isn't working guided remediation Related to guided remediation / osv-scanner fix

Comments

@khai-tran
Copy link

Hi folks, I ran into an issue when trying to patch a specific vulnerability via vulns filter yielded nothing. Reference: https://google.github.io/osv-scanner/experimental/guided-remediation/#vulnerability-selection

Steps to reproduce

  • Scan npm dependencies with osv-scanner -r ., verify that vulnerability exists (e.g https://osv.dev/vulnerability/GHSA-xf7w-r453-m56c) in console output.
  • Patch with osv-scanner fix --non-interactive --strategy=relock -M package.json -L package-lock.json --data-source native --vulns=GHSA-xf7w-r453-m56c (trying with alias such as CVE-2019-13173 also failed).
  • Console output:
Found 0 vulnerabilities matching the filter
No dependency patches are possible
REMAINING-VULNS: 0
UNFIXABLE-VULNS: 0

Is OSV Id different then one of the value above? Unrelated to the issue, but it would be nice to be able to filter on the alias (CVE Id) as well, as they're more popular.

@michaelkedar
Copy link
Member

michaelkedar commented Apr 12, 2024

I think this might be caused by an oversight on my end.
I'm guessing completely relocking the project resolves the vulnerability without the need to change your direct requirements, and it doesn't actually check for the vulnerabilities in the lockfile before so it thinks there are no vulnerabilities.

Could you please verify by manually relocking the project yourself, by either:

  • deleting package-lock.json and node_modules/ then running npm install
  • or launching guided remediation in interactive mode and choosing Re-lock project -> Write changes to manifest.

Then could you run the osv-scanner scan to check if the vulnerability is removed.

Thanks for trying guided remediation and uncovering these issues, it's really appreciated!

Unrelated to the issue, but it would be nice to be able to filter on the alias (CVE Id) as well, as they're more popular.

Yes, that's a good idea. I do want to eventually support aliases better.

@michaelkedar michaelkedar added the bug Something isn't working label Apr 12, 2024
@michaelkedar michaelkedar reopened this Apr 12, 2024
@khai-tran
Copy link
Author

Thanks @michaelkedar for taking a look! To add more context, the project has package.json and package-lock.json. Running fix command did not modify package.json, so re-locking with npm install yielded no changes. I would expect the outcome would be modifying both package files accordingly, otherwise it could confuse devs if those files are not in sync.

FWIW, I had better luck with a direct dependency of the project (lodash) when running in interactive mode. For non-interactive, it was able to find the issue, but could not fix it:

osv-scanner fix --non-interactive --strategy=relock -M package.json -L package-lock.json --data-source native --vulns=GHSA-35jh-r3h4-6jhm

Found 1 vulnerabilities matching the filter
No dependency patches are possible
REMAINING-VULNS: 1
UNFIXABLE-VULNS: 1

@oliverchang
Copy link
Collaborator

Just to clarify: were you able to fix the same issue interactively?

"UNFIXABLE-VULNS: 1" in the CLI mode implies that it's a vulnerability which can't be fixed, because it's in a transitive dependency that can't be bumped through either relocking or bumping of direct dependencies.

@oliverchang
Copy link
Collaborator

Also regarding re-locking, did you delete package-lock.json and node_modules/ first? I believe npm doesn't actually change anything if those exist.

Re-locking typically does change your package-lock.json, even if package.json isn't changed. This is because npm's dependency resolution typically takes the latest versions available for all dependencies, leading to frequent changes in the resolved dependency graph.

If you're able to share your package.json / package-lock.json, we'd be very happy to take a look! If you would prefer to send it in a private email, please send it to ochang@google.com :)

@sc-ktr4n
Copy link

Hello, I'm attached the package lockfiles below:
package.json
package-lock.json

OSV output

| https://osv.dev/GHSA-jf85-cpcp-j695 | 9.1  | npm       | lodash                       | 4.17.4  | package-lock.json |
| https://osv.dev/GHSA-jf85-cpcp-j695 | 9.1  | npm       | lodash-es                    | 4.17.4  | package-lock.json |

The vulnerability was GHSA-jf85-cpcp-j695, which affects both lodash and lodash-es version 4.17.4:

  • Patching using interactive mode fixed lodash-es in package-lock.json file, while non-interactive mode failed.
  • Patching in non-interactive mode with strategy in-place also did upgrade it: UPGRADED-PACKAGE: lodash-es,4.17.4,4.17.21
  • Patching in non-interactive mode with strategy relock did not update anything
    I would expect lodash should be updated as well, or at least the output should tell why it can't be updated because of some reason.

IMO only updating lock file resulted in a mismatch between package files, and if the developer install or update new dependency, it could wipe out the patched in lock file (if they decide to remove lock file before relocking). An update to a dependency is typically is made to both files for that reason.

@michaelkedar
Copy link
Member

Thanks for providing your manifest & lockfile, it's very helpful!

First, I will note that GHSA-jf85-cpcp-j695 appears 3 times in the scan output:

| https://osv.dev/GHSA-jf85-cpcp-j695 | 9.1  | npm       | lodash                       | 3.10.1  | package-lock.json |
| https://osv.dev/GHSA-jf85-cpcp-j695 | 9.1  | npm       | lodash                       | 4.17.4  | package-lock.json |
| https://osv.dev/GHSA-jf85-cpcp-j695 | 9.1  | npm       | lodash-es                    | 4.17.4  | package-lock.json |

(lodash@3.10.1 may show up as a dev dependency, but it does also come in through one of your production dependencies - this is just a bug #924)

  • When I run in-place patches (both interactively and non-interactively), it upgrades lodash-es only, which fixes one instance of the vulnerability, but lodash@3.10.1 and lodash@4.17.4 both are still present and vulnerable.
  • Manually attempting to re-lock the project with npm gives an error due to mismatched peer dependencies (this is an issue I am aware of)
  • Using npm install --legacy-peer-deps to sidestep this issue (which is how guided remediation currently behaves), I see GHSA-jf85-cpcp-j695 now only affects lodash@3.10.1.
    • lodash@4.17.21 and lodash-es@4.17.21 are also installed, but neither are affected by the vulnerability.
  • Currently, the relock strategy only considers a vulnerability fixable if all instances of the vulnerability can be removed from the dependency graph.
  • From some manual inspection, every version of the font-loader package (which you directly depend on) brings in the vulnerable version of lodash, which is (one of the reasons) why it cannot be resolved.

IMO only updating lock file resulted in a mismatch between package files, and if the developer install or update new dependency, it could wipe out the patched in lock file (if they decide to remove lock file before relocking). An update to a dependency is typically is made to both files for that reason.

lodash-es is not a direct dependency of your project - it does not appear in your package.json so there is nothing in that file to update. The in-place strategy will not change the package-lock.json in a way that is incompatible with the package.json. Relocking can significantly change the computed dependency graph so the previous in-place patches cannot be applicable anymore.

at least the output should tell why it can't be updated because of some reason.

Yes, I'll create a new issue to surface reasons for failure better.

@sc-ktr4n
Copy link

Thanks for filing issue #925 and the explanation.
Out of curiosity, I tried a minimal test case below for lodash:

{
  "name": "foo",
  "version": "0.1.0",
  "private": true,
  "dependencies": {
    "lodash": "^3.10.1"
  }
}

osv-scanner was able to find the vulnerabilities, and fixed it in both package and package lock file, which is great!
Thanks again for taking a look!

@michaelkedar michaelkedar added the guided remediation Related to guided remediation / osv-scanner fix label May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working guided remediation Related to guided remediation / osv-scanner fix
Projects
None yet
Development

No branches or pull requests

4 participants