Skip to content

Preserve symbol table in binaries for static analysis (govulncheck)#10325

Merged
ndeloof merged 1 commit intodocker:v2from
tianon:no-stripping
Mar 13, 2023
Merged

Preserve symbol table in binaries for static analysis (govulncheck)#10325
ndeloof merged 1 commit intodocker:v2from
tianon:no-stripping

Conversation

@tianon
Copy link
Contributor

@tianon tianon commented Feb 27, 2023

While this stripping does decrease the binary size by some amount, it also removes the ability for govulncheck (https://go.dev/blog/vuln) to scan the binary for actual uses of vulnerable functions, requiring the user to clone the code locally and hope they're testing against the same version of the stdlib, etc that the binary was built with. If we stop passing -s, then we can then run govulncheck on the binary directly (making it easier to flag both false positives in CVE scans and actual issues worth looking into).

Here's an example of the output on a freshly built binary with this change:

$ govulncheck ./bin/build/docker-compose
govulncheck is an experimental tool. Share feedback at https://go.dev/s/govulncheck-feedback.

Using govulncheck@v0.0.0 with
vulnerability data from https://vuln.go.dev (last modified 27 Feb 23 16:29 UTC).

Scanning your binary for known vulnerabilities...
No vulnerabilities found.

Compared to the 1.16.0 release binary:

$ govulncheck ./docker-compose
govulncheck is an experimental tool. Share feedback at https://go.dev/s/govulncheck-feedback.

Using govulncheck@v0.0.0 with
vulnerability data from https://vuln.go.dev (last modified 27 Feb 23 16:29 UTC).

Scanning your binary for known vulnerabilities...
govulncheck: vulncheck.Binary: reading go:func.*: no symbol "go:func.*"

It's not 100% apples-to-apples, but the size difference between these binaries is ~46MiB for the 1.16.0 release and ~52MiB for the binary I built from this commit.

While this stripping does decrease the binary size by some amount, it also removes the ability for `govulncheck` (https://go.dev/blog/vuln) to scan the binary for actual uses of vulnerable functions, requiring the user to clone the code locally and hope they're testing against the same version of the stdlib, etc that the binary was built with.  If we stop passing `-s`, then we can then run `govulncheck` on the binary directly (making it easier to flag both false positives in CVE scans _and_ actual issues worth looking into).

Here's an example of the output on a freshly built binary with this change:

```console
$ govulncheck ./bin/build/docker-compose
govulncheck is an experimental tool. Share feedback at https://go.dev/s/govulncheck-feedback.

Using govulncheck@v0.0.0 with
vulnerability data from https://vuln.go.dev (last modified 27 Feb 23 16:29 UTC).

Scanning your binary for known vulnerabilities...
No vulnerabilities found.
```

Compared to the 1.16.0 release binary:

```console
$ govulncheck ./docker-compose
go: downloading golang.org/x/vuln v0.0.0-20230224180816-edec1fb0a9c7
govulncheck is an experimental tool. Share feedback at https://go.dev/s/govulncheck-feedback.

Using govulncheck@v0.0.0 with
vulnerability data from https://vuln.go.dev (last modified 27 Feb 23 16:29 UTC).

Scanning your binary for known vulnerabilities...
govulncheck: vulncheck.Binary: reading go:func.*: no symbol "go:func.*"
```

It's not 100% apples-to-apples, but the size difference between these binaries is ~46MiB for the 1.16.0 release and ~52MiB for the binary I built from this commit.

Signed-off-by: Tianon Gravi <admwiggin@gmail.com>
@codecov
Copy link

codecov bot commented Feb 27, 2023

Codecov Report

Base: 73.89% // Head: 72.79% // Decreases project coverage by -1.11% ⚠️

Coverage data is based on head (559a248) compared to base (d4f156c).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##               v2   #10325      +/-   ##
==========================================
- Coverage   73.89%   72.79%   -1.11%     
==========================================
  Files           2        2              
  Lines         272      272              
==========================================
- Hits          201      198       -3     
- Misses         60       62       +2     
- Partials       11       12       +1     
Impacted Files Coverage Δ
pkg/e2e/framework.go 70.98% <0.00%> (-1.18%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@laurazard laurazard requested review from a team, StefanScherer, glours, laurazard, milas, ndeloof, nicksieger and ulyssessouza and removed request for a team February 28, 2023 10:22
Copy link
Contributor

@ndeloof ndeloof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion on this topic as I don't know the actual impacts doing so

Copy link
Contributor

@milas milas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - here's the full size comparisons across architectures from HEAD of v2

macOS is basically identical, Linux/Windows gain ~5-6MB as you noted

I'm ok with this as we're still removing the DWARF debug info (-w) which is much bigger [some of the binaries would be >80MB with that 😬]

(That said, I think we're probably due for a dependency review to try and shrink the binary some.)

STRIPPED

❯ du -k bin/build/*
53632	bin/build/darwin_amd64
51264	bin/build/darwin_arm64
47168	bin/build/linux_amd64
45120	bin/build/linux_arm64
44096	bin/build/linux_arm_v6
44096	bin/build/linux_arm_v7
46144	bin/build/linux_ppc64le
45120	bin/build/linux_riscv64
50240	bin/build/linux_s390x
48192	bin/build/windows_amd64
45120	bin/build/windows_arm64

UNSTRIPPED

❯ du -k bin/build/*
53312	bin/build/darwin_amd64
51808	bin/build/darwin_arm64
53312	bin/build/linux_amd64
51264	bin/build/linux_arm64
50240	bin/build/linux_arm_v6
50240	bin/build/linux_arm_v7
52288	bin/build/linux_ppc64le
52288	bin/build/linux_riscv64
56384	bin/build/linux_s390x
53312	bin/build/windows_amd64
51264	bin/build/windows_arm64

@milas milas changed the title Remove "-s" from LDFLAGS Preserve symbol table in binaries for static analysis (govulncheck) Mar 3, 2023
@ndeloof ndeloof merged commit fc4d2df into docker:v2 Mar 13, 2023
@tianon tianon deleted the no-stripping branch March 13, 2023 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants