Add the OpenSSF Scorecard GitHub Action

[pnacht]

Description

Hello, I'm working on behalf of Google and the Open Source Security Foundation to help essential open-source projects improve their supply-chain security. Given how crucial PHP is to the modern internet, the OpenSSF has naturally identified it as one of the 100 most critical open source projects.

I saw the oss-fuzz badge on your README. oss-fuzz is just one of many tools Google and the OpenSSF have developed to improve the safety of the open-source community.

Would you consider adopting another OpenSSF tool called Scorecards? Scorecards runs dozens of automated security checks to help maintainers better understand their project's supply-chain security posture. It is most commonly adopted as the Scorecard GitHub Action. It is lightweight and runs on every change to the repository's main branch. The results of its checks are available on the project's security dashboard, and include suggestions on how to solve any issues (see an example below). This Action has been adopted by 1800+ projects already, having some prominent users like Tensorflow, Angular and Flutter.

Would you be interested in a PR that adds this Action? Optionally, it can also publish your results to the OpenSSF REST API, which allows a badge with the project's score to be added to its README.

In case of doubts or concerns you can check out the Scorecards FAQ. Anyway, feel free to reach me out.

[damianwadley]

Most of PHP's internal discussions happen on the "internals" mailing list. Is that something you'd be able to join temporarily? https://www.php.net/mailing-lists.php#internals

If the PR to add support wouldn't be too much of an investment of your time then I think that would be a good idea, as it would be useful as a visual reference in helping us understand more about the requirements of using Scorecards - and, of course, save us the effort of doing it ourselves later.

[iluuu1994]

@pnacht Thank you for reaching out! I took a quick look at https://api.securityscorecards.dev/projects/github.com/php/php-src. A couple of things:

5 out of last 30 changesets reviewed before merge -- score normalized to 1

How is this determined? We often rebase before merging (possibly with small adjustments) as fast-forward merges are preferred. I would've expected this number to be higher.

Signed-Releases / Packaging

We don't release via GitHub. I'm not sure if the -1 score is included.

HEAD is vulnerable to OSV-2021-1199, OSV-2022-235, OSV-2022-573

At least OSV-2022-573 seems to have been addressed.

I'm not sure if this provides a big benefit for us. The one variable that changes over time are code reviews but we don't have guidelines enforcing them. We also have some long-term contributors (most notably Dmitry) who have pretty much exclusive knowledge on certain areas of the codebase (JIT) where enforcing code reviews probably doesn't make much sense (edit: or to rephrase, would probably take a considerable amount of time from contributors, which we currently can't really afford). We're already trying to use common sense and not wasting time for trivial things by pushing directly and creating PRs for everything else.

Let's see how others feel :slightly_smiling_face:

[pnacht]

As suggested by @damianwadley, I've submitted a PR which includes the Action. I'll happily join the mailing list to continue this conversation! (I'll send an initial mail shortly making sure to summarize the conversation here so far).

@iluuu1994:

5 out of last 30 changesets reviewed before merge -- score normalized to 1

How is this determined? We often rebase before merging (possibly with small adjustments) as fast-forward merges are preferred. I would've expected this number to be higher.

[...]

I'm not sure if this provides a big benefit for us. The one variable that changes over time are code reviews but we don't have guidelines enforcing them. We also have some long-term contributors (most notably Dmitry) who have pretty much exclusive knowledge on certain areas of the codebase (JIT) where enforcing code reviews probably doesn't make much sense (edit: or to rephrase, would probably take a considerable amount of time from contributors, which we currently can't really afford). We're already trying to use common sense and not wasting time for trivial things by pushing directly and creating PRs for everything else.

Indeed, the Code-Review only considers changes which are submitted via PR and which are subsequently merged after some form of code review. This may be a formal process where someone else must approve the changes; it may also be an implicit code review when whoever merges the PR is different from its author (under the assumption whoever did the merge checked the PR before merging).

I've seen the PHP git workflow allows for unilateral commits by core maintainers, which as you point out is entirely understandable.

However, Scorecards does indicate through the Code-Review check you pointed out that there's a security trade-off here (the Branch-Protection check is also relevant here). For example, were Dmitry's account to be compromised, someone could push a malicious commit to the repository with little oversight.

That's all Scorecards is reporting: that the current process exposes the project to a certain level of risk, when other protocols could be adopted that reduce that risk. But you are absolutely correct that these protocols have costs of their own (notably on other maintainer's time and possibly the project's overall speed of deployment). The decision of the best balance between agility and security is naturally up to you all who've invested so much time on this project and who are aware of its specific circumstances.

Signed-Releases / Packaging

We don't release via GitHub. I'm not sure if the -1 score is included.

The -1 is not included. That score is simply ignored when the final result is tabulated.

[ricardoboss]

However, Scorecards does indicate through the Code-Review check you pointed out that there's a security trade-off here (the Branch-Protection check is also relevant here). For example, were Dmitry's account to be compromised, someone could push a malicious commit to the repository with little oversight.

...as has almost been the case with the incident in March last year: https://github.com/php/php-src/commit/c730aa26bd52829a49f2ad284b181b7e82a68d7d and https://github.com/php/php-src/commit/2b0f239b211c7544ebc7a4cd2c977a5b7a11ed8a

