diff mbox series

Commit: Fix CVE-2026-19582

Message ID 87jyp49gon.fsf@redhat.com
State New
Headers
Series Commit: Fix CVE-2026-19582 |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 fail Patch failed to apply
linaro-tcwg-bot/tcwg_binutils_check--master-arm fail Patch failed to apply

Commit Message

Nick Clifton Sept. 2, 2026, 8:54 a.m. UTC 
  Hi Guys,

  I am applying the attached patch to fix CVE-2026-19582.

  This is an annoying, not really valid as a CVE, bug which is based
  upon the fact that peXXigen.c:rsrc_sort_entries() uses a fixed size
  static buffer for constructing error messages.  A specially crafted
  (and invalid) PE format input file could trigger a buffer overflow
  potentially causing the linker to crash.

  The fix I have chosen is to change the buffer into a rsrc_string
  structure and to use this to keep track of the space remaining whilst
  constructing the error message.  Which is a lot of faff for a case
  that will almost certainly never arise.  But hey ho, the bug has
  been reported and now it is fixed.

Cheers
  Nick

https://bugzilla.redhat.com/show_bug.cgi?id=2513754

Comments

Andreas Schwab Sept. 2, 2026, 9:20 a.m. UTC | #1 
On Sep 02 2026, Nick Clifton wrote:

>   The fix I have chosen is to change the buffer into a rsrc_string
>   structure and to use this to keep track of the space remaining whilst
>   constructing the error message.  Which is a lot of faff for a case
>   that will almost certainly never arise.  But hey ho, the bug has
>   been reported and now it is fixed.

Wouldn't it be easier to use bfd_asprintf?
Nick Clifton Sept. 2, 2026, 10:36 a.m. UTC | #2 
Hi Andreas,

>>    The fix I have chosen is to change the buffer into a rsrc_string
>>    structure and to use this to keep track of the space remaining whilst
>>    constructing the error message.  Which is a lot of faff for a case
>>    that will almost certainly never arise.  But hey ho, the bug has
>>    been reported and now it is fixed.
> 
> Wouldn't it be easier to use bfd_asprintf?

In hindsight - maybe ?  The problem with bfd_asprintf() and similar
functions is that they do not handle incremental string construction well.
The rsrc_resource_name() function constructs the error message piecemeal,
appending more and more text to a string buffer as it parses the resource.
So in order to use them we would have to reuse the allocated buffer in
successive calls, eg:

   char * msg = bfd_asprintf ("part one");
   msg = bfd_asprintf ("%s %s", msg, "part two");
   msg = bfd_asprintf ("%s %s", msg, "part three");

This should work I think, but it does also seem wasteful of memory.

Cheers
   Nick
ASSI Sept. 2, 2026, 12:36 p.m. UTC | #3 
Nick Clifton writes:
> Hi Andreas,
>
>>>    The fix I have chosen is to change the buffer into a rsrc_string
>>>    structure and to use this to keep track of the space remaining whilst
>>>    constructing the error message.  Which is a lot of faff for a case
>>>    that will almost certainly never arise.  But hey ho, the bug has
>>>    been reported and now it is fixed.
>> Wouldn't it be easier to use bfd_asprintf?
>
> In hindsight - maybe ?  The problem with bfd_asprintf() and similar
> functions is that they do not handle incremental string construction well.
> The rsrc_resource_name() function constructs the error message piecemeal,
> appending more and more text to a string buffer as it parses the resource.
> So in order to use them we would have to reuse the allocated buffer in
> successive calls, eg:
>
>   char * msg = bfd_asprintf ("part one");
>   msg = bfd_asprintf ("%s %s", msg, "part two");
>   msg = bfd_asprintf ("%s %s", msg, "part three");
>
> This should work I think, but it does also seem wasteful of memory.

Wouldn't msg get free'd before you get the chance to use it as an
argument?  You'd want a function that used a passed-in buffer to append
something at the end instead (while keeping the buffer from
overflowing).


Regards,
Achim.
diff mbox series

Patch

diff --git a/bfd/peXXigen.c b/bfd/peXXigen.c
index 9794ecdaabe..17a94c461fe 100644
--- a/bfd/peXXigen.c
+++ b/bfd/peXXigen.c
@@ -3811,92 +3811,152 @@  rsrc_cmp (bool is_name, rsrc_entry * a, rsrc_entry * b)
   return res;
 }
 
