-
Notifications
You must be signed in to change notification settings - Fork 377
utils: add bounds checking for Unix domain socket paths #1837
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's GuideReplaces unsafe strcpy calls when assigning to sockaddr_un.sun_path with explicit length validation, strncpy, and manual null-termination to prevent buffer overflows. Class diagram for updated Unix domain socket path handlingclassDiagram
class sockaddr_un {
char sun_path[108]
}
class libcrun_error_t
class open_unix_domain_client_socket {
+int open_unix_domain_client_socket(const char *path, int dgram, libcrun_error_t *err)
+// Now checks path length before copying
}
class open_unix_domain_socket {
+int open_unix_domain_socket(const char *path, int dgram, libcrun_error_t *err)
+// Now checks path length before copying
}
sockaddr_un <.. open_unix_domain_client_socket : uses
sockaddr_un <.. open_unix_domain_socket : uses
libcrun_error_t <.. open_unix_domain_client_socket : error reporting
libcrun_error_t <.. open_unix_domain_socket : error reporting
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
src/libcrun/utils.c
Outdated
} | ||
|
||
strcpy (addr.sun_path, path); | ||
if (strlen (path) >= sizeof (addr.sun_path)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since you are already computing the strlen(). Could we just store it in a variable and then use memcpy instead? That would be faster (and we don't have to drop the check for strncpy in make syntax-check
)
Replace unsafe strcpy() with proper length validation and memcpy() to prevent buffer overflow when copying socket paths to sockaddr_un.sun_path. This prevents potential buffer overflow vulnerabilities when handling long socket paths. Signed-off-by: Jindrich Novy <jnovy@redhat.com>
Replaced the strncpy() with memcpy() @giuseppe PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Replace unsafe strcpy() with proper length validation and strncpy() to prevent buffer overflow when copying socket paths to sockaddr_un.sun_path.
This prevents potential buffer overflow vulnerabilities when handling long socket paths.
Summary by Sourcery
Add bounds checking and safe copying for Unix domain socket paths in open_unix_domain_client_socket and open_unix_domain_socket to eliminate buffer overflow vulnerabilities.
Bug Fixes:
Enhancements: