sparc_attrs.c: Prevent buffer overflow in sparc_check_object_attribute

Message ID 20241105142509.111583-1-ant.v.moryakov@gmail.com
State Rejected
Headers
Series sparc_attrs.c: Prevent buffer overflow in sparc_check_object_attribute |

Commit Message

Anton Moryakov Nov. 5, 2024, 2:25 p.m. UTC
  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

Serhei Makarov Nov. 5, 2024, 4:58 p.m. UTC | #1
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.
  
Mark Wielaard Nov. 5, 2024, 6:37 p.m. UTC | #2
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
  

Patch

diff --git a/elfutils/backends/sparc_attrs.c b/elfutils/backends/sparc_attrs.c
index 974e8fb..0ba17c8 100644
--- a/elfutils/backends/sparc_attrs.c
+++ b/elfutils/backends/sparc_attrs.c
@@ -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;