Add vulnerability scanning with OSV scanner

[faern]

Adding (yet another) scanner for known vulnerabilities in our dependency tree(s). This one actually checks some places we have not looked at before.

Draft until we have figured out what lockfiles we want to check (if not all) and fixed or ignored all existing reported vulns.

The vulns it reports can be found here: https://github.com/mullvad/mullvadvpn-app/security/code-scanning?page=1&query=pr%3A6001+is%3Aopen

---

This change is 

[linear]

DES-706 Integrate OSV-scanner into CI

[github-advanced-security]

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.

[albin-mullvad]

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @faern)

---

osv-scanner.toml line 10 at r1 (raw file):

# * The default should be to ignore a vulnerability for three months.
# * A vulnerability can be ignored for up to a year at most (Use extremely sparsely).
# * If anything above three months is used, write a short comment about why this ignore is longer.

I would prefer keeping the reason for longer ignores in the reason block since:

• For longer reasons multiple lines might be necessary.
• The reason information might be relevant in generated reports and/or on the GH Security tab/page. I'm not sure how it works, but might be worth considering.

I don't have a super strong opinon though, so I'm also fine going with the suggested approach.

---

.github/workflows/osv-scanner.yml line 2 at r1 (raw file):

---
# Copyright 2024 Google LLC

Just to double check, do we need to include this copyright for such a trivial configuration file?

---

.github/workflows/osv-scanner.yml line 19 at r1 (raw file):

on:
  pull_request:

Just a thought here: we could limit this to same paths as the signature lockfile protection, right? Even though this workflow is quite lite it would be nice to limit the amount of workflows triggered per PR since we already have a ton

---

.github/workflows/osv-scanner.yml line 33 at r1 (raw file):

  # Only need to read contents
  contents: read
  # Needed to read the workflow from another repository(???)

TODO?

Code quote:

???

---

gui/scripts/osv-scanner.toml line 17 at r1 (raw file):

# libwebp bundled with Pillow is vulnerable to CVE-2023-5129
[[IgnoredVulns]]
id = "GHSA-56pw-mpj4-fxww"

Should this be CVE-2023-5129? 🤔

Code quote:

GHSA-56pw-mpj4-fxww

---

test/osv-scanner.toml line 0 at r1 (raw file):
Just a reminder to add the following:

# See repository root `osv-scanner.toml` for instructions and rules for this file.

[albin-mullvad]

Reviewed 1 of 1 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @faern)

---

-- commits line 32 at r4:
Feel free to squash these with the prior commit (Ignore android vulnerabilities reported by osv-scanner) 🙂

Code quote:

- 6d3d928 by Albin (albin-mullvad):
  Move android osv-scanner config to gradle dir

- 684004a by Albin (albin-mullvad):
  Try fix name reference

---

test/osv-scanner.toml line 1 at r4 (raw file):

# See repository root `osv-scanner.toml` for instructions and rules for this file.

Reviewable seem to complain about the file ending

[faern]

Reviewed 1 of 1 files at r2, 1 of 1 files at r3.
Reviewable status: 6 of 12 files reviewed, 5 unresolved discussions (waiting on @albin-mullvad)

---

gui/scripts/osv-scanner.toml line 17 at r1 (raw file):

Previously, albin-mullvad wrote…

Should this be CVE-2023-5129? 🤔

It does not list that as an alias on osv.dev. That CVE is part of the references, but it's not the only reference. If I change it to the CVE then osv-scanner does not understand the ignore and warn about it as it's not an alias.
Not all IDs need to be CVEs, it's just recommended to try to use the original ID when available.

---

test/osv-scanner.toml line at r1 (raw file):

Previously, albin-mullvad wrote…

Just a reminder to add the following:

# See repository root `osv-scanner.toml` for instructions and rules for this file.

Done!

---

test/osv-scanner.toml line 1 at r4 (raw file):

Previously, albin-mullvad wrote…

Reviewable seem to complain about the file ending

Oops. Done.

---

.github/workflows/osv-scanner.yml line 2 at r1 (raw file):

Previously, albin-mullvad wrote…

Just to double check, do we need to include this copyright for such a trivial configuration file?

