otp icon indicating copy to clipboard operation
otp copied to clipboard

otp scan PRs for vulnerabilities

Open kikofernandez opened this issue 7 months ago • 9 comments

  • scan PRs for vendor vulnerabilities.
  • the submission of the vendor SBOM should happen only on push events.
  • vulnerability scanning of dependencies must happen on a per PR basis, and on a per push basis (although Dependatbot should inform us of this).

GH should fix https://github.com/actions/dependency-review-action/issues/923 for us to get alerts about dependencies.

kikofernandez avatar May 02 '25 07:05 kikofernandez

CT Test Results

    38 files   1 006 suites   7h 25m 15s ⏱️ 12 565 tests 11 762 ✅   802 💤 1 ❌ 26 975 runs  24 837 ✅ 2 137 💤 1 ❌

For more details on these failures, see this check.

Results for commit d2b09efe.

:recycle: 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

github-actions[bot] avatar May 02 '25 07:05 github-actions[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?

garazdawi avatar May 04 '25 20:05 garazdawi

I think you are right @garazdawi I have updated the PR to scan for vulnerabilities using OSV.

Summary

  • CC++ works better in OSV providing a sha, and they do range analysis.
  • Our vendor libraries should provide a versionInfo, and in the case of CC++ vendor libraries, we should provide the sha as well. If there is no sha, then we cannot do vulnerability scanning for this CC++ vendor library.
  • If a vendor library belongs to an ecosystem, we write its ecosystem in the vendorInfo. OSV works better for standard ecosystems, such as npm, maven, hex, etc.

I have tested that it works by manually hand-picking previous commit that fixed a known vulnerability reported to on github repos.

Information sent

{
  "queries": [
    {
      "commit": "1a8db63788c34a50e39e273d39b7e1033208aea2",
      "package": { "name": "github.com/madler/zlib" }
    },
    {
      "commit": "a465fe71ab3d0e224b2b4bd0fac69ae68ab9239d",
      "package": { "name": "github.com/asmjit/asmjit" }
    },
    {
      "commit": "f8745da6ff1ad1e7bab384bd1f9d742439278e99",
      "package": { "name": "github.com/facebook/zstd" }
    },
    {
      "commit": "442029c6fa37f1b6f9203357de09672d5704077c",
      "package": { "name": "github.com/microsoft/STL" }
    },
    {
      "commit": "1264a946ba66eab320e927bfd2362e0c8580c42f",
      "package": { "name": "github.com/ulfjack/ryu" }
    },
    {
      "commit": "cdfb0923a66155ce97640fca68ae57b3a2972029",
      "package": { "name": "github.com/openssl/openssl" }
    },
    {
      "commit": "1e09555d6950bfcf83bd98fa597b0c6440d43c9c",
      "package": { "name": "github.com/PCRE2Project/pcre2" }
    },
    {
      "commit": "636dfadc70ce26f2473870570bfd9ec352806b1d",
      "package": { "name": "github.com/openssl/openssl" }
    },
    {
      "package": {
        "ecosystem": "npm",
        "name": "tablesorter"
      },
      "version": "2.32"
    },
    {
      "package": {
        "ecosystem": "npm",
        "name": "jquery"
      },
      "version": "3.7.1"
    },
    {
      "commit": "fd0f60daea24e9c62d372d774be9e32ce2b0849d",
      "package": { "name": "github.com/wxWidgets/wxWidgets" }
    }
  ]
}

Result

[OSV] There are existing vulnerabilities:
- github.com/madler/zlib: CVE-2023-45853
- github.com/openssl/openssl: CVE-2024-12797
- github.com/PCRE2Project/pcre2: OSV-2024-1237
- github.com/wxWidgets/wxWidgets: CVE-2024-58249

kikofernandez avatar May 05 '25 19:05 kikofernandez

