[v1.6] Remove usages of an old versions of github.com/emicklei/go-restful

[mkevac]

Description

There is a still usage on an old (and vulnerable) version of github.com/emicklei/go-restful in pkg/cri/streaming/server.go

➜  containerd git:(v1.6.18) rg "go-restful" go.mod
29:	github.com/emicklei/go-restful v2.9.5+incompatible
➜  containerd git:(v1.6.18) go mod why -m github.com/emicklei/go-restful
# github.com/emicklei/go-restful
github.com/containerd/containerd/pkg/cri/streaming
github.com/emicklei/go-restful
➜  containerd git:(v1.6.18) rg "github.com/emicklei/go-restful" pkg/cri
pkg/cri/streaming/server.go
48:	restful "github.com/emicklei/go-restful"

Current version of github.com/emicklei/go-restful is v3

Steps to reproduce the issue

No response

Describe the results you received and expected

I expected v3 version to be used.

What version of containerd are you using?

v1.6.18

Any other relevant information

No response

Show configuration if it is related to CRI plugin.

No response

[Anis-cpu-13]

Hello, as part of a university project and a first contribution to open source software, I would like to work on this issue

[AkihiroSuda]

Hello, as part of a university project and a first contribution to open source software, I would like to work on this issue

Yes, you can cherry pick this
481fb92

[Abhishek-569]

Is this issue fixed? I can't find the old version in pkg/cri/streaming/server.go.

[estesp]

I just opened #8271 to solve this

[mmerkes]

@mkevac Looks like this issue can be resolved?

[matrey]

Seems like there are still traces of go-restful v3.7.3, which is also vulnerable to CVE-2022-1996 (need to use at least v3.8.0)

[mmerkes]

Makes sense. #8221 up to resolve it!

[AkihiroSuda]

CVE-2022-1996

containerd is not affected.
The affected component (CrossOriginResourceSharing, emicklei/go-restful@fd3c327) is not used in containerd.

[matrey]

Yes sorry for the shortcut, I didn't mean to imply there is any actual risk in relation to this CVE. But scanning tools used for compliance (Prisma Cloud in my case) see a go-restful v3.7.3 inside containerd and don't like it.

[samuelkarp]

You should report a bug to your scanning tool vendor. The govulncheck tool can be used to verify whether a given binary invokes code affected by a vulnerability. https://pkg.go.dev/vuln/GO-2022-0619 is the relevant Go vulnerability report and is not detected by govulncheck.

[jpmcb]

You should report a bug to your scanning tool vendor.

@samuelkarp - while I agree that contaienrd is not vulnerable, most security scanning tools will only go so deep and ask "what packages are being pulled in?"

I would call this a transitive vulnerability since contaienrd does carry an older version of the software that happens to have a CVE in a block that is not currently getting called into. I would say it's not enough to dismiss this: what if a contributor someday does decide they need to use that function an accidentally introduce vulnerable code? What if a CI/CD workflow is kicked off that has a change which has the malicious piece of code? There are alot of (very unlikely) scenarios where carrying an old dependency is a bad idea.

Regardless, on the main branch, old very versions of go-restful seem to still be present in the go.sum:

❯ git show HEAD
commit 54732fa9fb921b98f4fc5d96025307cc807f04d9 (HEAD -> main, origin/main, origin/HEAD)
Merge: b27301cd0 27c0fe3eb
Author: Derek McGowan <derek@mcg.dev>
Date:   Thu Apr 27 08:05:30 2023 -0700

    Merge pull request #8449 from thaJeztah/runc_binary_1.1.7

    update runc binary to v1.1.7

❯ go mod graph | rg github.com/emicklei/go-restful@v2.9.5
github.com/containerd/containerd@v1.6.8 github.com/emicklei/go-restful@v2.9.5+incompatible
github.com/containerd/containerd@v1.5.0-beta.3 github.com/emicklei/go-restful@v2.9.5+incompatible
github.com/containerd/containerd@v1.5.7 github.com/emicklei/go-restful@v2.9.5+incompatible
k8s.io/apiserver@v0.22.5 github.com/emicklei/go-restful@v2.9.5+incompatible
k8s.io/apiserver@v0.20.1 github.com/emicklei/go-restful@v2.9.5+incompatible
k8s.io/apiserver@v0.20.6 github.com/emicklei/go-restful@v2.9.5+incompatible
k8s.io/code-generator@v0.19.7 github.com/emicklei/go-restful@v2.9.5+incompatible
github.com/containerd/containerd@v1.6.1 github.com/emicklei/go-restful@v2.9.5+incompatible
github.com/containerd/containerd@v1.5.0-beta.1 github.com/emicklei/go-restful@v2.9.5+incompatible
github.com/containerd/containerd@v1.5.1 github.com/emicklei/go-restful@v2.9.5+incompatible
github.com/containerd/containerd@v1.5.0-rc.0 github.com/emicklei/go-restful@v2.9.5+incompatible
github.com/containerd/containerd@v1.5.0-beta.4 github.com/emicklei/go-restful@v2.9.5+incompatible
k8s.io/apiserver@v0.20.4 github.com/emicklei/go-restful@v2.9.5+incompatible
github.com/containerd/containerd@v1.5.8 github.com/emicklei/go-restful@v2.9.5+incompatible

[samuelkarp]

There's ongoing discussion in #8412, so I'm only going to reply from a philosophical perspective rather than address this particular dependency.

while I agree that contaienrd is not vulnerable, most security scanning tools will only go so deep and ask "what packages are being pulled in?"

Yes, and I would consider this to be a bug in the scanner. While a tool like govulncheck cannot eliminate false positives, it does dramatically reduce them through symbol analysis.

I would call this a transitive vulnerability since contaienrd does carry an older version of the software that happens to have a CVE in a block that is not currently getting called into.

I think we'll have to disagree there. If the code is not getting invoked, no behavior of that code (features, bugs, or vulnerabilities) is reflected in the behavior of containerd.

I would say it's not enough to dismiss this: what if a contributor someday does decide they need to use that function an accidentally introduce vulnerable code? What if a CI/CD workflow is kicked off that has a change which has the malicious piece of code?

Code review and maintenance is an ongoing activity of all the maintainers of this project. We do our best, but of course things can slip through. One idea that @tianon had on another project was to set up automation to run govulncheck and alert when something is found; we could do that as both a cron job and as part of our regular CI.

There are alot of (very unlikely) scenarios where carrying an old dependency is a bad idea.

For a stable branch of software, the focus tends to be more on reducing the overall churn in order to reduce the risk of regressions. In the absence of a relevant bug fix or security fix, the risk of introducing other, unnecessary change tends (to me) to outweigh the value of staying current.

I do agree that it's generally worth updating dependencies on an actively-developed branch (like main).

Regardless, on the main branch, old very versions of go-restful seem to still be present in the go.sum

The values in go.sum can be misleading. See the documentation:

The go.sum file may contain hashes for multiple versions of a module. The go command may need to load go.mod files from multiple versions of a dependency in order to perform minimal version selection. go.sum may also contain hashes for module versions that aren’t needed anymore (for example, after an upgrade).

govulncheck will analyze sources and a binary to tell whether a particular symbol from a particular version is included.

[fuweid]

closed by #8412