-static void
-rsrc_print_name (char * buffer, rsrc_string string)
+static bool
+rsrc_string_print_str (rsrc_string buffer, const char * format, char * context)
+{
+  if (buffer.string == NULL || buffer.len == 0)
+    return false;
+
+  int printed = snprintf ((char *) buffer.string, buffer.len, format, context);
+
+  if (printed >= (int) buffer.len)
+    {
+      buffer.len = 0;
+      return false;
+    }
+
+  buffer.string += printed;
+  buffer.len    -= printed;
+  return true;
+}
+
+static bool
+rsrc_string_print_int (rsrc_string buffer, const char * format, int context)
+{
+  if (buffer.string == NULL || buffer.len == 0)
+    return false;
+
+  int printed = snprintf ((char *) buffer.string, buffer.len, format, context);
+
+  if (printed >= (int) buffer.len)
+    {
+      buffer.len = 0;
+      return false;
+    }
+
+  buffer.string += printed;
+  buffer.len    -= printed;
+  return true;
+}
+
+static bool
+rsrc_print_name (rsrc_string buffer, rsrc_string string)
 {
   unsigned int  i;
   bfd_byte *    name = string.string;
 
   for (i = string.len; i--; name += 2)
-    sprintf (buffer + strlen (buffer), "%.1s", name);
+    {
+      if (! rsrc_string_print_str (buffer, "%.1s", (char *) name))
+	return false;
+    }
+
+  return true;
 }
 
