trivy icon indicating copy to clipboard operation
trivy copied to clipboard

bug(gobinary): incorrect ldflags parsing when `version` part has prefix

Open DmitriyLewen opened this issue 1 year ago • 8 comments

Description

Trivy detects different versions of Gobinary modules during multiple scans. This happens when gobinari uses 2 ldflags with the suffix versions. e.g.:

	build	-ldflags="-X github.com/argoproj/argo-cd/v2/common.version=2.11.0 
 -X github.com/argoproj/argo-cd/v2/common.kubectlVersion=v0.26.11 
-X \"github.com/argoproj/argo-cd/v2/common.extraBuildInfo=\" -extldflags \"-static\""

Discussed in https://github.com/aquasecurity/trivy/discussions/6669

DmitriyLewen avatar May 16 '24 05:05 DmitriyLewen

https://github.com/aquasecurity/trivy/pull/6705 addresses the case of ArgoCD, but a bigger problem is the inconsistent result. In the following case, we will face the same problem.

-X github.com/argoproj/argo-cd/v2/common.version=2.11.0
-X github.com/argoproj/argo-cd/v2/kubectl.version=v0.26.11 

What if we collect all versions and select one of them deterministically? Or return an error, like several versions found?

knqyf263 avatar May 17 '24 07:05 knqyf263

Or return an error, like several versions found?

I think this is bad idea. I think it's not their own binaries that users are scanning, so they can't update ldflags. And then users will not be able to scan binaries with multiple *.version=x.x.x flags.

What if we collect all versions and select one of them deterministically?

I didn't find many binaries using ldflags. I saw main, version, common. I didn't notice this, but I guess we can also check module name (something like that -X https://github.com/aquasecurity/trivy/trivy.version=v0.51.0

I suggest the following order <module_name> => common => main =>version.

DmitriyLewen avatar May 20 '24 10:05 DmitriyLewen

And then users will not be able to scan binaries with multiple *.version=x.x.x flags.

First of all, the scan does not fail because analyzer errors are always converted into warnings. Next, when I said return error, I meant leave the version empty, rather than select a version that could be wrong on our end.

I didn't find many binaries using ldflags.

How do they embed versions without using ldflags?

knqyf263 avatar May 20 '24 11:05 knqyf263

How do they embed versions without using ldflags?

Sorry, my brain isn't working well today. I checked installed in my PC binaries which were installed using go install command. Obviously these binaries do not use ldflags.

First of all, the scan does not fail because analyzer errors are always converted into warnings. Next, when I said return error, I meant leave the version empty, rather than select a version that could be wrong on our end.

Got it. Then this solution looks good. I tried to discover some logic that we can use to detect core packages, but ldglag is so custom that all users use their own logic... e.g. go-containerregistry uses github.com/google/go-containerregistry/cmd/crane/cmd.Version + github.com/google/go-containerregistry/pkg/v1/remote/transport.Version (path not included "transport"). Goreleaser has a default value - https://goreleaser.com/cookbooks/using-main.version/, but it is easy to configure.

It also looks like we need to parse URL. e.g. user might use version.Version=x.x.x for two modules in their repository:

  • github.com/owner/cmd/module/version.Version=2.0.0
  • github.com/owner/pkg/submodule/submodule/version.Version=1.0.0

It looks like you are right and best solution is to skip version for binaries with more than one version in ldflags.

DmitriyLewen avatar May 20 '24 13:05 DmitriyLewen

I didn't notice this, but I guess we can also check module name (something like that -X https://github.com/aquasecurity/trivy/trivy.version=v0.51.0

As you suggested, it may be a good idea to increase the priority if -X starts with the module name and /cmd/, like github.com/google/go-containerregistry/cmd. If we still find more than one applicable version, it should leave it empty.

knqyf263 avatar May 21 '24 02:05 knqyf263

Just to confirm that I understood you correctly:

  1. github.com/<owner>/<repo>/cmd/*/*.version=x.x.x format

    • format only:
      • github.com/google/go-containerregistry/cmd/crane/any.Version=1.0.0 => 1.0.0.
      • (cmd from non-root dir): github.com/google/go-containerregistry/v1/cmd/crane/any.Version=1.0.0 => step 2
    • format + hardcoded prefix:
      • github.com/google/go-containerregistry/cmd/crane/any.Version=1.0.0 + github.com/google/go-containerregistry/pkg/v1/remote/main.2.0.0 => 1.0.0 or empty?
    • format + another prefix:
      • github.com/google/go-containerregistry/cmd/crane/any.Version=1.0.0 + github.com/google/go-containerregistry/pkg/v1/remote/transport.2.0.0 => 1.0.0 or empty?
  2. hardcoded prefixes (main, common, version, cmd):

    • one version with hardcoded prefix:
      • common.Version=1.0.0 => 1.0.0
    • hardcoded + another prefixes:
      • common.Version=1.0.0 + transport.Version=2.0.0 => 1.0.0
    • two hardcoded prefixes:
      • common.Version=1.0.0 + main.Version=2.0.0 => empty version.
  3. other prefixes

    • one version:
      • kubectl.Version=1.0.0 => 1.0.0
    • two version:
      • terraform.Version=1.0.0 + kubectl.Version=2.0.0 => empty version.

DmitriyLewen avatar May 21 '24 07:05 DmitriyLewen

In any case, it is a heuristic, so it is simply how we define it. If there is more than one matching version in the same category (1, 2, 3 as you described), make the version empty. And in order of priority from the top.

  1. github.com/<owner>/<repo>/cmd/*/*.version=x.x.x format
  2. hardcoded prefixes (main, common, version, cmd):
  3. other prefixes
graph TD
   A{Number of versions matching 1} -->|0| B{Number of versions matching 2}
   A -->|1| C[Adopt that version]
   A -->|2+| D[Empty]
   
   B -->|0| E{Number of versions matching 3}
   B -->|1| F[Adopt that version]
   B -->|2+| G[Empty]
   
   E -->|0| H[Empty]
   E -->|1| I[Adopt that version]
   E -->|2+| J[Empty]
  • format + hardcoded prefix:
    • github.com/google/go-containerregistry/cmd/crane/any.Version=1.0.0 + github.com/google/go-containerregistry/pkg/v1/remote/main.2.0.0 => 1.0.0 or empty?

Since there is only one version matching the category 1, we take 1.0.0.

  • format + another prefix:

same as above.

knqyf263 avatar May 21 '24 10:05 knqyf263

Actually, all categories must start with the module name and end with version or ver. So, I think we can simplify them.

  1. Include /cmd/ path after the module name
  2. hardcoded prefixes (main, common, version, cmd):
  3. other prefixes

knqyf263 avatar May 21 '24 10:05 knqyf263