I now replaced all of this with their recommended setup from https://google.github.io/osv-scanner/github-action/#scan-on-pull-request instead. I hope this works nicer and has less boilerplate.

---

.github/workflows/osv-scanner.yml line 19 at r1 (raw file):

Previously, albin-mullvad wrote…

Just a thought here: we could limit this to same paths as the signature lockfile protection, right? Even though this workflow is quite lite it would be nice to limit the amount of workflows triggered per PR since we already have a ton

Is there another way than duplicating the relevant subset of that list? That makes it slightly error prone towards missing new/moved/renamed lockfiles.

---

android/osv-scanner.toml line 66 at r2 (raw file):

[[IgnoredVulns]]
name = "org.bouncycastle:bcpkix-jdk18on"

This has a name but no id. I have not seen this before. Is there no ID to assign it or how come this is so different? 🤔

[albin-mullvad]

Reviewed 5 of 5 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @faern)

---

.github/workflows/verify-locked-down-signatures.yml line 28 at r5 (raw file):

      - building/android-container-image.txt
      - building/sigstore/**
      - '**/osv-scanner.toml'

I'm not sure the ignore path is needed here since no other path should include osv-scanner.toml files? 🤔 But I guess it doesn't hurt

---

gui/scripts/osv-scanner.toml line 17 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

It does not list that as an alias on osv.dev. That CVE is part of the references, but it's not the only reference. If I change it to the CVE then osv-scanner does not understand the ignore and warn about it as it's not an alias.
Not all IDs need to be CVEs, it's just recommended to try to use the original ID when available.

Alright 👍

---

.github/workflows/osv-scanner.yml line 2 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I now replaced all of this with their recommended setup from https://google.github.io/osv-scanner/github-action/#scan-on-pull-request instead. I hope this works nicer and has less boilerplate.

👍

---

.github/workflows/osv-scanner.yml line 19 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Is there another way than duplicating the relevant subset of that list? That makes it slightly error prone towards missing new/moved/renamed lockfiles.

Yeah, I guess that might not be possible since we didn't use that in the signature verification workflow 🤔 I guess we can skip it for now

---

android/osv-scanner.toml line 66 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

This has a name but no id. I have not seen this before. Is there no ID to assign it or how come this is so different? 🤔

There were tons of vulnerabilities for this crypto library that we also don't rely on in the production app (since all crypto is done in rust and go) so I therefore just ignore all for its name until we've investigated those further during the coming month

[faern]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @albin-mullvad)

---

osv-scanner.toml line 10 at r1 (raw file):

For longer reasons multiple lines might be necessary.

I don't see why you could not put multi line comments about the same thing? I don't understand this argument.

The reason information might be relevant in generated reports and/or on the GH Security tab/page. I'm not sure how it works, but might be worth considering.

Yes, the structured reason is nice that it can be included in reports I guess. But this does not talk about the "reason" for the expiry time. Confusing with two things named "reason". I don't understand how this is an argument for including the expiry time here.

I see the reason field as why can we safely ignore this, and that usually has nothing to do with time. It's more of "We can ignore this because we don't call that function" or something like this. The time aspect is more of "After time X we need to re-evaluate whether or not we call into the bad function".

I don't have a super strong stance here neither. But it just feels like the reason is not time sensitive, and when we bump an expiry time usually the reason a CVE can be ignored does not change, but the argument for the new expiry time might.

---

.github/workflows/verify-locked-down-signatures.yml line 28 at r5 (raw file):

Previously, albin-mullvad wrote…

I'm not sure the ignore path is needed here since no other path should include osv-scanner.toml files? 🤔 But I guess it doesn't hurt

This entry makes the osv-scanner files included. I wanted changes to these files to require signatures, since they are affecting the security of the supply chain. They should also be changed rather infrequently.

But sadly that did not work. The parsing of this list in the bash script was not smart enough :( So I'll remove it for now, maybe we can fix it later, it's not critical.

[albin-mullvad]

Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @faern)

---

osv-scanner.toml line 10 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

For longer reasons multiple lines might be necessary.

I don't see why you could not put multi line comments about the same thing? I don't understand this argument.

