Buffer Overflow Hardening - #1890
Closed
Abdullah-Ebryx wants to merge 0 commit into
Closed
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuidePR hardens against buffer overflows by extending chroot_realpath to accept a buffer size parameter, updating all its call sites to pass the actual buffer length, and replacing hardcoded PATH_MAX in snprintf calls with the dynamic size. Class diagram for updated chroot_realpath function signature and usageclassDiagram
class chroot_realpath {
+char *chroot_realpath(const char *chroot, const char *path, char resolved_path[], unsigned long int size_resolved_path)
}
class libcrun_container_checkpoint_linux_criu {
+calls chroot_realpath(..., buf, sizeof(buf))
}
class libcrun_container_restore_linux_criu {
+calls chroot_realpath(..., buf, sizeof(buf))
}
class safe_openat_fallback {
+calls chroot_realpath(..., buffer, sizeof(buffer))
}
libcrun_container_checkpoint_linux_criu --> chroot_realpath
libcrun_container_restore_linux_criu --> chroot_realpath
safe_openat_fallback --> chroot_realpath
Flow diagram for buffer size propagation in chroot_realpath usageflowchart TD
A[Call site: libcrun_container_checkpoint_linux_criu] -->|pass buf, sizeof(buf)| B[chroot_realpath]
C[Call site: libcrun_container_restore_linux_criu] -->|pass buf, sizeof(buf)| B
D[Call site: safe_openat_fallback] -->|pass buffer, sizeof(buffer)| B
B --> E[snprintf uses size_resolved_path]
E --> F[Buffer overflow risk reduced]
File-Level Changes
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 there - I've reviewed your changes and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/libcrun/chroot_realpath.c:50` </location>
<code_context>
#define MAX_READLINKS 32
-char *chroot_realpath(const char *chroot, const char *path, char resolved_path[])
+char *chroot_realpath(const char *chroot, const char *path, char resolved_path[],unsigned long int size_resolved_path )
{
char copy_path[PATH_MAX];
</code_context>
<issue_to_address>
**suggestion:** Consider using size_t for buffer size parameter instead of unsigned long int.
Using size_t is standard for buffer sizes in C, helps prevent type mismatches, and clarifies the parameter's purpose.
Suggested implementation:
```c
char *chroot_realpath(const char *chroot, const char *path, char resolved_path[], size_t size_resolved_path )
```
If `chroot_realpath` is declared in a header file (e.g., `chroot_realpath.h`), update the parameter type there as well:
<<<<<<< SEARCH
char *chroot_realpath(const char *chroot, const char *path, char resolved_path[], unsigned long int size_resolved_path );
=======
char *chroot_realpath(const char *chroot, const char *path, char resolved_path[], size_t size_resolved_path );
>>>>>>> REPLACE
Also, ensure that all calls to `chroot_realpath` throughout the codebase use a `size_t` value for the `size_resolved_path` argument.
</issue_to_address>
### Comment 2
<location> `src/libcrun/criu.c:643` </location>
<code_context>
return crun_make_error (err, 0, "CRIU does not support `src-nofollow` for bind mounts");
- dest_in_root = chroot_realpath (status->rootfs, def->mounts[i]->destination, buf);
+ dest_in_root = chroot_realpath (status->rootfs, def->mounts[i]->destination, buf,sizeof(buf));
if (UNLIKELY (dest_in_root == NULL))
{
</code_context>
<issue_to_address>
**issue (bug_risk):** sizeof(buf) may not always be correct if buf is not a statically sized array.
If buf is a pointer or dynamically allocated, sizeof(buf) will not give the actual buffer size. Pass the correct buffer size explicitly to avoid errors.
</issue_to_address>
### Comment 3
<location> `tests/tests_libcrun_fuzzer.c:114` </location>
<code_context>
# endif
/* Defined in chroot_realpath.c */
-char *chroot_realpath (const char *chroot, const char *path, char resolved_path[]);
+char *chroot_realpath (const char *chroot, const char *path, char resolved_path[],unsigned long int size_resolved_path);
</code_context>
<issue_to_address>
**issue (bug_risk):** Incorrect function declaration in test file.
The parameter 'sizeof()' is invalid in the function declaration and will prevent compilation. Update the declaration to match the implementation: char *chroot_realpath(const char *chroot, const char *path, char resolved_path[], unsigned long int size_resolved_path);
</issue_to_address>
### Comment 4
<location> `tests/tests_libcrun_fuzzer.c:126-128` </location>
<code_context>
if (path == NULL)
return 0;
- chroot_realpath (".", path, resolved_path);
+ chroot_realpath (".", path, resolved_path,sizeof(resolved_path));
(void) resolved_path;
return 0;
</code_context>
<issue_to_address>
**suggestion (testing):** Test does not check for buffer overflow or error conditions.
Please add assertions to verify chroot_realpath's behavior with under-allocated buffers, long paths, and error returns, including NULL and errno values.
```suggestion
// Test with a sufficiently large buffer
int ret = chroot_realpath(".", path, resolved_path, sizeof(resolved_path));
assert(ret != NULL || errno != 0);
// Test with an under-allocated buffer
char small_buf[4];
errno = 0;
ret = chroot_realpath(".", path, small_buf, sizeof(small_buf));
assert(ret == NULL);
assert(errno == ERANGE || errno != 0);
// Test with a very long path
char long_path[4096];
memset(long_path, 'a', sizeof(long_path) - 1);
long_path[sizeof(long_path) - 1] = '\0';
errno = 0;
ret = chroot_realpath(".", long_path, resolved_path, sizeof(resolved_path));
assert(ret == NULL || ret != NULL);
// If ret is NULL, errno should be set
if (ret == NULL) {
assert(errno != 0);
}
// Test with NULL path
errno = 0;
ret = chroot_realpath(".", NULL, resolved_path, sizeof(resolved_path));
assert(ret == NULL);
assert(errno != 0);
// Test with NULL resolved_path buffer
errno = 0;
ret = chroot_realpath(".", path, NULL, sizeof(resolved_path));
assert(ret == NULL);
assert(errno != 0);
return 0;
```
</issue_to_address>
### Comment 5
<location> `tests/tests_libcrun_fuzzer.c:127-111` </location>
<code_context>
- chroot_realpath (".", path, resolved_path);
+ chroot_realpath (".", path, resolved_path,sizeof(resolved_path));
(void) resolved_path;
return 0;
}
</code_context>
<issue_to_address>
**suggestion (testing):** Test does not assert correctness of resolved_path.
Please add assertions verifying resolved_path contains the expected value for valid inputs and remains unchanged or NULL for invalid inputs.
Suggested implementation:
```c
int ret = chroot_realpath (".", path, resolved_path, sizeof(resolved_path));
if (ret == 0) {
// For valid input, resolved_path should contain the expected real path.
// Replace "/expected/real/path" with the actual expected value for your test case.
assert(strcmp(resolved_path, "/expected/real/path") == 0 && "resolved_path does not match expected value");
} else {
// For invalid input, resolved_path should remain unchanged or be NULL.
// If resolved_path is expected to be NULL, check for that.
assert(resolved_path[0] == '\0' && "resolved_path should be empty for invalid input");
}
return 0;
}
```
- You will need to replace `"/expected/real/path"` with the actual expected resolved path for your test case.
- If your implementation of `chroot_realpath` sets `resolved_path` to NULL for invalid input, adjust the assertion accordingly (e.g., `assert(resolved_path == NULL)`).
- Make sure to include the necessary headers for `assert` and `strcmp` if not already present: `#include <assert.h>` and `#include <string.h>`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
giuseppe
reviewed
Oct 8, 2025
| #define MAX_READLINKS 32 | ||
|
|
||
| char *chroot_realpath(const char *chroot, const char *path, char resolved_path[]) | ||
| char *chroot_realpath(const char *chroot, const char *path, char resolved_path[],size_t size_resolved_path ) |
Member
There was a problem hiding this comment.
can we just add a documentation hint that resolved_path must be at least PATH_MAX bytes? There is no point in supporting a smaller size
Contributor
Author
There was a problem hiding this comment.
Certainly, we could take that route. However, the changes required to support the new function arguments were minimal, and this approach helps avoid future technical debt by enforcing the required buffer length while still allowing dynamic buffer allocation for other use cases. There's also the consideration that relying solely on documentation assumes the caller will implement things correctly, whereas this change enforces correctness at the API level and prevents misuse.
Collaborator
|
@Abdullah-Ebryx can you please squash the commits? |
|
Ephemeral COPR build failed. @containers/packit-build please check. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hardening against buffer overflow (BOF) vulnerabilities requires more than just limiting the scanfn function with PATH_MAX. This approach doesn't account for scenarios where the buffer is under-allocated
Summary by Sourcery
Harden chroot_realpath against potential buffer overflows by extending its API to accept an explicit buffer size, updating its implementation to use that size for bounds checking, and adjusting all callers (including tests) to pass the buffer length.
Bug Fixes:
Enhancements:
Tests: