You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
As part of a code review of a project that uses OpenAMP, I came across multiple instances of unsafe strncpy() use in the OpenAMP code base.
The specific issue is that the strncpy() function does NOT 0-terminate the destination buffer if the length of the input string is >= size of the destination buffer. This may be contrary to expectations, but it is long-standing and documented behaviour (see for example https://pubs.opengroup.org/onlinepubs/9699919799/functions/strncpy.html)
As a result, code that expects the destination buffer to be 0-terminated after a call to strncpy() -- and that is most code, because the destination buffer is typically used as a regular 0-terminated string elsewhere -- might read beyond the end of the buffer.
Safe use of strncpy() follows each call by an explicit 0-termination, like so:
In the open-amp code base, strncpy() is used in 5 places as far as I can tell; none uses the safe idiom, so all are potentially vulnerable to buffer over-reads. I found strncpy() in the following locations in the open-amp sources:
apps/system/linux/machine/generic/platform_info.c (lines 146 and 165, functions sk_unix_client() and sk_unix_server())
lib/include/openamp/remoteproc.h (line 584, function remoteproc_init_mem())
lib/rpmsg/rpmsg.c (lines 144 and 278, functions rpmsg_send_ns_message() and rpmsg_register_endpoint())
A quick review of the buffers involved shows that each of them is used in at least one location as a (presumed) 0-terminated string (typically through a %s parameter in printf() or similar), so these are all latent problems. I would not classify them as security vulnerabilities, but in my opinion these cases should be fixed even if not exploitable.
The text was updated successfully, but these errors were encountered:
This PR adds 0-termination of the destination buffer after each call of strncpy() in the open-amp project. In one place optional {...} braces were added to an else branch because its if branch now needs braces.
I cannot process the PR any further because it needs to be signed off by an approved maintainer.
Is this what you need for review, or do I need to do anything else?
As part of a code review of a project that uses OpenAMP, I came across multiple instances of unsafe strncpy() use in the OpenAMP code base.
The specific issue is that the strncpy() function does NOT 0-terminate the destination buffer if the length of the input string is >= size of the destination buffer. This may be contrary to expectations, but it is long-standing and documented behaviour (see for example https://pubs.opengroup.org/onlinepubs/9699919799/functions/strncpy.html)
As a result, code that expects the destination buffer to be 0-terminated after a call to strncpy() -- and that is most code, because the destination buffer is typically used as a regular 0-terminated string elsewhere -- might read beyond the end of the buffer.
Safe use of strncpy() follows each call by an explicit 0-termination, like so:
char buffer[...];
strncpy(buffer, source, sizeof(buffer));
buffer[sizeof(buffer)-1] = '\0';
In the open-amp code base, strncpy() is used in 5 places as far as I can tell; none uses the safe idiom, so all are potentially vulnerable to buffer over-reads. I found strncpy() in the following locations in the open-amp sources:
apps/system/linux/machine/generic/platform_info.c (lines 146 and 165, functions sk_unix_client() and sk_unix_server())
lib/include/openamp/remoteproc.h (line 584, function remoteproc_init_mem())
lib/rpmsg/rpmsg.c (lines 144 and 278, functions rpmsg_send_ns_message() and rpmsg_register_endpoint())
A quick review of the buffers involved shows that each of them is used in at least one location as a (presumed) 0-terminated string (typically through a %s parameter in printf() or similar), so these are all latent problems. I would not classify them as security vulnerabilities, but in my opinion these cases should be fixed even if not exploitable.
The text was updated successfully, but these errors were encountered: