conversion: mention the potential risks of blindly copying annotations #1061
Conversation
cyphar
requested review from
jonjohnsonjr,
jonboulle,
stevvooe,
sudo-bmitch,
sajayantony,
tianon and
vbatts
as code owners
While the ability to copy arbitrary container labels into the generated runtime-spec is a very useful feature, it needs to be mentioned that some runtime-spec annotations (such as "org.systemd.property.*" and "run.oci.hooks.*") can allow an image to cause runtimes to either configure an insecure container or act as a way to attack the host machine. It should be noted this is no different to any other malicious config.json attack -- it is the responsibility of runtime-spec generators to make sure the configuration is secure. Reported-by: Akihiro Suda <[email protected]> Signed-off-by: Aleksa Sarai <[email protected]>
| A converter MAY choose to not include annotations specified in the image, if the annotations are considered to be unsafe by the converter's security policy. | ||
| If a converter does omit annotations during the conversion, it SHOULD provide feedback to the user to indicate that an annotation has not been converted. | ||
|
|
||
| **Implementor's Note:** Some implementations (such as Docker/Moby) are known to indiscriminately and silently exclude all annotations specified in the image. Implementations SHOULD NOT do this. |
There was a problem hiding this comment.
“SHOULD NOT do this” sounds controversial.
Practically, Docker/Moby, Podman, containerd, CRI-O, etc. will have to continue doing this permanently, as any runtime annotation can be unsafe (unless we officially have the denylist)
There was a problem hiding this comment.
This can be eventually a solution
There was a problem hiding this comment.
Agreed that this is controversial; honestly my favorite feature of annotations (over labels, for example) is that they are NOT inherited (either during build or during run), so this sounds like a misfeature to me 😕 don't most runtimes still keep track of which image a container was started from such that the image annotations could be queried if desired?
As a user, if any annotations were actually necessary for the correct operation of a given container, I would want that to be opt-in at the very most, especially if they cause the runtime behavior to change. 🙈
There was a problem hiding this comment.
The specification text explicitly says that this behaviour is a SHOULD (with some parts being a MUST). Any reasonable reading of the text would lead to the conclusion that dropping all labels is something one SHOULD NOT do. I didn't want to mention Docker because they don't support this part of the spec anyway, but if we have to mention them it makes very little sense to me to have a specification that reads:
Implementations SHOULD xyz.
Implementations MUST abc.
Implementations MUST NOT foo.Implementor's note: Docker doesn't do any of this (so feel free to ignore it).
If you want to remove this feature, we can have a discussion about that, but de-facto saying this part of the specification can be safely ignored seems underhanded.
And yes, I did write this in a combative manner because an implementor note saying that a "popular implementation" (though it did take 6 years for it to actually be implemented, during which time umoci has arguably been the most popular OCI image-spec implementation) has not implemented the spec doesn't seem like something that should be tacitly endorsed.
Agreed that this is controversial; honestly my favorite feature of annotations (over labels, for example) is that they are NOT inherited (either during build or during run), so this sounds like a misfeature to me
This feature was already merged into the spec a while ago. I'm not quite clear what you mean by inheritance -- this text is talking about image labels being added to the runtime-spec annotations. Descriptor annotations are very explicitly not copied into the runtime-spec annotations, this entire text is about image labels being copied into runtime-spec annotations. This allows you to set an image label like com.nvidia.gpu.cores=4 on an image which a runtime could use to make decisions or adjust resource allocation in a way the runtime-spec does not allow.
don't most runtimes still keep track of which image a container was started from such that the image annotations could be queried if desired?
See above, I think we're talking about different things.
As a user, if any annotations were actually necessary for the correct operation of a given container, I would want that to be opt-in at the very most, especially if they cause the runtime behavior to change.
Sure, but @AkihiroSuda's preference was that we remove the feature entirely (or restrict it to the point that it cannot be used for generic features not mentioned by the image-spec explicitly). I do like his suggestion for having a list of potentially unsafe annotations in runc features though.
There was a problem hiding this comment.
This file was created in 2016 (544e5ff), Docker implemented OCI media types in 2018 (moby/moby@c4f0515). I can't speak for the rationale for why this wasn't implemented in Docker's tooling (or later in containerd), but the reality is that it wasn't.
What I'm trying to say is that this is a pretty clear signal IMO that this whole feature should probably be considered optional from an implementor's perspective (given that an extremely popular implementation did not include it, and indeed even the one the spec was based on), so it feels wrong to call out Docker as being special or even unique here.
(I certainly wouldn't argue with removing the feature, but I'm also not exactly the target audience for it.)
There was a problem hiding this comment.
Docker doesn't implement any aspect of this conversion spec (despite being written specifically so they could implement it if they wanted with lots of flexibility). Even if you feel that copying labels into runtime-spec annotations is a misfeature, the annotation copying of ImageConfig fields is very critical for compatibility with runtimes (how else will a runtime know what the container's STOPSIGNAL is?) and Docker doesn't implement that either -- I don't think it's reasonable to say that we should also de-facto make that part of this spec a no-op (without removing the feature).
In my view, Docker is not compliant with this (optional) part of the image-spec. They're free to not implement it, but IMHO that means they also don't need to be mentioned because what they do in this case is not relevant.
umoci implements this behaviour as described in the spec.
There was a problem hiding this comment.
I am also not sure about this wording and require the conversion from the image annotations.
One disadvantage with this feature and the deny list approach suggested in opencontainers/runtime-spec#1202 is that we will require an additional call to the OCI runtime before we can safely feed it an OCI configuration file.
Without the conversion mechanism, we can more or less safely use the same config.json with different runtimes. Using the conversion mechanism instead, we need to be careful and filter out the unsafe annotations before we can run the container.
What do you think instead about having an explicit list of domains allowed for the conversion mechanism and expecting the runtime to not use them for anything security-sensitive?
| @@ -128,3 +128,10 @@ If there is a conflict (same key but different value) between an implicit annota | |||
|
|
|||
| A converter MAY add annotations which have keys not specified in the image. | |||
| A converter MUST NOT modify the values of annotations specified in the image. | |||
|
|
|||
| Note there is a risk that some annotations might be used by container runtimes to do operations that pose a security risk (such as running container hooks on the host or modifying security-related aspects of the container configuration). | |||
| As with any runtime specification configuration, generators SHOULD verify that the generated container configuration is safe before it is used to create a container. | |||
There was a problem hiding this comment.
Verification is practically impossible with the current engine implementations.
Probably, runtimes have to return the list of unsafe annotation prefixes (org.systemd.*, run.oci.*) in https://github.com/opencontainers/runtime-spec/blob/main/features.md
There was a problem hiding this comment.
There was a problem hiding this comment.
I agree being able to get a list of possibly dangerous annotation prefixes is a good idea (after all, using annotations to modify security properties is something the runtime-spec is currently silent on and arguably is outside of its security model), though I think we should simply say here that converters MAY make use of that information.
There was a problem hiding this comment.
I think there's a limited set of maintainers that are familiar with both the image and runtime specs, and I'm not one of them. I am curious, on line 126:
A converter SHOULD NOT attempt to extract annotations from manifests or image indices.
What does "extract" mean in that context? One way of reading it is the annotations in image and index manifests are never converted, but that conflicts with what we say elsewhere. And it's not clear why Config.Labels would not be included in that list for other possible interpretations.
| A converter MUST NOT modify the values of annotations specified in the image. | ||
|
|
||
| Note there is a risk that some annotations might be used by container runtimes to do operations that pose a security risk (such as running container hooks on the host or modifying security-related aspects of the container configuration). | ||
| As with any runtime specification configuration, generators SHOULD verify that the generated container configuration is safe before it is used to create a container. | ||
| A converter MAY choose to not include annotations specified in the image, if the annotations are considered to be unsafe by the converter's security policy. | ||
| If a converter does omit annotations during the conversion, it SHOULD provide feedback to the user to indicate that an annotation has not been converted. | ||
|
|
||
| **Implementor's Note:** Some implementations (such as Docker/Moby) are known to indiscriminately and silently exclude all annotations specified in the image. Implementations SHOULD NOT do this. |
There was a problem hiding this comment.
I'd move up the omit section to avoid confusion with the line that says "MUST NOT modify". And to avoid pointing fingers, here's an alternate wording of the implementor's note.
| A converter MUST NOT modify the values of annotations specified in the image. | |
| Note there is a risk that some annotations might be used by container runtimes to do operations that pose a security risk (such as running container hooks on the host or modifying security-related aspects of the container configuration). | |
| As with any runtime specification configuration, generators SHOULD verify that the generated container configuration is safe before it is used to create a container. | |
| A converter MAY choose to not include annotations specified in the image, if the annotations are considered to be unsafe by the converter's security policy. | |
| If a converter does omit annotations during the conversion, it SHOULD provide feedback to the user to indicate that an annotation has not been converted. | |
| **Implementor's Note:** Some implementations (such as Docker/Moby) are known to indiscriminately and silently exclude all annotations specified in the image. Implementations SHOULD NOT do this. | |
| A converter MAY omit, but MUST NOT modify, the values of annotations specified in the image. | |
| If a converter does omit annotations during the conversion, it SHOULD provide feedback to the user to indicate that an annotation has not been converted. | |
| Note there is a risk that some annotations might be used by container runtimes to do operations that pose a security risk (such as running container hooks on the host or modifying security-related aspects of the container configuration). | |
| As with any runtime specification configuration, generators SHOULD verify that the generated container configuration is safe before it is used to create a container. | |
| **Implementor's Note:** Many implementations exclude all annotations specified in the image. | |
| For portability, dependencies on annotation conversion should be avoided. |
There was a problem hiding this comment.
"extract" means to copy the annotation in the same way that Config.Labels are copied into runtime-spec annotations. The point is that we only copy labels, not descriptor (or any other) annotations.
There was a problem hiding this comment.
I'm confused how that interpretation of "extract" doesn't conflict with the next line:
If there is a conflict (same key but different value) between an implicit annotation (or annotation in manifests or image indices) and an explicitly specified annotation in
Config.Labels, the value specified inConfig.LabelsMUST take precedence.
One line is saying "don't use it" while the next says "here's the precedence".
There was a problem hiding this comment.
The first requirement is that they SHOULD NOT copy said annotations, but if they decide to copy those other annotations, labels MUST take precedence. (I forgot I made this a SHOULD NOT rather than a MUST NOT. The logic is that a runtime can set any other custom annotations as long as the Config.Label and other Config annotations take precedence, so it doesn't make sense to say you MUST NOT copy these because they have the right to set them anyway if they treat them as "defaults".)
|
I don't know the history and I'm sure it's complicated. But in the long term should we move the convertion.md next to https://github.com/opencontainers/runtime-spec/blob/v1.0.0/bundle.md ? From a location standpoint it does look like the image spec is in between worlds. Moving the conversion to a target format (in this case a runtime bundle) to the runtime spec, might reflect the similar thinking that targets or clients who consume the image spec define their behavior within their scope. For this specific PR, I would just lean on folks who have more context on runtime. |
While the ability to copy arbitrary container labels into the generated runtime-spec is a very useful feature, it needs to be mentioned that some runtime-spec annotations (such as
org.systemd.property.*andrun.oci.hooks.*) can allow an image to cause runtimes to either configure an insecure container or act as a way to attack the host machine.It should be noted this is no different to any other malicious config.json attack -- it is the responsibility of runtime-spec generators to make sure the configuration is secure.
Reported-by: Akihiro Suda [email protected]
Signed-off-by: Aleksa Sarai [email protected]
/cc @AkihiroSuda