opencontainers / distribution-spec

OCI Distribution Specification
https://opencontainers.org
Apache License 2.0
828 stars 205 forks source link

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

Closed SteveLasker closed 2 years ago

SteveLasker commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

SteveLasker commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

This issue is pure political filibuster. Please stop.

vbatts commented 2 years ago

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