The reason information might be relevant in generated reports and/or on the GH Security tab/page. I'm not sure how it works, but might be worth considering.

Yes, the structured reason is nice that it can be included in reports I guess. But this does not talk about the "reason" for the expiry time. Confusing with two things named "reason". I don't understand how this is an argument for including the expiry time here.

I see the reason field as why can we safely ignore this, and that usually has nothing to do with time. It's more of "We can ignore this because we don't call that function" or something like this. The time aspect is more of "After time X we need to re-evaluate whether or not we call into the bad function".

I don't have a super strong stance here neither. But it just feels like the reason is not time sensitive, and when we bump an expiry time usually the reason a CVE can be ignored does not change, but the argument for the new expiry time might.

Alright, let's got with the current approach and revisit the topic in case it doesn't work

---

.github/workflows/verify-locked-down-signatures.yml line 28 at r5 (raw file):

Previously, faern (Linus Färnstrand) wrote…

This entry makes the osv-scanner files included. I wanted changes to these files to require signatures, since they are affecting the security of the supply chain. They should also be changed rather infrequently.

But sadly that did not work. The parsing of this list in the bash script was not smart enough :( So I'll remove it for now, maybe we can fix it later, it's not critical.

Ah, sorry, my mistake! 🙈

[faern]

Reviewable status: 11 of 12 files reviewed, 1 unresolved discussion

---

osv-scanner.toml line 10 at r1 (raw file):
FYI: One place the reason is used is when running from the CLI. Then it will print something like:

CVE-2023-44271 and 3 aliases have been filtered out because: Only used internally, on trusted input. This tool is also scheduled for removal completely, so not worth trying to upgrade

[albin-mullvad]

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion

[faern]

This PR is now more or less ready for final review. The things missing are:

• Some entries in gui/osv-scanner.toml have inadequate reasons and have not been proven to be solvable by a package upgrade yet. Awaiting input from @raksooo
• rexml in ci/ios/upload-vm/Gemfile.lock is still a problem. Awaiting an upgrade of this package from @buggmagnet FIXED

[albin-mullvad]

Reviewed 3 of 3 files at r9, all commit messages.
Reviewable status: 11 of 13 files reviewed, 1 unresolved discussion (waiting on @faern)

[raksooo]

Reviewed 1 of 4 files at r1, 3 of 4 files at r4, 3 of 5 files at r5, 1 of 1 files at r6, 2 of 2 files at r7, 2 of 3 files at r8.
Reviewable status: 11 of 13 files reviewed, 3 unresolved discussions (waiting on @faern)

---

gui/osv-scanner.toml line 14 at r10 (raw file):

id = "CVE-2020-7608" # GHSA-p9pc-299p-vxgp
ignoreUntil = 2024-09-05
reason = ""

This package is used to parse commands run by either us or trusted libraries.

---

gui/osv-scanner.toml line 26 at r10 (raw file):

id = "CVE-2024-4068" # GHSA-grv7-fg5c-xmjg
ignoreUntil = 2024-09-05
reason = ""

reason:

This package is only used to match paths from either us or trusted libraries.

[faern]

Reviewable status: 10 of 13 files reviewed, 2 unresolved discussions (waiting on @albin-mullvad and @raksooo)

---

gui/osv-scanner.toml line 14 at r10 (raw file):

Previously, raksooo (Oskar Nyberg) wrote…

This package is used to parse commands run by either us or trusted libraries.

Done.

---

gui/osv-scanner.toml line 26 at r10 (raw file):

Previously, raksooo (Oskar Nyberg) wrote…

reason:

This package is only used to match paths from either us or trusted libraries.

Done.

---

android/gradle/osv-scanner.toml line 74 at r11 (raw file):

ecosystem = "Maven"
ignore = true
effectiveUntil = 2024-08-02

Wow. Accidentally missed another one again! Unless they fix this in osv-scanner itself we must almost look into other ways of sanitizing these config files for errors that make us ignore things forever 🙈 google/osv-scanner#1098

[faern]

Reviewed 1 of 2 files at r11, all commit messages.
Reviewable status: 10 of 13 files reviewed, 2 unresolved discussions (waiting on @raksooo)