bug#72245: [PATCH] Fix integer overflow when reading XPM

bug#72245: [PATCH] Fix integer overflow when reading XPM

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 172 bytes --]

Severity: minor

Since XPM files are untrusted input, I think we'd better handle integer
overflow when parsing it, in case the file is malformed.

Proposed patch attached.

[-- Attachment #2: 0001-Fix-integer-overflow-when-reading-XPM.patch --]
[-- Type: text/x-patch, Size: 2111 bytes --]

From 2aa0e1ac9705201939b30a8ca39b3354cbd62a8e Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Mon, 22 Jul 2024 16:00:30 +0200
Subject: [PATCH] Fix integer overflow when reading XPM

* src/image.c (xpm_str_to_int): New function.
(xpm_load_image): Avoid integer overflow when reading XPM by replacing
sscanf with strtol, to correctly handle integer overflow when reading a
malformed XPM file.
---
 src/image.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/src/image.c b/src/image.c
index 90e6312e128..d8a8dc57ea9 100644
--- a/src/image.c
+++ b/src/image.c
@@ -19,6 +19,7 @@ Copyright (C) 1989-2024 Free Software Foundation, Inc.
 
 #include <config.h>
 
+#include <errno.h>
 #include <fcntl.h>
 #include <math.h>
 #include <unistd.h>
@@ -6254,6 +6255,27 @@ xpm_str_to_color_key (const char *s)
   return -1;
 }
 
+static int
+xpm_str_to_int (char **buf)
+{
+  char *p;
+
+  errno = 0;
+  long result = strtol (*buf, &p, 10);
+  if (p == *buf || errno == ERANGE || errno == EINVAL
+      || result < INT_MIN || result > INT_MAX)
+    return -1;
+
+  /* Error out if we see something like "12x3xyz".  */
+  if (!c_isspace (*p) && *p != '\0')
+    return -1;
+
+  /* Update position to read next integer.  */
+  *buf = p;
+
+  return (int)result;
+}
+
 static bool
 xpm_load_image (struct frame *f,
                 struct image *img,
@@ -6311,10 +6333,14 @@ #define expect_ident(IDENT)					\
     goto failure;
   memcpy (buffer, beg, len);
   buffer[len] = '\0';
-  if (sscanf (buffer, "%d %d %d %d", &width, &height,
-	      &num_colors, &chars_per_pixel) != 4
-      || width <= 0 || height <= 0
-      || num_colors <= 0 || chars_per_pixel <= 0)
+  char *next_int = buffer;
+  if ((width = xpm_str_to_int (&next_int)) <= 0)
+    goto failure;
+  if ((height = xpm_str_to_int (&next_int)) <= 0)
+    goto failure;
+  if ((num_colors = xpm_str_to_int (&next_int)) <= 0)
+    goto failure;
+  if ((chars_per_pixel = xpm_str_to_int (&next_int)) <= 0)
     goto failure;
 
   if (!check_image_size (f, width, height))
-- 
2.45.2

bug#72245: [PATCH] Fix integer overflow when reading XPM

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Mon, 22 Jul 2024 07:35:55 -0700
> 
> Since XPM files are untrusted input, I think we'd better handle integer
> overflow when parsing it, in case the file is malformed.
> 
> Proposed patch attached.

Thanks.

Paul, any comments or suggestions?

> From 2aa0e1ac9705201939b30a8ca39b3354cbd62a8e Mon Sep 17 00:00:00 2001
> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Mon, 22 Jul 2024 16:00:30 +0200
> Subject: [PATCH] Fix integer overflow when reading XPM
> 
> * src/image.c (xpm_str_to_int): New function.
> (xpm_load_image): Avoid integer overflow when reading XPM by replacing
> sscanf with strtol, to correctly handle integer overflow when reading a
> malformed XPM file.
> ---
>  src/image.c | 34 ++++++++++++++++++++++++++++++----
>  1 file changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/src/image.c b/src/image.c
> index 90e6312e128..d8a8dc57ea9 100644
> --- a/src/image.c
> +++ b/src/image.c
> @@ -19,6 +19,7 @@ Copyright (C) 1989-2024 Free Software Foundation, Inc.
>  
>  #include <config.h>
>  
> +#include <errno.h>
>  #include <fcntl.h>
>  #include <math.h>
>  #include <unistd.h>
> @@ -6254,6 +6255,27 @@ xpm_str_to_color_key (const char *s)
>    return -1;
>  }
>  
> +static int
> +xpm_str_to_int (char **buf)
> +{
> +  char *p;
> +
> +  errno = 0;
> +  long result = strtol (*buf, &p, 10);
> +  if (p == *buf || errno == ERANGE || errno == EINVAL
> +      || result < INT_MIN || result > INT_MAX)
> +    return -1;
> +
> +  /* Error out if we see something like "12x3xyz".  */
> +  if (!c_isspace (*p) && *p != '\0')
> +    return -1;
> +
> +  /* Update position to read next integer.  */
> +  *buf = p;
> +
> +  return (int)result;
> +}
> +
>  static bool
>  xpm_load_image (struct frame *f,
>                  struct image *img,
> @@ -6311,10 +6333,14 @@ #define expect_ident(IDENT)					\
>      goto failure;
>    memcpy (buffer, beg, len);
>    buffer[len] = '\0';
> -  if (sscanf (buffer, "%d %d %d %d", &width, &height,
> -	      &num_colors, &chars_per_pixel) != 4
> -      || width <= 0 || height <= 0
> -      || num_colors <= 0 || chars_per_pixel <= 0)
> +  char *next_int = buffer;
> +  if ((width = xpm_str_to_int (&next_int)) <= 0)
> +    goto failure;
> +  if ((height = xpm_str_to_int (&next_int)) <= 0)
> +    goto failure;
> +  if ((num_colors = xpm_str_to_int (&next_int)) <= 0)
> +    goto failure;
> +  if ((chars_per_pixel = xpm_str_to_int (&next_int)) <= 0)
>      goto failure;
>  
>    if (!check_image_size (f, width, height))
> -- 
> 2.45.2
> 

bug#72245: [PATCH] Fix integer overflow when reading XPM

[Paul Eggert]

On 2024-07-22 08:01, Eli Zaretskii wrote:
> +  if (p == *buf || errno == ERANGE || errno == EINVAL

This should be:

    if (errno || p == *buf

as other errors are possible at least in theory, and p might be 
uninitialized on error.

>> +  return (int)result;

As a style matter this cast does more harm than good, as it will 
suppress a static check if 'result' happens to be a pointer type, and it 
could suppress a dynamic check on some debugging-oriented systems. I 
would say just 'return result;'.

bug#72245: [PATCH] Fix integer overflow when reading XPM

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 651 bytes --]

Paul Eggert <eggert@cs.ucla.edu> writes:

> On 2024-07-22 08:01, Eli Zaretskii wrote:
>> +  if (p == *buf || errno == ERANGE || errno == EINVAL
>
> This should be:
>
>     if (errno || p == *buf
>
> as other errors are possible at least in theory, and p might be
> uninitialized on error.
>
>>> +  return (int)result;
>
> As a style matter this cast does more harm than good, as it will
> suppress a static check if 'result' happens to be a pointer type, and it
> could suppress a dynamic check on some debugging-oriented systems. I
> would say just 'return result;'.

Thanks for reviewing.

I've attached an updated patch with your proposed changes.

[-- Attachment #2: 0001-Fix-integer-overflow-when-reading-XPM.patch --]
[-- Type: text/x-patch, Size: 2069 bytes --]

From 6444e4bbd0c5a3af1e7914b6dafaa5b9eb0cfad6 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Mon, 22 Jul 2024 16:00:30 +0200
Subject: [PATCH] Fix integer overflow when reading XPM

* src/image.c (xpm_str_to_int): New function.
(xpm_load_image): Avoid integer overflow when reading XPM by replacing
sscanf with strtol, to correctly handle integer overflow when reading a
malformed XPM file.
---
 src/image.c | 33 +++++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/src/image.c b/src/image.c
index 90e6312e128..464e4567de2 100644
--- a/src/image.c
+++ b/src/image.c
@@ -19,6 +19,7 @@ Copyright (C) 1989-2024 Free Software Foundation, Inc.
 
 #include <config.h>
 
+#include <errno.h>
 #include <fcntl.h>
 #include <math.h>
 #include <unistd.h>
@@ -6254,6 +6255,26 @@ xpm_str_to_color_key (const char *s)
   return -1;
 }
 
+static int
+xpm_str_to_int (char **buf)
+{
+  char *p;
+
+  errno = 0;
+  long result = strtol (*buf, &p, 10);
+  if (errno || p == *buf || result < INT_MIN || result > INT_MAX)
+    return -1;
+
+  /* Error out if we see something like "12x3xyz".  */
+  if (!c_isspace (*p) && *p != '\0')
+    return -1;
+
+  /* Update position to read next integer.  */
+  *buf = p;
+
+  return result;
+}
+
 static bool
 xpm_load_image (struct frame *f,
                 struct image *img,
@@ -6311,10 +6332,14 @@ #define expect_ident(IDENT)					\
     goto failure;
   memcpy (buffer, beg, len);
   buffer[len] = '\0';
-  if (sscanf (buffer, "%d %d %d %d", &width, &height,
-	      &num_colors, &chars_per_pixel) != 4
-      || width <= 0 || height <= 0
-      || num_colors <= 0 || chars_per_pixel <= 0)
+  char *next_int = buffer;
+  if ((width = xpm_str_to_int (&next_int)) <= 0)
+    goto failure;
+  if ((height = xpm_str_to_int (&next_int)) <= 0)
+    goto failure;
+  if ((num_colors = xpm_str_to_int (&next_int)) <= 0)
+    goto failure;
+  if ((chars_per_pixel = xpm_str_to_int (&next_int)) <= 0)
     goto failure;
 
   if (!check_image_size (f, width, height))
-- 
2.45.2

bug#72245: [PATCH] Fix integer overflow when reading XPM

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Stefan Kangas <stefankangas@gmail.com> writes:

> Severity: minor
>
> Since XPM files are untrusted input, I think we'd better handle
> integer
> overflow when parsing it, in case the file is malformed.
>
> Proposed patch attached.

What are the security implications of accepting whatever scanf produces
in the event of an overflow?

bug#72245: [PATCH] Fix integer overflow when reading XPM

[Stefan Kangas]

Po Lu <luangruo@yahoo.com> writes:

> Stefan Kangas <stefankangas@gmail.com> writes:
>
>> Severity: minor
>>
>> Since XPM files are untrusted input, I think we'd better handle
>> integer
>> overflow when parsing it, in case the file is malformed.
>>
>> Proposed patch attached.
>
> What are the security implications of accepting whatever scanf produces
> in the event of an overflow?

There is a good summary here:

    https://cwe.mitre.org/data/definitions/190.html

bug#72245: [PATCH] Fix integer overflow when reading XPM

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Stefan Kangas <stefankangas@gmail.com> writes:

> Po Lu <luangruo@yahoo.com> writes:
>
>> Stefan Kangas <stefankangas@gmail.com> writes:
>>
>>> Severity: minor
>>>
>>> Since XPM files are untrusted input, I think we'd better handle
>>> integer
>>> overflow when parsing it, in case the file is malformed.
>>>
>>> Proposed patch attached.
>>
>> What are the security implications of accepting whatever scanf produces
>> in the event of an overflow?
>
> There is a good summary here:
>
>     https://cwe.mitre.org/data/definitions/190.html

I'm asking which component of xpm_load_image is not adequately prepared
to reject excessive values of these image dimension fields, for the
immediately adjacent statements verify that width, height, num_colors,
and chars_per_pixel are not invalid.  Otherwise I can find no reason to
substantially reinvent the wheel and complicate image.c with a pedantic
10-line function for reading numbers with overflow checking,
implementations of which already abound in that file in one shape or
another.

bug#72245: [PATCH] Fix integer overflow when reading XPM

[Stefan Kangas]

Po Lu <luangruo@yahoo.com> writes:

> Otherwise I can find no reason to substantially reinvent the wheel and
> complicate image.c with a pedantic 10-line function for reading
> numbers with overflow checking, implementations of which already
> abound in that file in one shape or another.

Thanks, but this diatribe doesn't really help.  If you think you can do
a better job, then fine by me.  Please show us the patch.

bug#72245: [PATCH] Fix integer overflow when reading XPM

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Stefan Kangas <stefankangas@gmail.com> writes:

> Po Lu <luangruo@yahoo.com> writes:
>
>> Otherwise I can find no reason to substantially reinvent the wheel and
>> complicate image.c with a pedantic 10-line function for reading
>> numbers with overflow checking, implementations of which already
>> abound in that file in one shape or another.
>
> Thanks, but this diatribe doesn't really help.  If you think you can do
> a better job, then fine by me.  Please show us the patch.

I'm saying that there is nothing to be done.  This change is needless,
and the report should be closed, whatever opinions the security theater
might hold on the matter.

bug#72245: [PATCH] Fix integer overflow when reading XPM

[Stefan Kangas]

Po Lu <luangruo@yahoo.com> writes:

> I'm saying that there is nothing to be done.  This change is needless,
> and the report should be closed, whatever opinions the security theater
> might hold on the matter.

I wasn't the one that started a subthread about security.  You did.

The primary consideration here is correctness.  Undefined behaviour is
generally undesirable, and is a source of both bugs and security issues
in the wild.  This is not "security theater", but a fact.  No amount of
handwaving or throwing expletives around will make it go away.

That said, since you are asking, we are indeed discussing security
sensitive code, that is executed without prompting, for example, when
users receive emails or browse the web.  We are also discussing image
processing, an area that is notorious for the bugs and security issues
that tend to lurk in its many complexities.  On the CWE-190 page that I
linked, there are several examples of integer overflow in image
processing that has lead to very real exploits.  This is not some
academic issue.

Whether or not anyone has demonstrated that Emacs can be exploited using
this vector frankly misses the point.  Let's start with making Emacs
behave correctly and predictably in the face of invalid input.  This
really is the bare minimum.  Then we can discuss whether or not we have
more work to do, security implications, and all the rest of it.

XPM being a relatively simple format, I'm sure that this code can be
fully audited.  I invite you to do so, and I'm hoping that this will
reveal that your faith in this code is well-founded.  Meanwhile, I
reported an unrelated crash in XPM image processing in Bug#72255.

Since we don't have an alternative patch, I will install the one I
proposed in the next couple of days.  Thanks.

bug#72245: [PATCH] Fix integer overflow when reading XPM

[Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Stefan Kangas <stefankangas@gmail.com> writes:

> Po Lu <luangruo@yahoo.com> writes:
>
>> I'm saying that there is nothing to be done.  This change is needless,
>> and the report should be closed, whatever opinions the security theater
>> might hold on the matter.
>
> I wasn't the one that started a subthread about security.  You did.
>
> The primary consideration here is correctness.  Undefined behaviour is
> generally undesirable, and is a source of both bugs and security issues
> in the wild.  This is not "security theater", but a fact.  No amount of
> handwaving or throwing expletives around will make it go away.

Why don't you begin by deleteing the undefined behavior in mark_memory?
By definition, after having executed undefined behavior once, all of the
future behavior of a C program becomes undefined.  For this reason
alone, it is meaningless to speak of undefined behavior in Emacs, only
whether specific behavior produces _actual_ crashes or corruption.

> That said, since you are asking, we are indeed discussing security
> sensitive code, that is executed without prompting, for example, when
> users receive emails or browse the web.  We are also discussing image
> processing, an area that is notorious for the bugs and security issues
> that tend to lurk in its many complexities.  On the CWE-190 page that I
> linked, there are several examples of integer overflow in image
> processing that has lead to very real exploits.  This is not some
> academic issue.
>
> Whether or not anyone has demonstrated that Emacs can be exploited using
> this vector frankly misses the point.  Let's start with making Emacs
> behave correctly and predictably in the face of invalid input.  This
> really is the bare minimum.  Then we can discuss whether or not we have
> more work to do, security implications, and all the rest of it.

It behaves as correctly and predictably as it should: it does not crash.

> XPM being a relatively simple format, I'm sure that this code can be
> fully audited.  I invite you to do so, and I'm hoping that this will
> reveal that your faith in this code is well-founded.  Meanwhile, I
> reported an unrelated crash in XPM image processing in Bug#72255.
>
> Since we don't have an alternative patch, I will install the one I
> proposed in the next couple of days.  Thanks.

It is correctly implemented as it stands.  You are essentially proposing
to have code that has not posed difficulties be needlessly complicated
with ugly pedantic error-checking.

bug#72245: [PATCH] Fix integer overflow when reading XPM

[Eli Zaretskii]

> Cc: 72245@debbugs.gnu.org
> Date: Tue, 23 Jul 2024 23:15:09 +0800
> From:  Po Lu via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> > Since we don't have an alternative patch, I will install the one I
> > proposed in the next couple of days.  Thanks.
> 
> It is correctly implemented as it stands.  You are essentially proposing
> to have code that has not posed difficulties be needlessly complicated
> with ugly pedantic error-checking.

This crosses the line.  Stefan is one of the Emacs co-maintainers, and
as such, it's his prerogative to decide to install code changes.  You
have made your point, and abundantly so.  Your opinions have been
heard and overruled.  Please accept that.  There's no need and no
point to say what you think time and again, let alone in harsh words.

bug#72245: [PATCH] Fix integer overflow when reading XPM

[Eli Zaretskii]

> Cc: 72245@debbugs.gnu.org
> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Tue, 23 Jul 2024 07:51:29 -0700
> 
> That said, since you are asking, we are indeed discussing security
> sensitive code, that is executed without prompting, for example, when
> users receive emails or browse the web.

Only in some MUAs, yes?  For example, Rmail doesn't by default show
the images (or any other attachments), it requires a user action to do
so.

> XPM being a relatively simple format, I'm sure that this code can be
> fully audited.  I invite you to do so, and I'm hoping that this will
> reveal that your faith in this code is well-founded.  Meanwhile, I
> reported an unrelated crash in XPM image processing in Bug#72255.

That file doesn't cause a crash on MS-Windows, FWIW, but the code
which processes XPM images in Emacs on Windows is very different.

bug#72245: [PATCH] Fix integer overflow when reading XPM

[Andreas Schwab]

On Jul 23 2024, Eli Zaretskii wrote:

> That file doesn't cause a crash on MS-Windows, FWIW, but the code
> which processes XPM images in Emacs on Windows is very different.

The absence of a crash does not prove anything, though.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#72245: [PATCH] Fix integer overflow when reading XPM

[Eli Zaretskii]

> From: Andreas Schwab <schwab@linux-m68k.org>
> Cc: Stefan Kangas <stefankangas@gmail.com>,  luangruo@yahoo.com,
>   72245@debbugs.gnu.org
> Date: Tue, 23 Jul 2024 19:39:16 +0200
> 
> On Jul 23 2024, Eli Zaretskii wrote:
> 
> > That file doesn't cause a crash on MS-Windows, FWIW, but the code
> > which processes XPM images in Emacs on Windows is very different.
> 
> The absence of a crash does not prove anything, though.

It isn't the absence of a crash alone.  I see an error message in
*Messages* saying the XPM image is invalid, and the window shows an
empty rectangle, as always with invalid images.  So Emacs actually
detects that the image is invalid, announces that, and doesn't try to
show it on the screen.

bug#72245: [PATCH] Fix integer overflow when reading XPM

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 72245@debbugs.gnu.org
>> From: Stefan Kangas <stefankangas@gmail.com>
>> Date: Tue, 23 Jul 2024 07:51:29 -0700
>>
>> That said, since you are asking, we are indeed discussing security
>> sensitive code, that is executed without prompting, for example, when
>> users receive emails or browse the web.
>
> Only in some MUAs, yes?  For example, Rmail doesn't by default show
> the images (or any other attachments), it requires a user action to do
> so.

Yes, and it's presumably also dependent on user options.

I'd hope that most MUAs disable showing images by default (notmuch
does), but I didn't check.

bug#72245: [PATCH] Fix integer overflow when reading XPM

[Stefan Kangas]

Version: 31.1

Stefan Kangas <stefankangas@gmail.com> writes:

> Since we don't have an alternative patch, I will install the one I
> proposed in the next couple of days.  Thanks.

Pushed to master as commit 73277a4097b.  Closing.