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

Threat model/security review for adding the Data field #321

Closed
SteveLasker opened this issue Apr 28, 2022 · 10 comments
Closed

Threat model/security review for adding the Data field #321

SteveLasker opened this issue Apr 28, 2022 · 10 comments

Comments

@SteveLasker
Copy link
Contributor

Has there been any considerations for a threat model or security review for adding the data field?

Today, a manifest can be safely pulled, without concern of binary content. It's one of the great designs of the registry APIs.

A security scanner can evaluate the digest of the manifest, and verify if it, or the digests of the layers|blobs have been identified as having security vulnerabilities. This applies to any manifest request, where metadata was assumed to be safely returned.

The data filed changes this assumption. A manifest may be pulled for evaluation, and it can now contain binary data. If another exploited process is waiting to run arbitrary code, a manifest pull can now include exploitable code that wasn't previously

Just suggesting we should think through this scenario before broadly adopting.

@imjasonh
Copy link
Member

imjasonh commented Apr 29, 2022

A manifest could always have included arbitrary content, whether or not data is defined as having special meaning, simply by having data in annotations, unknown fields, etc.

It would require a pretty serious bug in JSON parsing or base64 decoding or sha256 calculation to result in any kind of RCE or DoS or similar vulnerability. Not that bugs don't exist (they do), but these are in general fairly well understood, and tested, and pentested components, especially in the common case where it's in Go. If there's an RCE in encoding/json, OCI registries are among the least of my worries.

If there's a specific demonstrable concern you have in mind I'd suggest we evaluate that specifically, instead of worrying about increasingly nebulous "what if" scenarios that may never come to pass.

@SteveLasker
Copy link
Contributor Author

I'm not suggesting a JSON parsing issue. Once binary data is intentionally added to a manifest, we are adding a new threat model that was not intended before.
It's also not a registry problem, rather a client that pulls a manifest will now have binary data, that it didn't have before.

@sudo-bmitch
Copy link
Contributor

I've implemented this already in regclient. What specific scenario do you have that can reproduce a vulnerability?

@SteveLasker
Copy link
Contributor Author

From above:

A manifest may be pulled for evaluation, and it can now contain binary data. If another exploited process is waiting to run arbitrary code, a manifest pull can now include exploitable code that wasn't previously exploitable

@sudo-bmitch
Copy link
Contributor

sudo-bmitch commented Apr 29, 2022

Doesn't that same theoretical risk exist with all blob pulls today? And couldn't there already be base64 encoded data in a manifest annotation today?

@SteveLasker
Copy link
Contributor Author

Pulling a blob is expected to be binary content, and it's after a manifest is evaluated, which can also be confirmed a security scanner already cleared the digest of the layer.

Annotations are minimal values, and not used broadly. The data property, as specified, intentionally brings binary data into the manifest.

Considering the focus on security, should we not consider the security impact of changing the behavior and surface area of a lightweight manifest pull?

@sudo-bmitch
Copy link
Contributor

Many languages, Go included, don't make arbitrary data that's been loaded into memory executable anyway. The scenario where there's any risk here seems to involve:

  1. an application that processes manifests but not blobs
  2. data is extracted from base64 to binary even though it won't be used
  3. data read into memory in a way that's executable
  4. no other fields like annotations are parsed and extracted
  5. a second process that can detect and execute that data that isn't already detected as malicious code

I don't see the real world risk there, but then I'm not normally a red team attacker. My code implementing this is open source, feel free to reach out to me with any CVEs identified.

My bigger concern is this feels like an attempt to reopen a continuous debate that took over a year for us to finally resolve, so unless there's a very good reason to reopen that discussion, I'd rather us move forward.

@dlorenc
Copy link

dlorenc commented Apr 29, 2022

I have to agree with Brandon. This has been discussed ad nauseam. This issue isn't actionable without a clear, specific, threat or example.

@jdolitsky
Copy link
Member

This issue is pure political filibuster. Please stop.

@opencontainers opencontainers locked and limited conversation to collaborators Apr 29, 2022
@vbatts
Copy link
Member

vbatts commented Apr 29, 2022

Thanks for the level review @sudo-bmitch, and for closing the issue @jdolitsky

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

No branches or pull requests

6 participants