The vendor vulnerability scanning now fails because I changed the vendor.json sha to refer to existing CVEs, and this PR detects them (https://github.com/erlang/otp/actions/runs/14845728041/job/41679578778?pr=9790 for otp, and this for my fork https://github.com/kikofernandez/otp/actions/runs/14845971887/job/41680388662)

kikofernandez avatar May 06 '25 06:05 kikofernandez

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.

I have updated the scripts and this is the end result, using OSV for vulnerability scanning.

  1. Perform vulnerability scanning per PR. If a CVE is found the PR fails that test
  2. Perform vulnerability scanning as per scheduled and fan out per branch. If a CVE is found, create a SARIF file and upload to GH, such that GH Security contains those under Code Scanning on a per last three release basis. (we avoid duplicates via fingerprinting, and postpone the OpenVex integration for now)

kikofernandez avatar May 23 '25 14:05 kikofernandez

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Jun 10 '25 17:06 CLAassistant

This is the expected result whenever there is a CVE found. https://github.com/kikofernandez/otp/actions/runs/15716372559/job/44286988466

Checking OpenVex statements in 'vex/otp-28.openvex.json'...
OpenVex statements found.
Exiting known vulnerabilities already open:
- github.com/madler/zlib: CVE-2023-45853
- github.com/PCRE2Project/pcre2: OSV-2025-300
- github.com/wxWidgets/wxWidgets: CVE-2024-58249
[Vulnerability] Contact OTP team.
The following CVEs must be checked in OpenVex statements for master:
- github.com/openssl/openssl: CVE-2025-4575

What it says is that it has detected a possible CVE for which the OTP team has not created a VEX statement, thus, it is not clear if ErlangOTP is affected. They way to proceed is to inform OTP team, OTP team pushes a VEX statement about it, and the PR should be re-run (which will pick up the latest master statements and be good to go).

I have removed all the SARIF generation because it was producing more issues than anything else, and instead we use OpenVex as source of truth to confirm if we are affected by CVEs.

kikofernandez avatar Jun 17 '25 19:06 kikofernandez

The current PR will fail in the reusable workflow because it looks for master to contain openvex statements, and that cannot happen until this PR is merged.

kikofernandez avatar Jun 17 '25 19:06 kikofernandez

This is the first time I see the OTP purl pkg:otp/[email protected] used as referencing an OTP release. Is this really the right way to express that? Right now I believe the second "path" component is defined as describing an OTP "application". If we assign a special meaning to the value "erlang" in this position, it means there can never be an application by that name.

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 pkg:otp/%[email protected]?

voltone avatar Aug 21 '25 07:08 voltone

thanks for the feedback.

  • And yes, I agree with your insights. That said whether that's an issue under the otp type or not, I am not sure... I do not think that Erlang/OTP, Elixir, nor Gleam will ever name an application erlang; for the same reason that no one should create an application name Elixir that resides under otp/elixir :)

    I took the naming based on EEF convention that Elixir is under pkg:otp/[email protected] (https://security.erlef.org/specs/otp_purl_type)

  • Thanks for the suggestion. I am not too fond of the %40 :)

  • I see that in the OTP SBOM we place OTP releases as pkg:github/erlang/[email protected]" Just to be able to merge the PR and start our trial period, I can change it to be consistent with SBOM purls.

    We can adjust once we take a final decision. I think that using the github package convention will always be valid, but I would like some consistency between how we name things. If Elixir and Gleam are under otp/elixir and otp/gleam, it makes sense to me that the Erlang language becomes also under the same type :)

kikofernandez avatar Aug 21 '25 07:08 kikofernandez

I took the naming based on EEF convention that Elixir is under pkg:otp/[email protected] (https://security.erlef.org/specs/otp_purl_type)

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.

Just to be able to merge the PR and start our trial period, I can change it to be consistent with SBOM purls.

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

voltone avatar Aug 21 '25 07:08 voltone

I took the naming based on EEF convention that Elixir is under pkg:otp/[email protected] (https://security.erlef.org/specs/otp_purl_type)

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.

Just to be able to merge the PR and start our trial period, I can change it to be consistent with SBOM purls.

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

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. Then I agree with the reasoning and to keep it consistent

Thanks!

kikofernandez avatar Aug 21 '25 08:08 kikofernandez

Summing up my thoughts I shared on Slack:

1. Scope of otp purls

  • The otp purl type was designed to identify OTP applications, not entire Erlang/OTP releases.
  • For example, pkg:otp/elixir refers to the Elixir OTP application (which lives in lib/elixir in the Elixir repo), while the Elixir project itself is pkg:github/elixir-lang/elixir.
  • Erlang is not an OTP application—it’s a collection of them (e.g. kernel, stdlib, etc.).

2. No special cases

  • Assigning a special meaning to pkg:otp/erlang or pkg:otp/@erlang would introduce exceptions into the spec, which I’d prefer to avoid.
  • I’d rather not create special cases in the purl format just to deal with one-off scenarios.

3. Using the application module

  • One of the design goals is that any otp purl should correspond to something you can handle via the application module in Erlang.
  • That doesn’t apply to an entire release, so pkg:otp/erlang doesn’t fit.

4. Representing releases

  • OTP releases are better represented using their actual source location, e.g. pkg:github/erlang/[email protected].

  • In an SBOM, we can include multiple external references to cover various distributions or identifiers, for example:

This keeps the semantics clear: pkg:otp/... is for OTP applications, while pkg:github/... (or others) is for releases or distributions.

maennchen avatar Aug 21 '25 08:08 maennchen

I also did a tiny change to the flowchart in https://github.com/package-url/purl-spec/pull/472 to better reflect the things stated above.

maennchen avatar Aug 21 '25 08:08 maennchen

Thanks for your input as well @maennchen . I will follow this advice, I think it is good and I misunderstood the meaning of the PURL otp.

kikofernandez avatar Aug 21 '25 08:08 kikofernandez