golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.98k stars 17.67k forks source link

proposal: cmd/go: add environment variable to disallow //go:embed for modules #68020

Closed cedws closed 4 months ago

cedws commented 4 months ago

Proposal Details

After the XZ backdoor situation, developers want to scrutinise their dependencies more and take away elements of unnecessary trust.

The Go compiler is already quite good at protecting the user from malicious dependencies. My understanding is that build time code execution is explicitly disallowed and considered a vulnerability. Of course, the compiler can't protect you from malicious dependencies at runtime, but any malicious code in a dependency module would need to be in plain sight and missed by any external review.

However, //go:embed can create additional avenues for a malicious payload. The XZ backdoor was possible because of unauditable binary blobs inside of the repository. //go:embed makes it possible to embed arbitrary malicious payloads inside of the binary. The consumer of a malicious module wouldn't necessarily know about this unless they go and check for it.

I propose a new environment variable, GONOEMBED= for blacklisting specific module paths from using the //go:embed directive. To disable the embed directive for all modules from GitHub, it would look something like:

GONOEMBED=github.com/*

It may be desirable for some users to instead whitelist specific module paths. In this case, a GOEMBED= environment variable could also be used to only allow the embed directive for specific module paths.

GOEMBED=github.com/foo/*

Both GONOEMBED and GOEMBED could be used together to only allow the embed directive for specific module paths, for example, internal modules authored by the company.

GONOEMBED=github.com/*
GOEMBED=github.com/foo/*

Naturally, it may also be used for local modules.

GONOEMBED=*
GOEMBED=mymodule/*

An environment variable is preferable to a compiler flag because it can be more easily controlled and rolled out across developers' machines, such as through an MDM, shared shell configs, or using something like direnv.

When a module that uses the embed directive is under a disallowed path, this should result in a compile time error and inform the user that //go:embed has been disabled for the module.

seankhliao commented 4 months ago

I don't think this solves any real issue, if anything it may give users a false sense of security. Malicious code could just inline whatever they were going to embed into go source files.

What is the threat model that this is trying to address?

cedws commented 4 months ago

I forgot to address the obvious - why is embedding a payload directly with //go:embed any different to an inline base64/byte const? A malicious developer could just revert to the latter.

Blobs committed to a repo raise fewer alarm bells. GitHub doesn't show the diff for blobs so you don't know what has changed. They're also harder to spot in code review because they're collapsed by default. I think it's a similar story for other code forges. If you see a delta of a few thousand lines of base64 in a Go source file, that is very immediately suspicious.

There's also more tooling for detecting suspicious code deltas. It's easier to do static analysis for.

dominikh commented 4 months ago

Go modules can already include various binary blobs, such as object files.

cedws commented 4 months ago

@dominikh Do you mean through embedding the data in consts? Or some other mechanism?

thediveo commented 4 months ago

Makes ebpf pretty unusable in Go, cilium project might not be amused. Their great ebpf for Go module embeds ebpf .o files for little and big endian platforms, highly useful.

dominikh commented 4 months ago

@cedws by storing .syso files in the file system. Or, if using cgo, by storing and linking .a, .so, etc files. See for example https://github.com/rajveermalviya/go-webgpu/blob/a1aba016d10b72db8d3955eeb733580974950212/wgpu/wgpu.go#L6 and https://github.com/rajveermalviya/go-webgpu/tree/main/wgpu/lib/android/amd64, or https://github.com/golang/go/blob/fe36ce669c1a452d2b0e81108a7e07674b50692a/src/runtime/race/race_darwin_arm64.syso.

cedws commented 4 months ago

@dominikh Ah ok, yeah. Not much benefit if they can do that instead. Thanks.

timothy-king commented 4 months ago

You can write a static analysis pass using https://pkg.go.dev/golang.org/x/tools/go/analysis to collect the package+module information of all of the //go:embed directives your package transitively depends on. You can then enforce any policy you think is reasonable on your dependencies.

cedws commented 4 months ago

Thanks Timothy, this is nice.