features: add potentiallyUnsafeConfigAnnotations

[AkihiroSuda]

Fix #1202 , for prohibiting propagation of insecure annotations from remote images into OCI runtime config.json

https://github.com/opencontainers/image-spec/pull/1061/files#r1194531089

[cyphar]

I think a description of how the prefix should be interpreted needs to be put here. My personal opinion is we should have this be called unsafeAnnotations and have the ability to specify the full annotation name (a.b.c where only a.b.c is considered unsafe) as well as a namespace (x.y. where anything under x.y such as x.y.a and x.y.b but not x.y itself is considered unsafe).

Also this needs to be on one line to match the spec style.

[cyphar]

Also this definition is far too vague -- is the issue with these annotations that they could be used to bypass container protections (the original discussion we had about this issue on the security list), or that they are being parsed by the runtime at all?

The text implies the latter but that would mean:

1. Even something like STOPSIGNAL (org.opencontainers.image.stopSignal) would be considered "unsafe", making converting parts of the image-spec configuration (separate to the whole Config.Labels discussion) annotations from image-spec seem a little pointless.
2. The purpose of using annotations to affect container configuration would be considered "unsafe" entirely, which might be true for some things (such as annotations that modify hook or systemd configurations) but is not true for others (a hypothetical generic NVIDIA GPU configuration, or STOPSIGNAL, for instance).

However, the example appears to imply that this is just meant for actually unsafe annotations.

Maybe we should have two lists, one for annotations that the runtime parses at all (what the current text implies) and another for ones it considers unsafe (which we could define to mean that the runtime believes an attacker being able to control the value would be able to cause significant harm to the system). I think both lists would be useful for different reasons (the former is what annotation-based features are supported, the latter is which annotations are considered unsafe by the runtime).

[AkihiroSuda]

My personal opinion is we should have this be called unsafeAnnotations and have the ability to specify the full annotation name (a.b.c where only a.b.c is considered unsafe)

"a.b.c" is already valid input as an entry of unsafeAnnotationPrefixes.
A drawback is that you can't mark "a.b.c.d" safe, but probably it is not a huge deal in practice.

org.opencontainers.image.stopSignal

This annotation is consumed by Engines, not by Runtimes.

Maybe we should have two lists, one for annotations that the runtime parses at all (what the current text implies) and another for ones it considers unsafe (which we could define to mean that the runtime believes an attacker being able to control the value would be able to cause significant harm to the system).

Seems a breaking change. (Misinterpreted; thought you were suggesting to split []annotations in config.json)

Generating the precise list of the annotations might be sometimes difficult, e.g., for systemd annotations

[cyphar]

This annotation is consumed by Engines, not by Runtimes.

This is a distinction that doesn't exist in the runtime-spec (the word "engine" doesn't appear anywhere in the runtime-spec) -- if someone proposed that runc kill used the value of org.opencontainers.image.stopSignal by default, I wouldn't have an issue in principle with that idea (other than backwards-compatibility worries).

(To be honest, I'm not sure anyone actually does parse org.opencontainers.image.stopSignal today -- engines have their own copy of the image-spec configuration and thus they read the original configuration directly.)

Generating the precise list of the annotations might be sometimes difficult, e.g., for systemd annotations

If they can't be precise, a runtime can choose to be more conservative and output a blanket ban for all annotations related to systemd. The reason I wanted to have two lists is because it will let users choose how strict they want to be -- if they are running untrusted code in a very critical environment, they will probably want to reject any annotations that runc will parse at all -- while most users will just trust that the runtime has a good idea of what annotations are unsafe.

[AkihiroSuda]

Updated

Unsafe annotations in config.json

unsafeAnnotationsInConfig (array of strings, OPTIONAL) contains values of annotations property of config.json that may change the behavior of the runtime.

A value that ends with "." is interpreted as a prefix of annotations.

Example

"unsafeAnnotationsInConfig": [
  "com.example.foo.bar",
  "org.systemd.property."
]

The example above matches com.example.foo.bar, org.systemd.property.ExecStartPre, etc.
The example does not match com.example.foo.bar.baz.

[AkihiroSuda]

@cyphar PTAL

[cyphar]

I still feel that this text should be more specific as to what "may change the behaviour of the runtime" means, but that's a minor nit.

[AkihiroSuda]

Any suggestions?

[AkihiroSuda]

@utam0k @cyphar @giuseppe Can we merge this?

[giuseppe]

@utam0k @cyphar @giuseppe Can we merge this?

I am fine with merging it, though @cyphar had some comments. @cyphar are you fine to merge as it is?

[utam0k]

@utam0k @cyphar @giuseppe Can we merge this?

As for me, it is fine.

[cyphar]

I'm still not sure I agree that labelling all annotations that "affect runtime behaviour" as "unsafe" is the desirable behaviour (IMHO it should be based on whether it could be used to escalate privileges -- but to be fair, this might be hard to figure out in some cases), but I can see @AkihiroSuda's point (and ultimately the stopSignal example is theoretical since no runtime implements that behaviour).

A small nit, but maybe we should rename the annotation to potentiallyUnsafeConfigAnnotations or parseableConfigAnnotations (if there is a less ugly sounding version of this I would prefer that) since the annotations are not necessarily unsafe, they just have the potential to be unsafe.

[AkihiroSuda]

Renamed to potentiallyUnsafeConfigAnnotations as suggested

[AkihiroSuda]

@cyphar LGTY?

[AkihiroSuda]

cc @neersighted

[cyphar]

LGTM, thanks @AkihiroSuda 😸