-
Notifications
You must be signed in to change notification settings - Fork 3k
otp scan PRs for vulnerabilities #9790
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
otp scan PRs for vulnerabilities #9790
Conversation
CT Test Results 38 files 1 006 suites 7h 25m 15s ⏱️ For more details on these failures, see this check. Results for commit d2b09ef. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts// Erlang/OTP Github Action Bot |
|
From what I can tell only our github actions dependencies are scanned by this right now, is that correct? Will it in the future be able to use the information in the sbom created in the job you are adding this step to? or is this check only for github actions? |
|
I think you are right @garazdawi Summary
I have tested that it works by manually hand-picking previous commit that fixed a known vulnerability reported to on github repos. Information sent Result |
86a4478 to
c10c255
Compare
|
The vendor vulnerability scanning now fails because I changed the vendor.json |
4f13738 to
f74a0f5
Compare
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
007ae7f to
982b107
Compare
|
I have updated the scripts and this is the end result, using OSV for vulnerability scanning.
|
b359ae7 to
0389fe1
Compare
|
This is the first time I see the OTP purl Should we consider defining a better way to express "releases" in OTP purls, in a way that doesn't clash with application namespace? For example, NPM packages often use an "@" prefix (I don't know what it means, I don't do JS), which in a purl is encoded as "%40". So the Erlang/OTP release 28 could be specified as |
|
thanks for the feedback.
|
In an Elixir release there is an elixir application, and the version numbers are the same. So that example refers to the elixir application, not the release.
Yes, that is probably the best way forward. Whether or not OTP purl needs a way to express releases at all then becomes a separate discussion |
Ok, I did not know
Thanks! |
|
Summing up my thoughts I shared on Slack: 1. Scope of
2. No special cases
3. Using the
4. Representing releases
This keeps the semantics clear: |
|
I also did a tiny change to the flowchart in package-url/purl-spec#472 to better reflect the things stated above. |
|
Thanks for your input as well @maennchen . |
224776a to
08f01c0
Compare
08f01c0 to
8229932
Compare
|
|
||
| ## Introduction | ||
|
|
||
| This section describes how Erlang/OTP reports vulnerabilities for Erlang/OTP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think github advisories should be considered and explained here.
For Erlang/OTP vulnerabilities, this is how CVE are reported today and it will remain I guess.
As we discussed the 2 channels will be redundant. It would be good if we protect in some way against providing inconsistent information.
How Erlang/OTP CVEs are supposed to be fetched? From what source?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two channels are not redundant, but contain shared information :)
- Advisories only list vulnerabilities towards Erlang/OTP (including possible vendor, but not common case)
- VEX statements list:
- under investigation, probably not used,
- affected, same as Advisories, and
- not affected, useful to signal that upstream vulnerabilities do not affect us, silencing false positives, e.g., towards openssl/zlib/asmjit/ryu/pcre2/zstd/etc.
At the moment, there could be inconsistent information. This PR is for testing purposes of OpenVEX.
More details about all the good points that you raised here: #9790 (comment)
|
|
||
| Erlang/OTP will maintain VEX files for the latests three releases. | ||
| Because of this, Erlang/OTP will always contain the latest information in the `master` branch. | ||
| Any OpenVEX file in other branches is considered outdated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't they be deleted? or maybe somehow marked so when processed will generated an error indicating the outdatedness fact?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The script only considers useful the changes in master. If we start considering useful possible things in each of the 3 branches/releases that we maintain, it is a nightmare to sync between branches.
If we like OpenVEX, we can push automatic updates towards a new section in erlang.org that contains OpenVEX statements and users are supposed to fetch those.
For now, this PR is a way to test if OpenVEX is good for our use case. All the other alternatives are more complex, so we start with the simplest one :)
| the [Erlang/OTP Github repository](https://github.com/erlang/otp) in the `master` branch, where | ||
| `<version>` corresponds to the number of the Erlang/OTP release. | ||
|
|
||
| ## Erlang/OTP VEX Statements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can't manually maintain same information in 2 places, I think :-/
should advisory be source of information for Erlang/OTP CVEs? @rickard-green please also comment.
should detection of inconsistent data (advisories vs openvex.table) be something to add to our advisories verification script?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great point, but outside the scope of the PR.
Once this PR is merged, we can start a testing phase of OpenVEX.
- Do we find it useful?
- Easy with the tooling we have?
- Does it silence enough false positives?
After the PR is merged (I started working on the below changes yesterday), I would like to:
- release an automation script that download published advisories in Erlang/OTP Github Advisories (checks our advisories)
- takes our published OpenVEX files and performs set substraction operations,
OpenVEX -- Published Advisories = {}(ignoring non-OTP non-vulnerable statements), and
Published Advisories -- OpenVEX = {}.
This should guarantee consistency for published CVEs - If there are published advisories not present in OpenVEX files, create an automatic PR towards Erlang/OTP with updated
openvex.tableandotp-XX.openvex.jsonfiles in it - If all of this works, we "only" need to release manual OpenVEX statements (not automatically created) for the vendor dependencies that have new CVEs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am all in favour of making sure that we automate as much as possible.
If I were to add this part here as well, the PR may end up with more than 10,000 lines, and it is already tricky to test the Github Actions, so I would prefer that we go with a testing phase and continue adding improvements until we decide to fully adopt OpenVEX.
There will be new vulnerabilities and not automated things in the process, but creating a single PR that solves all of it is a huge job, has many moving parts:
- OSV API integration,
- GH Action integrations scheduled and per PR with reusable workflows that work across three releases,
- OpenVEX generation of OTP release and application version inference,
- testing and verification of OpenVEX results,
- probably more stuff that I forgot
If we add automatic generation of PRs and sync with GH Advisories for verification purposes, it is a humongous task for a testing phase of OpenVEX. The end result should contain as much automation as possible, but we are just starting 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR is "big enough" already.
Maybe remove Erlang/OTP vulnerabilities from it (just a thought to reduce scope and simplify the journey)? To reduce the scope but have all the mechanics you need in place? Also reduce the pressure on automating the sync between advisories and OpenVex?
04e7af8 to
2b27cb6
Compare
- perform vulnerability analysis on a pull requests basis and on a
scheduled basis.
- adds the option `osv-scan` to `otp-compliance.es` to submit requests
to OSV API.
- filter CVEs found based on VEX statements
- creation of reusable Github workflow to allow direct
calls (workflow_call) and gh triggering events (workflow_dispatch).
- add script to generate OpenVex statements based on ErlangOTP release
tree, i.e., generates statements for OTP versions and OTP applications
- create range of products for otp non_affected status
with this change, introducing an erlang/otp application will fetch all
versions of the application from initial version until the last version
of the OTP release.
Example:
```
{ "pkg:otp/[email protected]": "CVE-2023-45853",
"status": { "not_affected": "vulnerable_code_not_present" }}
```
will produce erts version 14.0 - 14.2.5.11 and all OTP releases as well.
This makes sense to write for erts and other applications that rely on
third party, simply to state that Erlang is not vulnerable. in any other
case, it does not make sense as either OTP is affected by a CVE towards
OTP, or it is not.
fa474ed to
d2b09ef
Compare
GH should fix actions/dependency-review-action#923 for us to get alerts about dependencies.