RFC: Include roave/security-advisories in composer.json

[ScopeyNZ]

Affected Version

4.x

Description

@Firesphere raised a good idea in a satellite module about including roave/security-advisories in composer.json to help assert people do not install known security vulnerabilities.

This module works by declaring composer conflicts with packages that are known to have security vulnerabilities - I believe this is auto-generated from the vulnerability database that SensioLabs maintains: https://security.sensiolabs.org/ (or apparently https://security.symfony.com/ now)

Questions:

• Does this have value for us?
• Should we include this module or just suggest it?
• Should we include it in framework or a module like silverstripe/installer (or all modules)

Sub Tasks

• Add security-advisories to upgrader
• Add security-advisories to installer
• Add security-advisories to recipe-core/recipe-cms
• Add security-advisories to vendor-plugin-helper
• Add security-advisories to sspak
• Add security-advisories to cow
• Add security-advisories to api.silverstripe.org

Related PRs

• Add security advisory to dev requirements. silverstripe-upgrader#140
• Add Roave Security advisories to composer silverstripe-installer#246

[Firesphere]

Questions:

• Does this have value for us?
• Should we include this module or just suggest it?
• Should we include it in framework or a module like silverstripe/installer (or all modules)

My answers, in order:
Yes, Include, All modules.

The security-advisories is not just generated from sensiolabs' list, but curated and maintained by a group of active developers (the Roave group), that also monitors security vulnerabilities outside of the SensioLab's list. I don't have an example right now, but I know a few things on the list are not in SensioLabs list.

*edit

Come to think of it, SensioLabs may use a once-every-while updated list of the security advisories, instead of vice versa (as @ScopeyNZ described it)

[maxime-rainville]

It's a good idea, however I think people should have the option to opt out if they don't want this feature. I would make it part of the installer, so it gets added to their composer file for new projects.

If they don't want it, they can remove it from their dependency.

[Firesphere]

I disagree with you @maxime-rainville , As the developers of SilverStripe, there is a certain responsibility we have in making sure the users of SilverStripe are not exposed to security related issues that could easily be avoided by adding this composer dependency.

My reasoning for adding it to all modules, is that if somebody happens to use an affected vendor module, and updates a different module, it would start conflicting, making it immediately visible that there is a problem with the vendor module version installed.

[maxime-rainville]

From their read me, it looks like they don't think it should be installed in libraries https://github.com/Roave/SecurityAdvisories#stability

This package is therefore only suited for installation in the root of your deployable project.

They recommend installing it as a dev dependency. That would make adding it to our modules a pointless exercise, because composer doesn't look at dev dependencies of packages when resolving dependencies for your project.

[kinglozzer]

I would make it part of the installer, so it gets added to their composer file for new projects.

+1 for this

[robbieaverill]

It's worth noting that we have a also commercially supported SilverStripe module which provide similar information for SilverStripe modules (not all PHP libraries) - https://github.com/bringyourownideas/silverstripe-maintenance

[maxime-rainville]

We definitively should add it to projects like:

• upgrader
• vendor-plugin-helper
• sspak
• api.silverstripe.org

[ScopeyNZ]

Yep I'm happy with that. I think we shouldn't add it to "component" modules. Although it is good practice I don't think we should enforce this sort of thing on those who use our modules. This falls in line with the usage recommended by the library itself that @maxime-rainville mentioned above.

[maxime-rainville]

Shall we call this one accepted?

Basically this will involved adding a dev requirement for "roave/security-advisories": "dev-master" to the following projects:

• silverstripe/installer ... so new SilverStripe project get it by default,
• upgrader, vendor-plugin-helper, sspak and any other project that distributes an executable,
• api.silverstripe.org and every other stand alone project we have.

Side note

I was looking at our own security advisories on the FriendsOfPHP repo. Looks like they are missing quite a few. We probably should add it to our process to proactively create push request to their repo when ever we disclose a new vulnerability.

[robbieaverill]

Do we want to add it to our recipes as well as installer, incase people create projects with them instead of installer?

I was looking at our own security advisories on the FriendsOfPHP repo. Looks like they are missing quite a few. We probably should add it to our process to proactively create push request to their repo when ever we disclose a new vulnerability.

Yep, we have this logged with cow: silverstripe/cow#85. @ScopeyNZ spent some time working on it for a hackday I think.

[robbieaverill]

It sounds like @maxime-rainville @kinglozzer and @ScopeyNZ are in favour. I'm ambivalent, but there's still a bunch of the @silverstripe/core-team that haven't had a say yet.

[ScopeyNZ]

Yep, we have this logged with cow: silverstripe/cow#85. @ScopeyNZ spent some time working on it for a hackday I think.

Yeah I got it working pretty well except for the fact the data doesn't have any good structure from silverstripe.org. There's a team doing some work on that site though so I've asked if they could let me know when they get to the advisory part so I can ask for some slight improvements 🙂 . Once that's done I think I'll be able to finish that off - hopefully not too long now.

[maxime-rainville]

Adding "roave/security-advisories" to the recipes would only work if people start their project like this composer create-project silverstripe/recipe-core myproject. Is that a common use case?

