Skip to content

cgroup: improve the handling of subcgroups and avoid path traversal#2117

Merged
giuseppe merged 1 commit into
containers:mainfrom
leonardomoreira00:feature/improve-cground-handling
Jul 2, 2026
Merged

cgroup: improve the handling of subcgroups and avoid path traversal#2117
giuseppe merged 1 commit into
containers:mainfrom
leonardomoreira00:feature/improve-cground-handling

Conversation

@leonardomoreira00

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors cgroup path resolution and process movement in libcrun to prevent directory traversal (such as using absolute paths or .. components) by utilizing openat2 with RESOLVE_BENEATH flags, falling back to a manual ancestor-tracking path resolution if unsupported. It also adds several tests to verify these restrictions. The review feedback points out that the new test test_resources_exec_cgroup_reject_dotdot should be skipped when systemd is the cgroup manager, as it relies on cgroupfs-style paths.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread tests/test_resources.py
@packit-as-a-service

Copy link
Copy Markdown

TMT tests failed. @containers/packit-build please check.

@leonardomoreira00 leonardomoreira00 force-pushed the feature/improve-cground-handling branch from b6bf71a to df65924 Compare July 1, 2026 14:23
@giuseppe

giuseppe commented Jul 1, 2026

Copy link
Copy Markdown
Member

I think this is overkill. The cgroup file system does not allow symlinks, so we don't need to worry about using openat2. This is not a security issue because the --cgroup path is passed from a trusted source, it is only to avoid silly mistakes.

Let's just count the depth of the path

@leonardomoreira00 leonardomoreira00 force-pushed the feature/improve-cground-handling branch from df65924 to 7d461ca Compare July 1, 2026 14:37
@leonardomoreira00

leonardomoreira00 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

I think this is overkill. The cgroup file system does not allow symlinks, so we don't need to worry about using openat2. This is not a security issue because the --cgroup path is passed from a trusted source, it is only to avoid silly mistakes.

Let's just count the depth of the path

Hi @giuseppe,

I understand it is not a security issue and could be solved in many different ways.
I also thought a lot before doing it, that's why it is a PR Draft, but unfortunately it was not possible to have a 5 minutes clarification with you, to get your opinion, otherwise I would have done.

The solution is basically getting the fd and calling the openat2 using the subgroup path.
And the fallback is the solution that you mentioned, it is just looking the path and counting the ancestors (depth).

I can remove the openat2 part if you wish, I did it because I found it simple and I want to practice the standard libraries. But as you wish, if you don't want to give me the opportunity to rethink your first opinion, I can remove it and use the fallback as the main solution.

@giuseppe

giuseppe commented Jul 1, 2026

Copy link
Copy Markdown
Member

I understand it is not a security issue and could be solved in many different ways. I also thought a lot before doing it, but unfortunately it was not possible to have a 5 minutes clarification with you, to get your opinion, otherwise I would have done.

when in doubts, it is better to ask, I'd have answered that question. In general, it is better to share your plan before starting working on something so big.

The solution is basically getting the fd and calling the openat2 using the subgroup path. And the fallback is the solution that you mentioned, it is just looking the path and counting the ancestors (depth).

I can remove the openat2 part if you wish, I did it because I found it simple and I want to practice the standard libraries. But as you wish, if you don't want to give me the opportunity to rethink your first opinion, I can remove it and use the fallback as the main solution.

I am worried that there is a lot of new code for something that is not a security issue, but just a hint to the caller, and in general it is not possible to have symlinks attacks on cgroupfs since symlinks are not allowed there. In this case, you can just parse the path and count how many .. there are.

Even simpler, we can just reject paths that have .., in this case you can use directly path_has_dot_dot_component.

@leonardomoreira00

leonardomoreira00 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

I understand it is not a security issue and could be solved in many different ways. I also thought a lot before doing it, but unfortunately it was not possible to have a 5 minutes clarification with you, to get your opinion, otherwise I would have done.

when in doubts, it is better to ask, I'd have answered that question. In general, it is better to share your plan before starting working on something so big.

The solution is basically getting the fd and calling the openat2 using the subgroup path. And the fallback is the solution that you mentioned, it is just looking the path and counting the ancestors (depth).
I can remove the openat2 part if you wish, I did it because I found it simple and I want to practice the standard libraries. But as you wish, if you don't want to give me the opportunity to rethink your first opinion, I can remove it and use the fallback as the main solution.

I am worried that there is a lot of new code for something that is not a security issue, but just a hint to the caller, and in general it is not possible to have symlinks attacks on cgroupfs since symlinks are not allowed there. In this case, you can just parse the path and count how many .. there are.

Even simpler, we can just reject paths that have .., in this case you can use directly path_has_dot_dot_component.

Yes. I understand completely your point.

Actually, I started from simply rejecting '/', and '..' to ending in this openat2 solution. Mainly because it takes the responsibility and I don't have to look the content of the path myself.
But then I thought: "Okay, maybe I'm being too much strict here, I will reject all paths having '..'.."

I'm finding it difficult to contribute... it is like when you start a new project or new job.
You don't know the maintainer 🗡️ and everybody has different expectations 👯‍♀️
You don't necessarily want to impress, but you don't want to sound dumb.

In any case, thanks for sharing how you think, if I find something else to contribute, I will ask.

@leonardomoreira00 leonardomoreira00 force-pushed the feature/improve-cground-handling branch 2 times, most recently from 3b9cb25 to 3e48bf8 Compare July 1, 2026 19:37
@leonardomoreira00 leonardomoreira00 marked this pull request as ready for review July 1, 2026 19:38
@leonardomoreira00

leonardomoreira00 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

V2 Test Results:

