sparc_attrs.c: Prevent buffer overflow in sparc_check_object_attribute
Commit Message
first report of the static analyzer:
A string is copied into the buffer 's' of size 577 without checking its length first at sparc_attrs.c:95.
Corrections explained:
Record Length Limit: We use strncat to add a line indicating the available remaining_size. This prevents writing beyond the allocated memory.
Remaining space update: remaining_size is updated after each entry to ensure that row additions do not cause overflow.
Found by RASU JSC.
Signed-off-by: Anton Moryakov <ant.v.moryakov@gmail.com>
---
elfutils/backends/sparc_attrs.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
Comments
On Tue, Nov 5, 2024, at 9:25 AM, Anton Moryakov wrote:
> Record Length Limit: We use strncat to add a line indicating the
> available remaining_size. This prevents writing beyond the allocated
> memory.
> Remaining space update: remaining_size is updated after each entry to
> ensure that row additions do not cause overflow.
It looks to me like the maximum possible length of the concatenated strings (from a hardcoded array a few lines prior to the patch) and the length of the buffer are both statically known, and thus it's not actually possible for the code to overflow the buffer. This is an interesting test case for developing a static analyzer, but not an actual bug.
Hi,
On Tue, Nov 05, 2024 at 11:58:19AM -0500, Serhei Makarov wrote:
> On Tue, Nov 5, 2024, at 9:25 AM, Anton Moryakov wrote:
> > Record Length Limit: We use strncat to add a line indicating the
> > available remaining_size. This prevents writing beyond the allocated
> > memory.
> > Remaining space update: remaining_size is updated after each entry to
> > ensure that row additions do not cause overflow.
>
> It looks to me like the maximum possible length of the concatenated
> strings (from a hardcoded array a few lines prior to the patch) and
> the length of the buffer are both statically known, and thus it's
> not actually possible for the code to overflow the buffer. This is
> an interesting test case for developing a static analyzer, but not
> an actual bug.
Or add a static_assert based on that knowledge as we discussed before
when this "RASU JSC" issue came up:
https://inbox.sourceware.org/elfutils-devel/20240702114611.GE29242@gnu.wildebeest.org/T
Cheers,
Mark
@@ -87,12 +87,17 @@ sparc_check_object_attribute (Ebl *ebl __attribute__ ((unused)),
}
char *s = name;
+ size_t remaining_size = sizeof(name) - 1;
for (cap = 0; cap < 32; cap++)
if (value & (1U << cap))
{
- if (*s != '\0')
- s = strcat (s, ",");
- s = strcat (s, caps[cap]);
+ if (*s != '\0'&& remaining_size > 1)
+ {
+ strncat(s, ",", remaining_size);
+ remaining_size --;
+ }
+ strncat(s, caps[cap], remaining_size);
+ remaining_size -= strlen(caps[cap]);
}
*value_name = s;