features: add potentiallyUnsafeConfigAnnotations#1205
features: add potentiallyUnsafeConfigAnnotations#1205cyphar merged 1 commit intoopencontainers:mainfrom
Conversation
d95f75e to
fc01739
Compare
fc01739 to
67e8954
Compare
features.md
Outdated
| **`unsafeAnnotationPrefixes`** (array of strings, OPTIONAL) contains prefixes of the [`annotations` property of `config.json`](config.md#annotations) | ||
| that may change the behavior of the runtime. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- Even something like
STOPSIGNAL(org.opencontainers.image.stopSignal) would be considered "unsafe", making converting parts of the image-spec configuration (separate to the wholeConfig.Labelsdiscussion) annotations from image-spec seem a little pointless. - 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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Updated
Unsafe annotations in
config.json
unsafeAnnotationsInConfig(array of strings, OPTIONAL) contains values ofannotationsproperty ofconfig.jsonthat 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 matchcom.example.foo.bar.baz.
67e8954 to
3790810
Compare
|
@cyphar PTAL |
features.md
Outdated
| ## <a name="featuresUnsafeAnnotationsInConfig" />Unsafe annotations in `config.json` | ||
|
|
||
| **`unsafeAnnotationsInConfig`** (array of strings, OPTIONAL) contains values of [`annotations` property of `config.json`](config.md#annotations) | ||
| that may change the behavior of the runtime. |
There was a problem hiding this comment.
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.
|
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 A small nit, but maybe we should rename the annotation to |
Fix issue 1202 Signed-off-by: Akihiro Suda <[email protected]>
3790810 to
5e98fec
Compare
|
Renamed to |
|
@cyphar LGTY? |
|
cc @neersighted |
Fix #1202 , for prohibiting propagation of insecure annotations from remote images into OCI runtime
config.jsonhttps://github.com/opencontainers/image-spec/pull/1061/files#r1194531089