ok 7 - resources-unified-exec-cgroup # 2.055s
ok 8 - resources-unified-exec-cgroup-with-initial-cpu-affinity # 2.057s
# crun command failed: /home/lm00/src/crun/crun --root /home/lm00/src/crun/.testsuite-run-6761/root exec --cgroup=../outside test-tmp9anwgj34 /init true
# Return code: 1
# crun command failed: /home/lm00/src/crun/crun --root /home/lm00/src/crun/.testsuite-run-6761/root exec --cgroup=./../outside test-tmp9anwgj34 /init true
# Return code: 1
# crun command failed: /home/lm00/src/crun/crun --root /home/lm00/src/crun/.testsuite-run-6761/root exec --cgroup=foo/../../outside test-tmp9anwgj34 /init true
# Return code: 1
# crun command failed: /home/lm00/src/crun/crun --root /home/lm00/src/crun/.testsuite-run-6761/root exec --cgroup=foo/bar/../../../outside test-tmp9anwgj34 /init true
# Return code: 1
ok 9 - resources-unified-exec-cgroup-reject-dotdot # 2.051s
# crun command failed: /home/lm00/src/crun/crun --root /home/lm00/src/crun/.testsuite-run-6761/root exec --cgroup=/foo test-tmpa9b815ve /init true
# Return code: 1
ok 10 - resources-unified-exec-cgroup-reject-absolute # 2.065s

V1 Test Results:

[lab@cgroup-v1-lab crun]$ sudo /opt/python-3.9/bin/python3.9 tests/test_cgroup_setup.py

# crun command failed: /home/lab/crun/crun --root /home/lab/crun/.testsuite-run-6063/root exec --cgroup=/outside test-tmpp92m741i /init true
# Return code: 1
# crun command failed: /home/lab/crun/crun --root /home/lab/crun/.testsuite-run-6063/root exec --cgroup=../outside test-tmpp92m741i /init true
# Return code: 1
# crun command failed: /home/lab/crun/crun --root /home/lab/crun/.testsuite-run-6063/root exec --cgroup=./../outside test-tmpp92m741i /init true
# Return code: 1
# crun command failed: /home/lab/crun/crun --root /home/lab/crun/.testsuite-run-6063/root exec --cgroup=foo/../../outside test-tmpp92m741i /init true
# Return code: 1
# crun command failed: /home/lab/crun/crun --root /home/lab/crun/.testsuite-run-6063/root exec --cgroup=foo/bar/../../../outside test-tmpp92m741i /init true
# Return code: 1
ok 35 - cgroup-v1-exec-cgroup-subpath # 0.135s

V1 Environment:

[lab@cgroup-v1-lab crun]$ cat /etc/os-release 
NAME="CentOS Linux"
VERSION="7 (Core)"
ID="centos"
ID_LIKE="rhel fedora"
VERSION_ID="7"
PRETTY_NAME="CentOS Linux 7 (Core)"
ANSI_COLOR="0;31"
CPE_NAME="cpe:/o:centos:centos:7"
HOME_URL="https://www.centos.org/"
BUG_REPORT_URL="https://bugs.centos.org/"

CENTOS_MANTISBT_PROJECT="CentOS-7"
CENTOS_MANTISBT_PROJECT_VERSION="7"
REDHAT_SUPPORT_PRODUCT="centos"
REDHAT_SUPPORT_PRODUCT_VERSION="7"

V2 Environment:

lm00@lm00:~/src/crun$ cat /etc/os-release +
NAME="Fedora Linux"
VERSION="43 (Workstation Edition)"
RELEASE_TYPE=stable
ID=fedora
VERSION_ID=43
VERSION_CODENAME=""
PRETTY_NAME="Fedora Linux 43 (Workstation Edition)"
ANSI_COLOR="0;38;2;60;110;180"
LOGO=fedora-logo-icon
CPE_NAME="cpe:/o:fedoraproject:fedora:43"
DEFAULT_HOSTNAME="fedora"
HOME_URL="https://fedoraproject.org/"
DOCUMENTATION_URL="https://docs.fedoraproject.org/en-US/fedora/f43/"
SUPPORT_URL="https://ask.fedoraproject.org/"
BUG_REPORT_URL="https://bugzilla.redhat.com/"
REDHAT_BUGZILLA_PRODUCT="Fedora"
REDHAT_BUGZILLA_PRODUCT_VERSION=43
REDHAT_SUPPORT_PRODUCT="Fedora"
REDHAT_SUPPORT_PRODUCT_VERSION=43
SUPPORT_END=2026-12-02
VARIANT="Workstation Edition"
VARIANT_ID=workstation

Comment thread src/libcrun/linux.c Outdated
@leonardomoreira00 leonardomoreira00 force-pushed the feature/improve-cground-handling branch from 3e48bf8 to d31c25c Compare July 2, 2026 07:56
Comment thread src/libcrun/linux.c Outdated
Comment thread src/libcrun/linux.c Outdated
@leonardomoreira00 leonardomoreira00 force-pushed the feature/improve-cground-handling branch from d31c25c to 639f7c6 Compare July 2, 2026 08:01
Signed-off-by: Leonardo Moreira <leonardo.moreira.coutinho@gmail.com>
@leonardomoreira00 leonardomoreira00 force-pushed the feature/improve-cground-handling branch from 639f7c6 to 37e0100 Compare July 2, 2026 08:05
Comment thread src/libcrun/linux.c Outdated

@giuseppe giuseppe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@giuseppe giuseppe merged commit 52b0e4a into containers:main Jul 2, 2026
46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cgroups: crun exec --cgroup` should confine sub-cgroup paths below the container control group

2 participants