GNU bug report logs - #72245
[PATCH] Fix integer overflow when reading XPM

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 72245 in the body.
You can then email your comments to 72245 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: Stefan Kangas <stefankangas <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Fix integer overflow when reading XPM
Date: Mon, 22 Jul 2024 07:35:55 -0700

[Message part 1 (text/plain, inline)]

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.

[0001-Fix-integer-overflow-when-reading-XPM.patch (text/x-patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefankangas <at> gmail.com>,
 Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 72245 <at> debbugs.gnu.org
Subject: Re: bug#72245: [PATCH] Fix integer overflow when reading XPM
Date: Mon, 22 Jul 2024 18:01:54 +0300

> From: Stefan Kangas <stefankangas <at> 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 <at> 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
> 

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eli Zaretskii <eliz <at> gnu.org>, Stefan Kangas <stefankangas <at> gmail.com>
Cc: 72245 <at> debbugs.gnu.org
Subject: Re: bug#72245: [PATCH] Fix integer overflow when reading XPM
Date: Mon, 22 Jul 2024 08:39:32 -0700

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;'.

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>, Eli Zaretskii <eliz <at> gnu.org>
Cc: 72245 <at> debbugs.gnu.org
Subject: Re: bug#72245: [PATCH] Fix integer overflow when reading XPM
Date: Mon, 22 Jul 2024 08:48:25 -0700

[Message part 1 (text/plain, inline)]

Paul Eggert <eggert <at> 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.

[0001-Fix-integer-overflow-when-reading-XPM.patch (text/x-patch, attachment)]

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

From: Po Lu <luangruo <at> yahoo.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 72245 <at> debbugs.gnu.org
Subject: Re: bug#72245: [PATCH] Fix integer overflow when reading XPM
Date: Tue, 23 Jul 2024 10:06:01 +0800

Stefan Kangas <stefankangas <at> 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?

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Po Lu <luangruo <at> yahoo.com>
Cc: 72245 <at> debbugs.gnu.org
Subject: Re: bug#72245: [PATCH] Fix integer overflow when reading XPM
Date: Mon, 22 Jul 2024 20:04:23 -0700

Po Lu <luangruo <at> yahoo.com> writes:

> Stefan Kangas <stefankangas <at> 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

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

From: Po Lu <luangruo <at> yahoo.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 72245 <at> debbugs.gnu.org
Subject: Re: bug#72245: [PATCH] Fix integer overflow when reading XPM
Date: Tue, 23 Jul 2024 11:41:01 +0800

Stefan Kangas <stefankangas <at> gmail.com> writes:

> Po Lu <luangruo <at> yahoo.com> writes:
>
>> Stefan Kangas <stefankangas <at> 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.

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Po Lu <luangruo <at> yahoo.com>
Cc: 72245 <at> debbugs.gnu.org
Subject: Re: bug#72245: [PATCH] Fix integer overflow when reading XPM
Date: Mon, 22 Jul 2024 21:12:44 -0700

Po Lu <luangruo <at> 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.

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

From: Po Lu <luangruo <at> yahoo.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 72245 <at> debbugs.gnu.org
Subject: Re: bug#72245: [PATCH] Fix integer overflow when reading XPM
Date: Tue, 23 Jul 2024 12:45:44 +0800

Stefan Kangas <stefankangas <at> gmail.com> writes:

> Po Lu <luangruo <at> 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.

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Po Lu <luangruo <at> yahoo.com>
Cc: 72245 <at> debbugs.gnu.org
Subject: Re: bug#72245: [PATCH] Fix integer overflow when reading XPM
Date: Tue, 23 Jul 2024 07:51:29 -0700

Po Lu <luangruo <at> 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.

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

From: Po Lu <luangruo <at> yahoo.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 72245 <at> debbugs.gnu.org
Subject: Re: bug#72245: [PATCH] Fix integer overflow when reading XPM
Date: Tue, 23 Jul 2024 23:15:09 +0800

Stefan Kangas <stefankangas <at> gmail.com> writes:

> Po Lu <luangruo <at> 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.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: luangruo <at> yahoo.com, 72245 <at> debbugs.gnu.org
Subject: Re: bug#72245: [PATCH] Fix integer overflow when reading XPM
Date: Tue, 23 Jul 2024 18:33:41 +0300

> Cc: 72245 <at> debbugs.gnu.org
> From: Stefan Kangas <stefankangas <at> 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.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Po Lu <luangruo <at> yahoo.com>
Cc: 72245 <at> debbugs.gnu.org, stefankangas <at> gmail.com
Subject: Re: bug#72245: [PATCH] Fix integer overflow when reading XPM
Date: Tue, 23 Jul 2024 18:39:04 +0300

> Cc: 72245 <at> 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 <at> 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.

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

From: Andreas Schwab <schwab <at> linux-m68k.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: luangruo <at> yahoo.com, 72245 <at> debbugs.gnu.org,
 Stefan Kangas <stefankangas <at> gmail.com>
Subject: Re: bug#72245: [PATCH] Fix integer overflow when reading XPM
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.

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

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Andreas Schwab <schwab <at> linux-m68k.org>
Cc: luangruo <at> yahoo.com, 72245 <at> debbugs.gnu.org, stefankangas <at> gmail.com
Subject: Re: bug#72245: [PATCH] Fix integer overflow when reading XPM
Date: Tue, 23 Jul 2024 20:54:52 +0300

> From: Andreas Schwab <schwab <at> linux-m68k.org>
> Cc: Stefan Kangas <stefankangas <at> gmail.com>,  luangruo <at> yahoo.com,
>   72245 <at> 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.

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: luangruo <at> yahoo.com, 72245 <at> debbugs.gnu.org
Subject: Re: bug#72245: [PATCH] Fix integer overflow when reading XPM
Date: Tue, 23 Jul 2024 14:39:42 -0700

Eli Zaretskii <eliz <at> gnu.org> writes:

>> Cc: 72245 <at> debbugs.gnu.org
>> From: Stefan Kangas <stefankangas <at> 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.

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: 72245-done <at> debbugs.gnu.org
Subject: Re: bug#72245: [PATCH] Fix integer overflow when reading XPM
Date: Sun, 1 Sep 2024 04:20:32 -0700

Version: 31.1

Stefan Kangas <stefankangas <at> 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.

Previous Next

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