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

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

Closed
cedws opened this issue Jun 15, 2024 · 9 comments
Closed
Labels

Comments

@cedws
Copy link

cedws commented Jun 15, 2024

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.

@cedws cedws added the Proposal label Jun 15, 2024
@seankhliao
Copy link
Member

seankhliao commented Jun 15, 2024

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
Copy link
Author

cedws commented Jun 15, 2024

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
Copy link
Member

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

@cedws
Copy link
Author

cedws commented Jun 15, 2024

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

@thediveo
Copy link

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.

@cedws
Copy link
Author

cedws commented Jun 15, 2024

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

@cedws cedws closed this as not planned Won't fix, can't repro, duplicate, stale Jun 15, 2024
@timothy-king
Copy link
Contributor

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
Copy link
Author

cedws commented Jun 15, 2024

Thanks Timothy, this is nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants