Security review: validate application names

[natalieparellano]

Description

In the security review, this is HIGH-1: Host compromise by overwriting trusted container images. The action plan asks us to ensure that

The CNB platform should prevent users from creating final application images having the same tags as trusted builders or as the trusted lifecycle image used when building applications

Proposed solution

Before executing a build, pack should read trusted builder information (pack config trusted-builders) and fail if the application name is equal to any of:

• any trusted builder
• any run image associated with a trusted builder (this requires reading the io.buildpacks.builder.metadata label and inspecting the stack key as run.images is currently not spec'd)
• buildpacksio/lifecycle

We should omit the tag portion of the reference when doing the string comparison.

Describe alternatives you've considered

Related issue: #2222.

Additional context

• This feature should be documented somewhere

[AidanDelaney]

I have made a first attempt at this in buildpacks/pack-private@230c76b (yes, I forked off github.com/buildpacks/pack, not pack-private :( )

I've not yet validated against "any run image associated with a trusted builder".

[AidanDelaney]

We can inspect every trusted builder. However, it's common for developers to trust our default set of builders and to add their own additional images. Then in a CI environment this leads to accessible internal builders and inaccessible external builders. The inaccessibility of external builders is reasonable as they're never used. However, if we now iterate over all trusted builders, we will see pack fail in these CI situations. Do we want to add an ignoreIfInaccessible flag to InspectBuilder? And, a short timeout? Even with a short timeout, this is likely to lead to noticiable extra latency in a pack build call.

builderInfo, err := inspector.InspectBuilder(trustedBuilder, false)
if err != nil {
	return fmt.Errorf("failed to retrieve trusted builder metadata")
}
for _, runner := range builderInfo.RunImages {
	runImage, err := name.ParseReference(runner.Image)
	if err != nil {
		return fmt.Errorf("run image %s for trusted builder %s is invalid", runner, builder)
	}
	if inputImage.Context().RepositoryStr() == runImage.Context().RepositoryStr() {
		return fmt.Errorf("name must not match run image name for trusted builder %s", builder)
	}
}

[natalieparellano]

@AidanDelaney you are my literal hero. Thank you 🙏🏼

[natalieparellano]

Do we want to add an ignoreIfInaccessible flag to InspectBuilder? And, a short timeout?

This makes sense to me.

Even with a short timeout, this is likely to lead to noticiable extra latency in a pack build call.

I think we should only do this check if the export target is the daemon. If the export target is the registry, then registry write permissions should protect trusted build inputs.

[AidanDelaney]

Work in buildpacks/pack-private#43 caches the run image names from a builder when we add a builder as a trusted builder. This adds a number of new failure modes to pack and adds complexity that we probably do not need.

I'm going to take this in a different direction:

1. if we're publishing to the daemon, then
   a. disallow publishing as well-known lifecycle names (we can't stop people overwriting company.com/mirror/lifecycle)
   b. disallow publishing as the current builder name
   c. disallow publishing as the current run image name
   d. disallow publishing as any trusted builder names
   - but we don't check against the run images names of trusted builders
   - document that pack in CI should be run using a "clean" daemon
2. else, we're publishing to a registry
   • document that the platform provider should take care to use builders/lifecycle that are read-only to the publishing credentials

[AidanDelaney]

I now think that buildpacks/pack-private#47 is "good enough". We perform the following list of checks. However, we do not check that someone is "overriding" a buildpack name that is required by a builder (it would be hard to create a valid poisoned buildpack using pack build). Similarly, we do not check that someone is "overriding" the run images of trusted builders (again, is it possible to create a poisoned run image using pack build?).

a. disallow publishing as well-known lifecycle names (we can't stop people overwriting company.com/mirror/lifecycle)
b. disallow publishing as the current builder name
c. disallow publishing as the current run image name
d. disallow publishing as any trusted builder names

[AidanDelaney]

Caching trusted builder run images when we add a new trusted builder does not gain us much. Trusted builders are generally referenced using mutable tags (i.e. builder:latest). Therefore, the run image names could have changed since the trusted builder was added.

In addition, pack has an open-world assumption for builders. Caching the run image names can not fix the situation where

1. pack build builder-run-image:somehow
   • Ask the user to build an image that coincides with a future trusted builder's run image name
2. pack config trusted-builder add builder
   • Add the trusted-builder
3. pack build --builder builder ...
   • Build using the poisoned run image.

We cannot forbid every possible future run image name.

[natalieparellano]

Many thanks for the updates @AidanDelaney 🙏🏼

I'll take a look soon.