linux: never chown devices #1847
Conversation
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Reviewer's GuideThis PR implements a security fix to prevent chown on /dev/null file descriptors by capturing its device identity and skipping ownership changes, and extends the test suite with new dev-null behavior tests, enhances test utilities for stdin redirection, and strengthens mount and network device tests for robust error handling. Sequence diagram for skipping chown on /dev/nullsequenceDiagram
participant Container as container_init_setup/exec_process_entrypoint/libcrun_container_restore
participant Linux as libcrun_reopen_dev_null/get_dev_null_stat
participant Chown as maybe_chown_std_streams
participant DevNull as /dev/null
Container->>Linux: get_dev_null_stat(&dev_null_stat, err)
Linux->>DevNull: open /dev/null
Linux->>DevNull: fstat(fd, &dev_null_stat)
Linux-->>Container: dev_null_stat
Container->>Chown: maybe_chown_std_streams(uid, gid, &dev_null_stat, err)
loop for each std fd (0,1,2)
Chown->>fd: fstat(i, &statbuf)
alt statbuf.rdev == dev_null_stat.rdev
Chown-->>Chown: skip chown
else
Chown->>fd: fchown(i, uid, gid)
end
end
Class diagram for updated /dev/null handling functionsclassDiagram
class libcrun_error_t
class stat
class linux {
+int libcrun_reopen_dev_null(struct stat *dev_null_stat, libcrun_error_t *err)
+int get_dev_null_stat(struct stat *dev_null_stat, libcrun_error_t *err)
+int maybe_chown_std_streams(uid_t container_uid, gid_t container_gid, const struct stat *dev_null_stat, libcrun_error_t *err)
}
linux ..> stat : uses
linux ..> libcrun_error_t : uses
class container {
+int container_init_setup(...)
+int exec_process_entrypoint(...)
+int libcrun_container_restore(...)
}
container ..> linux : calls
note for linux "New logic: skip chown if fd is /dev/null by comparing stat.rdev"
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @giuseppe - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/libcrun/container.c:1057` </location>
<code_context>
+ continue;
+ return crun_make_error (err, errno, "fstat fd `%d`", i);
+ }
+ if (statbuf.st_rdev == dev_null_stat->st_rdev)
+ continue;
+ }
</code_context>
<issue_to_address>
Comparing only st_rdev may not be sufficient for device identity.
Also compare st_dev to ensure accurate device identification and avoid false positives.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if (statbuf.st_rdev == dev_null_stat->st_rdev)
continue;
=======
if (statbuf.st_rdev == dev_null_stat->st_rdev &&
statbuf.st_dev == dev_null_stat->st_dev)
continue;
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/libcrun/container.c
Outdated
| if (statbuf.st_rdev == dev_null_stat->st_rdev) | ||
| continue; |
There was a problem hiding this comment.
suggestion (bug_risk): Comparing only st_rdev may not be sufficient for device identity.
Also compare st_dev to ensure accurate device identification and avoid false positives.
| if (statbuf.st_rdev == dev_null_stat->st_rdev) | |
| continue; | |
| if (statbuf.st_rdev == dev_null_stat->st_rdev && | |
| statbuf.st_dev == dev_null_stat->st_dev) | |
| continue; |
| if result.returncode != 0: | ||
| sys.stderr.write("# ip link add failed: %s\n" % result.stderr) | ||
| return -1 |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if specify_broadcast: | ||
| subprocess.run(["ip", "addr", "add", "10.1.2.3/24", "brd", "10.1.2.254", "dev", "testdevice"]) | ||
| result = subprocess.run(["ip", "addr", "add", "10.1.2.3/24", "brd", "10.1.2.254", "dev", "testdevice"], capture_output=True, text=True) | ||
| if result.returncode != 0: | ||
| sys.stderr.write("# ip addr add with broadcast failed: %s\n" % result.stderr) | ||
| return -1 | ||
| else: | ||
| subprocess.run(["ip", "addr", "add", "10.1.2.3/24", "dev", "testdevice"]) | ||
| result = subprocess.run(["ip", "addr", "add", "10.1.2.3/24", "dev", "testdevice"], capture_output=True, text=True) | ||
| if result.returncode != 0: | ||
| sys.stderr.write("# ip addr add without broadcast failed: %s\n" % result.stderr) | ||
| return -1 |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if result.returncode != 0: | ||
| sys.stderr.write("# ip addr add with broadcast failed: %s\n" % result.stderr) | ||
| return -1 |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if result.returncode != 0: | ||
| sys.stderr.write("# ip addr add without broadcast failed: %s\n" % result.stderr) | ||
| return -1 |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| for userns in [True, False]: | ||
| a = helper_mount("strictatime", is_file=True, userns=userns)[0] | ||
| if "relatime" not in a: | ||
| if a is None or "relatime" not in a: | ||
| return 0 | ||
| a = helper_mount("strictatime", tmpfs=False, userns=userns)[0] | ||
| if "relatime" not in a: | ||
| if a is None or "relatime" not in a: | ||
| return 0 | ||
| a = helper_mount("strictatime", userns=userns)[0] | ||
| if "relatime" not in a: | ||
| if a is None or "relatime" not in a: | ||
| return 0 |
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if a is None or "relatime" not in a: | ||
| return 0 |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if a is None or "relatime" not in a: | ||
| return 0 |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| if a is None or "relatime" not in a: | ||
| return 0 |
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| for userns in [True, False]: | ||
| a = helper_mount("exec", is_file=True, userns=userns)[0] | ||
| if "noexec" in a: | ||
| if a is not None and "noexec" in a: | ||
| return -1 | ||
| a = helper_mount("exec", tmpfs=False, userns=userns)[0] | ||
| if "noexec" in a: | ||
| if a is not None and "noexec" in a: | ||
| return -1 | ||
| a = helper_mount("exec", userns=userns)[0] | ||
| if "noexec" in a: | ||
| if a is not None and "noexec" in a: | ||
| return -1 |
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
giuseppe
force-pushed
the
do-not-chown-dev-null
branch
from
75f03ff to
856d158
Compare
|
TMT tests failed. @containers/packit-build please check. |
giuseppe
force-pushed
the
do-not-chown-dev-null
branch
5 times, most recently
from
f0bbbc0 to
ff5bd53
Compare
Closes: containers#1845 Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
giuseppe
force-pushed
the
do-not-chown-dev-null
branch
from
ff5bd53 to
a9d1299
Compare
|
@sourcery-ai summary |
|
@kolyshkin @flouthoc PTAL |
2 participants
Closes: #1845
Summary by Sourcery
Prevent crun from changing ownership of devices within containers by detecting and skipping it during chown of standard streams, update related libcrun APIs accordingly, and add tests and test enhancements to validate and reinforce this behavior.
New Features:
Enhancements:
Tests:
Summary by Sourcery
Prevent crun from changing ownership of device file descriptors in containers by skipping char/block devices during standard‐stream chown, and reinforce this behavior with expanded test coverage and utility improvements.
New Features:
Enhancements:
Tests:
Chores: