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

x/exp/cmd/govulncheck: exclude (fixed) vulnerability info from the own project #48079

Closed
hyangah opened this issue Aug 30, 2021 · 6 comments
Closed
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@hyangah
Copy link
Contributor

hyangah commented Aug 30, 2021

govulncheck version: v0.0.0-20210830180030-b6ec30af783f

From the x/crypto repo checked out (commit: 32db794688a5a24a23a43f2a984cecd5b3d8da5), run govulncheck and see the old vulnerabilities are printed.

 % govulncheck -json ./...    
{
	"SearchMode": 0,
	"Vulnerabilities": [
		{
			"id": "GO-2020-0012",
			"published": "2021-04-14T12:00:00Z",
			"modified": "2021-04-14T12:00:00Z",
			"aliases": [
				"CVE-2020-9283"
			],
			"package": {
				"name": "golang.org/x/crypto/ssh",
				"ecosystem": "Go"
			},
			"details": "An attacker can craft an ssh-ed25519 or sk-ssh-ed25519@openssh.com public\nkey, such that the library will panic when trying to verify a signature\nwith it. If verifying signatures using user supplied public keys, this\nmay be used as a denial of service vector.\n",
			"affects": {
				"ranges": [
					{
						"type": "SEMVER",
						"fixed": "0.0.0-20200220183623-bac4c82f6975"
					}
				]
			},
			"references": [
				{
					"type": "FIX",
					"url": "https://go-review.googlesource.com/c/crypto/+/220357"
				},
				{
					"type": "FIX",
					"url": "https://github.com/golang/crypto/commit/bac4c82f69751a6dd76e702d54b3ceb88adab236"
				},
				{
					"type": "WEB",
					"url": "https://groups.google.com/g/golang-announce/c/3L45YRc91SY"
				}
			],
			"ecosystem_specific": {
				"symbols": [
					"parseED25519",
					"ed25519PublicKey.Verify",
					"parseSKEd25519",
					"skEd25519PublicKey.Verify",
					"NewPublicKey"
				],
				"url": "https://go.googlesource.com/vulndb/+/refs/heads/master/reports/GO-2020-0012.yaml"
			}
		},
		{
			"id": "GO-2020-0013",
			"published": "2021-04-14T12:00:00Z",
			"modified": "2021-04-14T12:00:00Z",
			"aliases": [
				"CVE-2017-3204"
			],
			"package": {
				"name": "golang.org/x/crypto/ssh",
				"ecosystem": "Go"
			},
			"details": "By default host key verification is disabled which allows for\nman-in-the-middle attacks against SSH clients if\n[`ClientConfig.HostKeyCallback`] is not set.\n",
			"affects": {
				"ranges": [
					{
						"type": "SEMVER",
						"fixed": "0.0.0-20170330155735-e4e2799dd7aa"
					}
				]
			},
			"references": [
				{
					"type": "FIX",
					"url": "https://go-review.googlesource.com/38701"
				},
				{
					"type": "FIX",
					"url": "https://github.com/golang/crypto/commit/e4e2799dd7aab89f583e1d898300d96367750991"
				},
				{
					"type": "WEB",
					"url": "https://github.com/golang/go/issues/19767"
				},
				{
					"type": "WEB",
					"url": "https://bridge.grumpy-troll.org/2017/04/golang-ssh-security/"
				}
			],
			"ecosystem_specific": {
				"symbols": [
					"NewClientConn"
				],
				"url": "https://go.googlesource.com/vulndb/+/refs/heads/master/reports/GO-2020-0013.yaml"
			}
		}
	],
	"VulnFindings": {
		"GO-2020-0012": [
			{
				"Symbol": "golang.org/x/crypto/ssh.NewPublicKey",
				"Position": {
					"Filename": "/Users/hakim/vultest/crypto/ssh/keys.go",
					"Offset": 23513,
					"Line": 951,
					"Column": 29
				},
				"Type": "function",
				"Trace": [
					{
						"Description": "golang.org/x/crypto/ssh.NewSignerFromSigner(...)",
						"Position": {
							"Filename": "/Users/hakim/vultest/crypto/ssh/keys.go",
							"Offset": 23425,
							"Line": 950,
							"Column": 6
						}
					}
				]
			}
		],
		"GO-2020-0013": [
			{
				"Symbol": "golang.org/x/crypto/ssh.NewClientConn",
				"Position": {
					"Filename": "/Users/hakim/vultest/crypto/ssh/client.go",
					"Offset": 4990,
					"Line": 177,
					"Column": 38
				},
				"Type": "function",
				"Trace": [
					{
						"Description": "golang.org/x/crypto/ssh.Dial(...)",
						"Position": {
							"Filename": "/Users/hakim/vultest/crypto/ssh/client.go",
							"Offset": 4786,
							"Line": 172,
							"Column": 6
						}
					}
				]
			}
		]
	}
}

cc @FiloSottile @rolandshoemaker

@gopherbot gopherbot added this to the Unreleased milestone Aug 30, 2021
@timothy-king
Copy link
Contributor

cc @zpavlinovic

@zpavlinovic
Copy link
Contributor

Thanks for reporting this!

We were actually wondering what to do with such findings. We decided to be very conservative for such cases and report them or issue a warning.

Would not reporting anything for such findings be the best option? Is that what would you expect?

@hyangah
Copy link
Contributor Author

hyangah commented Aug 31, 2021

This is new, so I don't know the right answer.

An ideal scenario is to detect whether the fix is already in the main module by inspecting the commit history. But I am not sure how feasible and how practically useful it will be.

If that's not possible, I think hiding them by default won't be too bad assuming the repo owner already should know about their own vulnerability issues and did the right thing. Otherwise, projects that once had many vulnerabilities will be permanently scarred and this tool won't be usable due to the false positives.

@FiloSottile
Copy link
Contributor

Agreed with @hyangah, if we had a way to determine the precise version of the main module, I would still not be sure vulnerabilities in it should be reported, but without version information they should absolutely be ignored. We can't show an ever-growing set of false positives to developers for their own module.

@zpavlinovic zpavlinovic self-assigned this Aug 31, 2021
@zpavlinovic zpavlinovic added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 31, 2021
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/346609 mentions this issue: vulndb/internal/audit: filter out vulns for modules with "" version

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/391914 mentions this issue: vulncheck: remove isLocal check from fetchVulnerabilities

@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants