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

Exclude packages from security review #27

Closed
danepowell opened this issue Apr 14, 2016 · 12 comments
Closed

Exclude packages from security review #27

danepowell opened this issue Apr 14, 2016 · 12 comments
Assignees

Comments

@danepowell
Copy link

There are several instances where you might legitimately want to include a package with a security advisory in your project. Quite often, upgrading to a newer secure version of a package may be difficult or impossible, and the security vulnerability may be low criticality or not affect your project at all. You might also want to patch the insecure version of the module (using something like https://github.com/cweagans/composer-patches) rather than upgrading to a new version.

Unfortunately there's no way to allow these kinds of exceptions with SecurityAdvisories, and no workaround that I know of.

@Ocramius
Copy link
Member

There are several instances where you might legitimately want to include a package with a security advisory in your project.

I can't really think of any. Self-patching is a solution that requires disabling tools that detect issues anyway. If you build workarounds, then the tooling won't be able to know anyway.

Quite often, upgrading to a newer secure version of a package may be difficult or impossible, and the security vulnerability may be low criticality or not affect your project at all.

That is true indeed, but that's also why this package acts at composer update time, and not at composer install time. Any composer update is a potentially risky operation if proper e2e testing isn't in place.

You might also want to patch the insecure version of the module (using something like https://github.com/cweagans/composer-patches) rather than upgrading to a new version.

This can be done in a way that is compatible with this module by using "replace": "package/name" or "provides": {"package/name": "1.0.99999"}. You are responsible for making sure your replacement is compatible though.

Unfortunately there's no way to allow these kinds of exceptions with SecurityAdvisories, and no workaround that I know of.

Adding exceptions is not something you'd do in security-sensitive contexts. If your package provides a patch before the actual vendor, then do use the "replace" or "provides" tricks described above.

@josefsabl
Copy link

Well, there's this: https://security.snyk.io/vuln/SNYK-PHP-FIREBASEPHPJWT-2434829
And this: tuupola/slim-jwt-auth#217

I was looking for a way to "whitelist" a vulnerable package to find this thread. Now I need to remove security advisories altogether 😥.

@Ocramius
Copy link
Member

Ocramius commented Mar 1, 2023

TBH, sounds like you have a security issue, and you need to help with the affected library upgrade, or maintain your own fork of tuupola/slim-jwt-auth until upstream gets a stable release.

You are just shooting at the messenger here.

@josefsabl
Copy link

I didn't want to shoot or anything. I just wanted to showcase that this might be useful. Aforementioned vulnerability can be overcome with configuration which we checked so the vulnerability in fact is not there.

@Ocramius
Copy link
Member

Ocramius commented Mar 9, 2023

This library limits itself to declaring which ranges are not considered safe for installation: whether a CVE depends on misconfiguration or specific setup is not something that can be documented here.

If a library has a "security issue around de-serialization", yet you never deserialize stuff in your project, where you have the library installed, this project cannot say.

@rliebi
Copy link

rliebi commented Mar 16, 2023

We have a similar issue with our project where a library is having a security vulnerability which only can be exploited by logged in admin users. The fix is in the next Major version which is in ALPHA state, which is also suggested by the CVE. (the major version 11 which is not released yet)

So maybe there could be a check if when the fix version is not released yet, we could accept the package anyways? because what alternative do we have? a whitelist for a specific version would be fantastic.

(besides not requiring this security bundle)

https://huntr.dev/bounties/04447124-c7d4-477f-8364-91fe5b59cda0/

@Ocramius
Copy link
Member

I don't think that's reasonably feasible: we only take authoritative sources here, and as you can see, maintainers already do enough mistakes.

Packages are sometimes not even released via tagging.

@LosD
Copy link

LosD commented Jul 5, 2023

Damn. Was good while it lasted.This package is impossible to use in a sane way without whitelists. Removing.

For anyone that needs checks to continue even though a specific package cannot be updated right now: Use e.g. https://github.com/fabpot/local-php-security-checker instead. Not as convenient, but you can temporarily ignore packages (or filter if using in an automated script)

@Ocramius
Copy link
Member

Ocramius commented Jul 5, 2023

@LosD AFAIK, there is one way to allow-list an entry:

{
  "require": {
    "some/unsafe-package": "1.2.3 AS 99.99.99"
  }
}

I'm not 100% sure this will work, but it should trick SAT into thinking that the constraint is completely safe.

That said, security takes priority over:

  • product
  • bugs
  • backwards compatibility

A broken or non-functioning tool is better than a compromised one.

@LosD
Copy link

LosD commented Jul 5, 2023

@Ocramius The issue with that stance is that a lot of CVRs are issued when it is easy to misconfigure a package (see https://nvd.nist.gov/vuln/detail/CVE-2021-46743 which was the example for one of the requests from @josefsabl above), and others only covers certain uses. A lot of package maintainers will (understandably) not upgrade to sacrifice backwards compatibility for something that for their use case is a non-issue with a CVR number.

This all or nothing approach means worse security, not better.

@Ocramius
Copy link
Member

Ocramius commented Jul 5, 2023

Hard disagree on that, but then I'll gladly accept patches to generate manifests for each CVSS level, if you are willing to take that kind of risk upon you :P

This package is here as a clear exclusion list: that is documented in the first lines @ https://github.com/Roave/SecurityAdvisories/tree/885786b30f50e83a88941df2062d68de623599fd#usage

The fact that companies won't upgrade, then suffer the woes of that, is just the general misunderstanding that OSS is free as in "your problem now" :P

@Ocramius
Copy link
Member

Ocramius commented Jul 5, 2023

Meanwhile, locking here: I've made it clear that this package is just here to be used as an exclusion list that effectively shadows installable package versions.

Further feedback on this approach would need to come in form of PRs to https://github.com/Roave/SecurityAdvisoriesBuilder

@Roave Roave locked as resolved and limited conversation to collaborators Jul 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants