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

[Po Lu via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

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.