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

Feature: Add the OpenSSF Scorecard GitHub Action #25054

Closed
pnacht opened this issue Dec 1, 2022 · 8 comments
Closed

Feature: Add the OpenSSF Scorecard GitHub Action #25054

pnacht opened this issue Dec 1, 2022 · 8 comments

Comments

@pnacht
Copy link
Contributor

pnacht commented Dec 1, 2022

Is your feature request related to a problem? Please describe.

Open-source supply-chain attacks are increasing every year. Due to three.js' position as the most popular JS 3D library, it's been included in the Open Source Security Foundation (OpenSSF)'s list of the 100 most important open-source projects.

Describe the solution you'd like

Add the OpenSSF Scorecard GitHub Action, which performs dozens of automated checks to ensure the project's security posture is continuously solid. The Scorecard is a form of project "meta analysis"; it doesn't detect vulnerabilities in your code, but instead makes sure your settings and security features are following the best practices to minimize the risk of vulnerabilities.

For instance, #24332 was based on feedback from the Scorecards system.

(Spoiler alert: three.js has a very solid 7.5/10 score, which puts it at the top 5% of relevant projects)

Would you be interested in a PR to add this Action?

The Action has an associated badge that can be (optionally) added as well.

Additional context

I work for Google (an OpenSSF founding member), working full-time to help open-source maintainers improve their projects' security.

Detail of a Token-Permissions alert, indicating the specific file and remediation steps

@mrdoob
Copy link
Owner

mrdoob commented Dec 2, 2022

Interesting... If you make a PR we'll be able to see the results in the PR itself, right?

@pnacht
Copy link
Contributor Author

pnacht commented Dec 2, 2022

@mrdoob you can see the project's score, as well as the results for individual checks, via the API: https://api.securityscorecards.dev/projects/github.com/mrdoob/three.js

In summary, it has detected that:

  • a handful of PRs were merged without review (I believe that's Editor: Fix usage of USDZExporter. #25055, for example)
  • both 'dev' and 'master' lack branch protection
  • the project lacks a security policy regarding how vulnerabilities can be responsibly disclosed
  • the CI workflow could hash-pin its GitHub Action dependencies (actions/checkout@1254ab3... instead of @v3, for example).
  • the project isn't fuzzed

The Action currently doesn't have official support for running on PRs (that feature is due for release next quarter), it instead runs on every push to master (or dev) and populates the Security Dashboard with alerts such as the one shown above if it detects any missteps.

@mrdoob
Copy link
Owner

mrdoob commented Dec 21, 2022

Not sure about how I feel about imposing these "best practices" into the project.

@pnacht
Copy link
Contributor Author

pnacht commented Dec 21, 2022

Fair enough. Could you let me know what your concerns are? That'd be valuable feedback for the Scorecard team.

@LeviPesin
Copy link
Contributor

It's very hard to imagine what secuity vulnerabilities could be in three.js as library (not threejs.org website). three.js does not deal with any secure information - neither reads, nor produces.
(The only potential vulnerabilities that I can imagine are performance vulnerabilities - like, some possibilities of catastrophic backtracking or an infinite loop somewhere, and fuzzing can help with detecting these)

@pnacht
Copy link
Contributor Author

pnacht commented Dec 21, 2022

Hey @LeviPesin, thank you.

Just to clarify, the issues that Scorecard focuses on aren't about detecting vulnerabilities in the three.js library itself (it isn't a traditional static/dynamic analysis tool), but the protocols that the project has to prevent the codebase from being hijacked or maliciously modified.

As an example, should a maintainer's account be compromised, an attacker could push malicious code directly to the dev branch hidden in a "fix typo" commit (the classic use-case being for crypto-mining). Activating branch protection would mitigate this risk by ensuring everything goes through PRs. PRs can then be configured such that the PR author (even a maintainer) can't merge their own PRs.

These are the sorts of suggestions that Scorecard aims to make.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Dec 21, 2022

Thanks @pnacht, a few thoughts in context here. There's a frustrating history in web tooling of presumed security vulnerabilities being flagged by systems like npm audit, and surfaced directly to end-users in ways that (1) lack any understanding of project's context, and (2) offer no ways to provide that context. npm audit: Broken by Design is a good critique. As a maintainer I waste a lot of time trying to explain false positives1 to users who reasonably expect security audits to come back empty, or 'fixing' issues that have no plausible value to the project.

I understand Scorecard has different goals, and may be doing much better than npm audit in signal-to-noise ratio. Thank you for that, if so! I agree with most recommendations on the Scorecard, with a few exceptions. But to some extent, I feel inundated by noise already and cautious of introducing more as a maintainer. If there's room for incrementally "opting in" to checks slowly, that may be more appealing for us.


1 In contrast, I don't believe I've ever discovered an actual vulnerability through npm audit. 😕

@pnacht
Copy link
Contributor Author

pnacht commented Dec 21, 2022

Hey @donmccurdy, I get you. And yeah, Scorecard tries to keep the best possible signal/noise ratio (though we'll be the first to admit it isn't perfect!).

However, also note that the Scorecard info is already public (see the API link I shared earlier). The Action simply makes that info easier for maintainers to monitor in the Security Dashboard (which users can't see).

That being said, I absolutely understand if you feel there's already too much noise! If so, just close the issue and I'm grateful for all the feedback!

@mrdoob mrdoob closed this as completed Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants