Unsafe use of strncpy() throughout OpenAMP

[tarmasr]

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.

[arnopo]

Hello @tarmasr

you are right for Rpmsg we should at least have similar protection as the one added in Linux:
https://elixir.bootlin.com/linux/latest/source/drivers/rpmsg/rpmsg_ns.c#L51

Could you send a PR to fix the library?

[tarmasr]

Thanks @arnopo

There is a pull request here: #577

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?

[github-actions]

This issue has been marked as a stale issue because it has been open (more than) 45 days with no activity.