[robbieaverill]

It certainly is something we promote as an option, yeah. After all, installer is just a recipe with a theme in it too

[chillu]

Written up an issue to get these checks into our module CI - which seems separate? silverstripe/silverstripe-admin#764

[flamerohr]

I'm 👍 for it being a dev-requirement, it's a good idea to bring this in to provide developers a bit of due diligence

[robbieaverill]

Cool, sounds like it's rfc/accepted then

[phptek]

As @maxime-rainville spotted, the FriendsOfPHP package is heinously out of date and takes an odd approach to data that should be bang up to date - with the inclusion of YML files (instead of calling a web-service where one is available (https://www.silverstripe.org/download/security-releases/rss - in SilverStripe's case).

I wrote this shell script yesterday, quite independently of this thread which would be modified with whatever the SilverStripe specific approach becomes: https://gist.github.com/phptek/ee87577413ead93a8582397c45b3939a (It uses the security-checker security:check composer.lock approach - which is of course refs out-of-date SS advisories)

[ScopeyNZ]

I see that recipe-core and recipe-cms are in the OP now but I don't think they should be?

[robbieaverill]

I think they can be, you can create projects from them which is probably why they were added to the list

[ScopeyNZ]

I suppose it's benign anyway because only the dev dependencies in the root directory are installed.

[chillu]

Let's establish some goals (from my perspective):

• Visibility on upstream security issues for SilverStripe Supported Modules - see Scan for vulnerable NodeJS and Composer packages silverstripe-admin#764
• Encourage fast upgrades for security releases through good defaults for new SilverStripe projects (created through installer or composer create-project)
• Allow override of defaults
• Retain behaviour for existing SilverStripe projects (opt-in, don't start failing builds in a minor recipe/module upgrade)

I can see a few good reasons to NOT use this in your own projects:

• Teams choosing to use their own security checkers, e.g. part of a wider enterprise compliance effort
• Avoid tools which call out to the internet as part of their build process Edit: Misunderstood the tool, it doesn't do that
• Avoid untrusted dependencies, particularly since this package requires you to use dev-master. If github.com/roave ever gets compromised, an attacker can just chuck some PHP into that project and has a remote code execution. The fact that it's a require-dev only makes this marginally better, since non-prod environments often have sensitive data as well. All composer deps have this issue, and it should be solved by release signature checks, private composer servers - but at least for deps with @stable you can assume those go through a release process and allowing you to pin conservatively. You can't even pin roave to a SHA, because of the way it's implemented - you're required to pull in the latest version.
• The roave approach doesn't distinguish between severity on vulnerabilities - any version containing vulns will fail a composer update and composer require. As much as you want to believe that devs are going to update insecure deps when prompted, the reality is that it's often not feasible. For example, we know of large projects still running 3.1. Adding roave would prevent them from changing anything in their deps through composer update until they've upgraded to 3.6 - that's unfortunately not realistic.
• Looking at what the tool does, it's not clear at all that any failures caused by it are actually due to security issues in those dependencies. You'll just get the standard composer garbage around "dependency conflicts", which is unreadable at the best of times.

Because of the above, I don't think we should add this to the SilverStripe installer. It'll lead to a worse developer experience without clearly communicating the value it's trying to provide. Sorry for the late response on this, didn't really understand what the tool did when commenting earlier.

My recommendation would be that we add this to the "secure coding" advice on doc.silverstripe.org, and let each team evaluate those tradeoffs themselves.

[chillu]

Also, by adding this tool to the installer, we've made an implicit commitment to update the constraints set in https://github.com/Roave/SecurityAdvisories/blob/master/composer.json every time we do a security release. I don't see any change request to our security release process which documents that additional work, or attempts to automate that. Adding a security tool to our default download means we're in charge of ensuring it contains accurate information, delivered in a timely manner.

[kinglozzer]

FYI we’ve already merged it silverstripe/silverstripe-installer#246

[robbieaverill]

We can easily revert if needed, it hasn't been released yet

[chillu]

Argh, this is in a stable release now (4.3.1), right? @robbieaverill or @maxime-rainville Can you please revert this before 4.3.2?

[robbieaverill]

@chillu sure - are we reverting everything related to this or just installer?

[chillu]

My main concern is around reverting installer. I'm less concerned about upgrader, although I'd question the value it provides there, particularly since we're now introducing auto-updating PHAR into the upgrader as a default mechanism

[robbieaverill]

Reverted in silverstripe/silverstripe-installer@810f719

[maxime-rainville]

Having it in the upgrader will fail the build if we have an vulnerable dependency in our composer.lock file and prevent the phar release. I think it provides value.

[Firesphere]

Maybe update the tests to include it? So after doing the installation, attempt to include it in Travis, if that fails, we know there's a vulnerable dependency, but it's not immediately enforced upon the end users?

[Firesphere]

Argh, this is in a stable release now (4.3.1), right? @robbieaverill or @maxime-rainville Can you please revert this before 4.3.2?

No, it's not, from the looks of it, I can't find it in a fresh install except my own composer files @chillu

[chillu]

I'm not fussed about the upgrader, closing. Thanks for reverting, Robbie.