(Disclaimer: I am not a maintainer of PHP but am grateful someone out there looks out for these kinds of attacks!)

[smalyshev]

As far as I can see, that project considers any problem found by the fuzzer a "security vulnerability", which is not the case in the case of PHP. Due to this, I do not think this is a good match for PHP, and would not recommend enabling this functionality.

[pnacht]

@smalyshev Just to clarify, the Scorecard actually just checks whether projects have fuzzing enabled (indeed, PHP has a 10/10 in the Fuzzing check). As for the Vulnerabilities check, it simply checks to see if the project has any open OSV's, regardless of origin. As I understand it, it just so happens the latest OSV's were identified via fuzzing.

And just for my own education, why don't you consider problems identified by fuzzing to be security vulnerabilities in the case of PHP? Or do you mean the vulnerabilities it's found so far?

[iluuu1994]

And just for my own education, why don't you consider problems identified by fuzzing to be security vulnerabilities in the case of PHP? Or do you mean the vulnerabilities it's found so far?

They certainly can be but not always, or maybe not even usually. They often depend on very specific PHP code to trigger some kind of memory corruption that's unrealistic to occur in normal hand-written PHP code. But if the attacker can run arbitrary PHP code on your sever there's likely no need for that, and you can get to any information you want.

There are certainly problems identified by fuzzing that can occur in real world code, but maybe they should be classified as security vulnerabilities manually.

[smalyshev]

@pnacht the policy about security issues is https://wiki.php.net/security.

The short version of it is that nearly 100% of what fuzzing finds now is engine corner cases which require specially contrived code. And if you can feed arbitrary code to PHP, there's no security barrier left for you to cross - you can do basically anything.

It does not mean fuzzing didn't in the past - and may not in the future - find security issues too. That happened in the past, and definitely can happen in the future, and in that case such an issue will be handled as a security issue. But that's not all the issues fuzzer finds - and not even the majority of it.

To be clear, this is not to deny usefulness of the fuzzing and the reality of the bugs it finds. Fuzzing is important (that's why I myself contributed to making it work for PHP) and finds real bugs, and those need to be fixed. However, as it is now, most of them aren't and not likely to be in the future security bugs. That's why automatically publishing open fuzzing issues as "security vulnerabilities" in PHP would not be helpful.

[TysonAndre]

I'm not sure if this provides a big benefit for us. The one variable that changes over time are code reviews but we don't have guidelines enforcing them. We also have some long-term contributors (most notably Dmitry) who have pretty much exclusive knowledge on certain areas of the codebase (JIT) where enforcing code reviews probably doesn't make much sense (edit: or to rephrase, would probably take a considerable amount of time from contributors, which we currently can't really afford). We're already trying to use common sense and not wasting time for trivial things by pushing directly and creating PRs for everything else.

As someone who is currently not paid to work on php-src or PECLs during work hours, and has reviewed those changes before and contributed minor improvements to opcache, I agree that it would take a considerable amount of time from contributors for changes to the JIT, and that the project currently can't afford this - with months or years of background experience,

• I have looked over some of the changes made to the JIT years ago, unpaid, in my free time, because it was a feature interesting to me (I forget if it was JIT support for php 8.1 enums, or the PHP 7.4 typed properties, or both), though it took several days in my free time to get a basic understanding of what just the jit code affected by the pr was doing, and to reason about whether that the changes that were made were correct for edge cases.

  And that was after many months of being paid for a job that had a primary focus on migrating PECL extensions from PHP 5.6 to PHP 7.0 that gave me the background knowledge I'd need to review it (see https://pecl.php.net/user/tandre , e.g. igbinary, bug fixes for APCu's integration with serializers, contributions to several other widely extensions, and less-used extensions that require strong familiarity with engine internals such as runkit7), and months of paid and subsequently years of unpaid volunteer time to migrate those extensions to support subsequent minor version of PHP 7.x/8.x

• I would be ecstatic (as a volunteer contributor to php-src and its ecosystem, and an end user of the programming language of php) if there were more than 1 or a few people able to have a full time job or familiar with engine internals to ensure code reviews could be mandated for core contributors without introducing delays in bug fixes or slowing the development process on engine enhancements such as performance improvements. I expect that may require months or years of onboarding, though, and any employees paid to work on php-src would have to balance other priorities vs working on the JIT specifically.
  • Being distributed across time zones (and hours of the day when volunteers or employees can work on a project) is also a practical concern for delays in development of bug fixes and language/engine improvements, e.g. promoting bug fixes or security fixes in php 8.0 to 8.1, then again in 8.2

• Separately, even prior to this, I do think it would make some sense to review permissions and restrict the number of team members who can push directly somewhat if we haven't, e.g. to require code reviews from maintainers pushing tags or branches directly (e.g. accidentally pushing to php/php-src for the release branch instead of a forked branch). I don't know what other maintainers thoughts on this are, or how we'd get a consensus.

  • (e.g. it could be removed for members that were inactive for years, those who opt out manually to avoid accidentally pushing, have concerns about being the target of supply-chain attacks such as phishing or account takeover, or because they don't expect to need it more than few times a year, etc)

---

Separately, I'm still writing up and editing my questions about this initiative for the mailing list.

[pnacht]

After a thorough discussion in the mailing list (see here and use "[next in thread]" to read the rest of the conversation), the consensus was to not implement the Action.