status: restrict valid container ID names#2077
status: restrict valid container ID names#2077giuseppe wants to merge 2 commits intocontainers:mainfrom
Conversation
giuseppe
force-pushed
the
validate-container-name
branch
from
2d94b30 to
0d18d24
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves container ID and cgroup path validation by rejecting '..' components and enforcing an alphanumeric naming scheme. It also adds relevant test cases. Review feedback suggests replacing isalnum with explicit ASCII checks to avoid locale-dependency and adding a NULL check to the path utility for better robustness.
Allow only IDs that match [a-zA-Z0-9][a-zA-Z0-9_.-]*, following the same naming convention used by Podman and Docker. IDs starting with a dot are not allowed since the "." prefix is reserved for internal use. Closes: containers#2074 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
The run.oci.delegate-cgroup annotation value is used as a path fragment appended to the container cgroup path. Even if it is an unsafe annotation, reject values containing ".." components as a safety measure and prevent silly mistakes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
giuseppe
force-pushed
the
validate-container-name
branch
from
0d18d24 to
083ffde
Compare
| return true; | ||
| if (first) | ||
| return false; | ||
| return c == '_' || c == '.' || c == '-'; |
There was a problem hiding this comment.
Podman use this regex: ^[a-zA-Z0-9][a-zA-Z0-9_.-]*$.
Docker use this regex: ^[a-zA-Z0-9][a-zA-Z0-9_.-]+$. The difference is, one-letter names are not allowed (I checked that).
Both Docker and Podman are slightly more restrictve than runc:
- runc allows all this and also
'+'(since ~2016, Allow + in container ID opencontainers/runc#675); not sure why. - runc allows
_.-as the first character (but disallows names like '.' and '..').
NOTE Docker/Podman container names are not the same as crun/runc container IDs. Both Docker/containerd and Podman (I guess k8s, too) generate a random 64 character hexadecimal ID (like 0dda2d02664f88b49fae96705383da6da90149a49476ff6b0ee74b2ae3936c2f) which is used by crun/runc.
I'm not quite sure but maybe it makes sense to stay compatible with runc here (i.e. be less restrictive than docker/podman). The runc validation logic is here: https://github.com/opencontainers/runc/blob/main/libcontainer/factory_linux.go#L192
There was a problem hiding this comment.
The runc convention makes the validation a bit more difficult as we need to treat the string as a path but doable. In any case it is a best effort measure, I will change the implementation to match runc
| conf['annotations'] = {} | ||
| conf['annotations']['run.oci.systemd.subgroup'] = 'mysubgroup' | ||
|
|
||
| for invalid in ["../../../victim", "../escape", "a/../b"]: |
There was a problem hiding this comment.
nit: I would add ".." to the list.
2 participants
Allow only IDs that match [a-zA-Z0-9][a-zA-Z0-9_.-]*, following the same naming convention used by Podman and Docker.
IDs starting with a dot are not allowed since the "." prefix is reserved for internal use.
Also reject ".." in delegate-cgroup annotation to prevent misuses.
Closes: #2074