-static const char *
-rsrc_resource_name (rsrc_entry *entry, rsrc_directory *dir, char *buffer)
+static bool
+rsrc_resource_name (rsrc_entry *entry, rsrc_directory *dir, rsrc_string buffer)
 {
   bool is_string = false;
+  bool res = true;
 
-  buffer[0] = 0;
+  if (buffer.string == NULL || buffer.len == 0)
+    return false;
+
+  buffer.string[0] = 0;
 
-  if (dir != NULL && dir->entry != NULL && dir->entry->parent != NULL
+  if (dir != NULL
+      && dir->entry != NULL
+      && dir->entry->parent != NULL
       && dir->entry->parent->entry != NULL)
     {
-      strcpy (buffer, "type: ");
+      res &= rsrc_string_print_str (buffer, "%s", "type: ");
+
       if (dir->entry->parent->entry->is_name)
-	rsrc_print_name (buffer + strlen (buffer),
-			 dir->entry->parent->entry->name_id.name);
+	{
+	  res &= rsrc_print_name (buffer, dir->entry->parent->entry->name_id.name);
+	}
       else
 	{
 	  unsigned int id = dir->entry->parent->entry->name_id.id;
 
-	  sprintf (buffer + strlen (buffer), "%x", id);
+	  res &= rsrc_string_print_int (buffer, "%x", id);
+
 	  switch (id)
 	    {
-	    case 1: strcat (buffer, " (CURSOR)"); break;
-	    case 2: strcat (buffer, " (BITMAP)"); break;
-	    case 3: strcat (buffer, " (ICON)"); break;
-	    case 4: strcat (buffer, " (MENU)"); break;
-	    case 5: strcat (buffer, " (DIALOG)"); break;
-	    case 6: strcat (buffer, " (STRING)"); is_string = true; break;
-	    case 7: strcat (buffer, " (FONTDIR)"); break;
-	    case 8: strcat (buffer, " (FONT)"); break;
-	    case 9: strcat (buffer, " (ACCELERATOR)"); break;
-	    case 10: strcat (buffer, " (RCDATA)"); break;
-	    case 11: strcat (buffer, " (MESSAGETABLE)"); break;
-	    case 12: strcat (buffer, " (GROUP_CURSOR)"); break;
-	    case 14: strcat (buffer, " (GROUP_ICON)"); break;
-	    case 16: strcat (buffer, " (VERSION)"); break;
-	    case 17: strcat (buffer, " (DLGINCLUDE)"); break;
-	    case 19: strcat (buffer, " (PLUGPLAY)"); break;
-	    case 20: strcat (buffer, " (VXD)"); break;
-	    case 21: strcat (buffer, " (ANICURSOR)"); break;
-	    case 22: strcat (buffer, " (ANIICON)"); break;
-	    case 23: strcat (buffer, " (HTML)"); break;
-	    case 24: strcat (buffer, " (MANIFEST)"); break;
-	    case 240: strcat (buffer, " (DLGINIT)"); break;
-	    case 241: strcat (buffer, " (TOOLBAR)"); break;
+	    case   1: res &= rsrc_string_print_str (buffer, "%s", " (CURSOR)"); break;
+	    case   2: res &= rsrc_string_print_str (buffer, "%s", " (BITMAP)"); break;
+	    case   3: res &= rsrc_string_print_str (buffer, "%s", " (ICON)"); break;
+	    case   4: res &= rsrc_string_print_str (buffer, "%s", " (MENU)"); break;
+	    case   5: res &= rsrc_string_print_str (buffer, "%s", " (DIALOG)"); break;
+	    case   6: res &= rsrc_string_print_str (buffer, "%s", " (STRING)"); is_string = true; break;
+	    case   7: res &= rsrc_string_print_str (buffer, "%s", " (FONTDIR)"); break;
+	    case   8: res &= rsrc_string_print_str (buffer, "%s", " (FONT)"); break;
+	    case   9: res &= rsrc_string_print_str (buffer, "%s", " (ACCELERATOR)"); break;
+	    case  10: res &= rsrc_string_print_str (buffer, "%s", " (RCDATA)"); break;
+	    case  11: res &= rsrc_string_print_str (buffer, "%s", " (MESSAGETABLE)"); break;
+	    case  12: res &= rsrc_string_print_str (buffer, "%s", " (GROUP_CURSOR)"); break;
+	    case  14: res &= rsrc_string_print_str (buffer, "%s", " (GROUP_ICON)"); break;
+	    case  16: res &= rsrc_string_print_str (buffer, "%s", " (VERSION)"); break;
+	    case  17: res &= rsrc_string_print_str (buffer, "%s", " (DLGINCLUDE)"); break;
+	    case  19: res &= rsrc_string_print_str (buffer, "%s", " (PLUGPLAY)"); break;
+	    case  20: res &= rsrc_string_print_str (buffer, "%s", " (VXD)"); break;
+	    case  21: res &= rsrc_string_print_str (buffer, "%s", " (ANICURSOR)"); break;
+	    case  22: res &= rsrc_string_print_str (buffer, "%s", " (ANIICON)"); break;
+	    case  23: res &= rsrc_string_print_str (buffer, "%s", " (HTML)"); break;
+	    case  24: res &= rsrc_string_print_str (buffer, "%s", " (MANIFEST)"); break;
+	    case 240: res &= rsrc_string_print_str (buffer, "%s", " (DLGINIT)"); break;
+	    case 241: res &= rsrc_string_print_str (buffer, "%s", " (TOOLBAR)"); break;
 	    }
 	}
     }
 
   if (dir != NULL && dir->entry != NULL)
     {
-      strcat (buffer, " name: ");
+      res &= rsrc_string_print_str (buffer, "%s", " name: ");
+
       if (dir->entry->is_name)
-	rsrc_print_name (buffer + strlen (buffer), dir->entry->name_id.name);
+	{
+	  res &= rsrc_print_name (buffer, dir->entry->name_id.name);
+	}
       else
 	{
 	  unsigned int id = dir->entry->name_id.id;
 
-	  sprintf (buffer + strlen (buffer), "%x", id);
+	  res &= rsrc_string_print_int (buffer, "%x", id);
 
 	  if (is_string)
-	    sprintf (buffer + strlen (buffer), " (resource id range: %d - %d)",
-		     (id - 1) << 4, (id << 4) - 1);
+	    {
+	      res &= rsrc_string_print_str (buffer, "%s", " (resource id range: ");
+	      res &= rsrc_string_print_int (buffer, "%d", (id - 1) << 4);
+	      res &= rsrc_string_print_str (buffer, "%s", " - ");
+	      res &= rsrc_string_print_int (buffer, "%d", (id << 4) - 1);
+	      res &= rsrc_string_print_str (buffer, "%s", ")");
+	    }
 	}
     }
 
   if (entry != NULL)
     {
-      strcat (buffer, " lang: ");
+      res &= rsrc_string_print_str (buffer, "%s", " lang: ");
 
       if (entry->is_name)
-	rsrc_print_name (buffer + strlen (buffer), entry->name_id.name);
+	res &= rsrc_print_name (buffer, entry->name_id.name);
       else
-	sprintf (buffer + strlen (buffer), "%x", entry->name_id.id);
+	res &= rsrc_string_print_int (buffer, "%x", entry->name_id.id);
     }
 
-  return buffer;
+  return res;
 }
 
 /* *sigh* Windows resource strings are special.  Only the top 28-bits of
@@ -4156,11 +4216,20 @@  rsrc_sort_entries (rsrc_dir_chain *chain,
 			_bfd_error_handler (_(".rsrc merge failure: duplicate leaf"));
 		      else
 			{
-			  char buff[256];
-
-			  _bfd_error_handler (_(".rsrc merge failure: duplicate leaf: %s"),
-					      rsrc_resource_name (entry, dir, buff));
+#define RSRC_RES_NAME_LEN 256
+			  char buff[RSRC_RES_NAME_LEN];
+			  rsrc_string buffer;
+
+			  buffer.string = (bfd_byte *) buff;
+			  buffer.len = RSRC_RES_NAME_LEN;
+
+			  if (rsrc_resource_name (entry, dir, buffer))
+			    _bfd_error_handler (_(".rsrc merge failure: duplicate leaf: %s"),
+						buffer.string);
+			  else
+			    _bfd_error_handler (_(".rsrc merge failure: duplicate leaf"));
 			}
+
 		      bfd_set_error (bfd_error_file_truncated);
 		      return;
 		    }