GNU bug report logs - #75900
doprnt.c buffer overflow

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 75900 in the body.
You can then email your comments to 75900 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: bug-gnu-emacs <at> gnu.org, Paul Eggert <eggert <at> cs.ucla.edu>
Subject: doprnt.c buffer overflow
Date: Mon, 27 Jan 2025 19:13:09 +0000

doprnt sometimes overflows its buffer, causing stack smashing and (if
we're lucky) a glibc abort when make_formatted_string calls it:

(gdb) p make_formatted_string
("01234567890123456789012345678901234567890123456789012345678901234567890123456789)0123456789)01234567890123456789012345678901234567890123456789")
[Thread 0x7fffda9ff6c0 (LWP 20144) exited]
*** stack smashing detected ***: terminated

The reason is that in doprnt, the variable bufsize indicates the number
of remaining bytes in the buf, and bufptr points to the current byte in
the buf, but sometimes a byte is written and bufsize is not updated:

      else if (! LEADING_CODE_P (fmtchar))
	{
	  if (EQ (quoting_style, Qstraight) && fmtchar == '`')
	    fmtchar = '\'';

	  *bufptr++ = fmtchar;
	  continue;
	}

and

	{
	  do
	    *bufptr++ = *src++;
	  while (--srclen != 0);
	}

do this.  In the former case, we must update bufsize as it will be used
again.  In the latter case, it's sufficient to set it to 0 as this is
the last successful printing operation in this call.

A related issue is that if a multibyte character produced by a %s format
option would overflow the buffer, doprnt returns the buffer size minus
one, as expected, but hasn't actually modified all of the buffer data:
the final bytes will refer to uninitialized stack data.

For example, on my system, where FRAME_MESSAGE_BUF_SIZE (f) is 832, this
call:

(gdb) p message ("%s%s", "xxxx", SDATA(Fmake_string (0x8aa, 0x10002, Qt)))

will print the string

"xxxx䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀^@Z"

where the last characters are an ASCII NUL followed by a 'Z' (which I
put in the buffer after it was SAFE_ALLOCA'd).  The 'Z' should have been
overwritten, in which case message would still have printed two
unintended NUL characters.

I intend to fix the first two bugs, and fix message in a somewhat ugly
way for the other two.

Patches to follow once there is a bug number.

Pip

Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: bug-gnu-emacs <at> gnu.org
Cc: Paul Eggert <eggert <at> cs.ucla.edu>
Subject: Re: doprnt.c buffer overflow
Date: Mon, 27 Jan 2025 20:36:28 +0000

"Pip Cet" <pipcet <at> protonmail.com> writes:

> doprnt sometimes overflows its buffer, causing stack smashing and (if
> we're lucky) a glibc abort when make_formatted_string calls it:
>
> (gdb) p make_formatted_string
> ("01234567890123456789012345678901234567890123456789012345678901234567890123456789)0123456789)01234567890123456789012345678901234567890123456789")
> [Thread 0x7fffda9ff6c0 (LWP 20144) exited]
> *** stack smashing detected ***: terminated
>
> The reason is that in doprnt, the variable bufsize indicates the number
> of remaining bytes in the buf, and bufptr points to the current byte in
> the buf, but sometimes a byte is written and bufsize is not updated:
>
>       else if (! LEADING_CODE_P (fmtchar))
> 	{
> 	  if (EQ (quoting_style, Qstraight) && fmtchar == '`')
> 	    fmtchar = '\'';
>
> 	  *bufptr++ = fmtchar;
> 	  continue;
> 	}
>
> and
>
> 	{
> 	  do
> 	    *bufptr++ = *src++;
> 	  while (--srclen != 0);
> 	}
>
> do this.  In the former case, we must update bufsize as it will be used
> again.  In the latter case, it's sufficient to set it to 0 as this is
> the last successful printing operation in this call.
>
> A related issue is that if a multibyte character produced by a %s format
> option would overflow the buffer, doprnt returns the buffer size minus
> one, as expected, but hasn't actually modified all of the buffer data:
> the final bytes will refer to uninitialized stack data.
>
> For example, on my system, where FRAME_MESSAGE_BUF_SIZE (f) is 832, this
> call:
>
> (gdb) p message ("%s%s", "xxxx", SDATA(Fmake_string (0x8aa, 0x10002, Qt)))
>
> will print the string
>
> "xxxx䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀䀀^@Z"
>
> where the last characters are an ASCII NUL followed by a 'Z' (which I
> put in the buffer after it was SAFE_ALLOCA'd).  The 'Z' should have been
> overwritten, in which case message would still have printed two
> unintended NUL characters.
>
> I intend to fix the first two bugs, and fix message in a somewhat ugly
> way for the other two.
>
> Patches to follow once there is a bug number.

The first patch fixes doprnt by avoiding buffer overflows and clearing
the rest of the buffer even when the first character not to fit there
was multibyte.

From 49e1cc9ae476663cb1ddf6106f7a6f64cd558208 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet <at> protonmail.com>
Subject: [PATCH 1/2] Fix buffer overflows in doprnt (bug#75900)

* src/doprnt.c (doprnt): Clear rest of buffer on multibyte overflow.
Always decrement bufsize when writing a byte.
---
 src/doprnt.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/doprnt.c b/src/doprnt.c
index 335223f972a..d4b2d95fdc1 100644
--- a/src/doprnt.c
+++ b/src/doprnt.c
@@ -446,7 +446,8 @@ doprnt (char *buffer, ptrdiff_t bufsize, const char *format,
 		  while (tem != 0);
 
 		  memcpy (bufptr, string, tem);
-		  bufptr[tem] = 0;
+		  while (tem < bufsize)
+		    bufptr[tem++] = 0;
 		  /* Trigger exit from the loop, but make sure we
 		     return to the caller a value which will indicate
 		     that the buffer was too small.  */
@@ -498,6 +499,7 @@ doprnt (char *buffer, ptrdiff_t bufsize, const char *format,
 	    fmtchar = '\'';
 
 	  *bufptr++ = fmtchar;
+	  bufsize--;
 	  continue;
 	}
       else
@@ -523,7 +525,10 @@ doprnt (char *buffer, ptrdiff_t bufsize, const char *format,
       else
 	{
 	  do
-	    *bufptr++ = *src++;
+	    {
+	      *bufptr++ = *src++;
+	      bufsize--;
+	    }
 	  while (--srclen != 0);
 	}
     }
-- 
2.47.1

The second patch fixes interactive vmessage to deal with doprnt's
peculiar behavior when a multibyte character doesn't fit in the buffer:
doprnt returns bufsize - 1, even though there are fewer valid bytes in
the buffer.

Note that the previous allocation of the stack buffer was overly
generous, except in the very unlikely case that FRAME_MESSAGE_BUF_SIZE
returns 0.

From c62aed0270390d19bba251f356f406963c89baf5 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet <at> protonmail.com>
Subject: [PATCH 2/2] Avoid printing NUL characters in 'message' (bug#75900)

* src/xdisp.c (vmessage): Increase buffer size to fit an extra
multibyte character.  On buffer overflow, drop the last multibyte
character to determine an accurate byte length.
---
 src/xdisp.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/src/xdisp.c b/src/xdisp.c
index 5b5cb3849fc..4933315166f 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -12587,10 +12587,19 @@ vmessage (const char *m, va_list ap)
 	      ptrdiff_t len;
 	      ptrdiff_t maxsize = FRAME_MESSAGE_BUF_SIZE (f);
 	      USE_SAFE_ALLOCA;
-	      char *message_buf = SAFE_ALLOCA (maxsize + 1);
-
-	      len = doprnt (message_buf, maxsize, m, 0, ap);
+	      char *message_buf = SAFE_ALLOCA (maxsize + MAX_MULTIBYTE_LENGTH);
 
+	      len = doprnt (message_buf, maxsize + MAX_MULTIBYTE_LENGTH, m, 0, ap);
+	      /* doprnt returns the buffer size minus one when it
+		 truncated a multibyte sequence.  Work around that by
+		 truncating to the last valid multibyte head.  */
+	      if (len >= maxsize)
+		{
+		  len = maxsize - 1;
+		  while (!CHAR_HEAD_P (message_buf[len]))
+		    len--;
+		  message_buf[len] = 0;
+		}
 	      message3 (make_string (message_buf, len));
 	      SAFE_FREE ();
 	    }
-- 
2.47.1

Pip

Message #13 received at 75900-done <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Pip Cet <pipcet <at> protonmail.com>
Cc: 75900-done <at> debbugs.gnu.org
Subject: Re: doprnt.c buffer overflow
Date: Mon, 27 Jan 2025 17:21:46 -0800

Thanks, I installed those along with some related fixups.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.