ext/gd: drop XPM support on Windows

1 year ago by Christoph M. Becker — view source — reply

unread

Hi all,

I'm in the progress of updating all libraries required for ext/gd on
Windows. Since libxpm hasn't been updated for quite a while (we're
still shipping libxpm 3.5.12), I've attempted updating to libxpm 3.5.17.
However, besides the already existing mess of needing to fetch several
X11 header files from other repos, I've noticed that support for FOR_MSW
builds has completely been dropped[1]. That makes it even harder to
have a somewhat clean build.

Looking a bit further, I've noticed that three vulnerabilites have been
fixed in libxpm 3.5.15[2]; the third one doesn't affect our builds, but
the first two likely do, causing potential DoS, if crafted XPM images
are read by imagecreatefromxpm() (but not by imagecreatefromstring()
since this doesn't support XPM). While it should be possible to upgrade
to libxpm 3.5.15 (or at least to backport the respective fixes), I don't
think it makes sense to move forward supporting XPM images with ext/gd
on Windows. Besides that this format is typically used on Linux, it is
grossly out-dated. Even Gif is way superior, let alone PNG.

Therefore I suggest dropping XPM support from ext/gd on Windows as soon
as possible (might be a bit late for PHP 8.4, but might still be a good
idea). Note that XBM support is unrelated, since this is handled by the
bundled libgd without relying on any library. Also note that
getimagesize() is also not affected, since it doesn't support XPM anyway.

Any objections, or general thoughts?

[1]
https://gitlab.freedesktop.org/xorg/lib/libxpm/-/commit/b30fd0918f8d99aa718ede3da30f9d29f87063e1
[2] https://lists.x.org/archives/xorg-announce/2023-January/003312.html

Christoph

1 year ago by Niels Dossche — view source — reply

unread

Hi all,

I'm in the progress of updating all libraries required for ext/gd on
Windows. Since libxpm hasn't been updated for quite a while (we're
still shipping libxpm 3.5.12), I've attempted updating to libxpm 3.5.17.
However, besides the already existing mess of needing to fetch several
X11 header files from other repos, I've noticed that support for FOR_MSW
builds has completely been dropped[1]. That makes it even harder to
have a somewhat clean build.

Looking a bit further, I've noticed that three vulnerabilites have been
fixed in libxpm 3.5.15[2]; the third one doesn't affect our builds, but
the first two likely do, causing potential DoS, if crafted XPM images
are read by imagecreatefromxpm() (but not by imagecreatefromstring()
since this doesn't support XPM). While it should be possible to upgrade
to libxpm 3.5.15 (or at least to backport the respective fixes), I don't
think it makes sense to move forward supporting XPM images with ext/gd
on Windows. Besides that this format is typically used on Linux, it is
grossly out-dated. Even Gif is way superior, let alone PNG.

Therefore I suggest dropping XPM support from ext/gd on Windows as soon
as possible (might be a bit late for PHP 8.4, but might still be a good
idea). Note that XBM support is unrelated, since this is handled by the
bundled libgd without relying on any library. Also note that
getimagesize() is also not affected, since it doesn't support XPM anyway.

Any objections, or general thoughts?

[1]
https://gitlab.freedesktop.org/xorg/lib/libxpm/-/commit/b30fd0918f8d99aa718ede3da30f9d29f87063e1
[2] https://lists.x.org/archives/xorg-announce/2023-January/003312.html

Christoph

I agree, let's reduce that maintenance burden.

Kind regards
Niels

1 year ago by Pierre Joye — view source — reply

unread

hi Christoph,

Hi all,

Therefore I suggest dropping XPM support from ext/gd on Windows as soon
as possible (might be a bit late for PHP 8.4, but might still be a good
idea).

Given it is only useful on OSes relying on X/freedesktop, the impact should
be zero.

best,

1 year ago by Christoph M. Becker — view source — reply

unread

The GD PHP extension defines "imagecreatefromxpm" functions. How do you
manage it if XPM support is disabled?

Oh, wow, gd/config.w32 needs to be fixed (anyway). As is, if libxpm is
not available, ext/gd can't be built, what makes no sense, since the
code which needs libxpm is already guarded by HAVE_GD_XPM.

GD (libxpm) must be updated for the currently supported PHP versions to
provide a security fix.

Yeah, although I'm not really concerned about this, since I consider it
highly unlikely that any PHP code running on Windows accepts XPM images
from untrusted sources.

I have applied 5 patches on the master branch of winlibs/libxpm on
php-win-ext fork [1] and tagged "libxpm-3.5.12-1". The patch for the
last CVE has been manually integrated.

How do we integrate this fix? Patch on winlibs-builder? PR on
winlibs/libxpm repository?

This should be fixed in winlibs/libxpm. The patch in winlib-builder
doesn't make sense; I think I did this to make it easier to update
libxpm, though in hindsight this was probably a bad idea.

Christoph

1 year ago by Pierre Joye — view source — reply

unread

hi,

This should be fixed in winlibs/libxpm. The patch in winlib-builder
doesn't make sense; I think I did this to make it easier to update
libxpm, though in hindsight this was probably a bad idea.

I mentioned that long ago, but a maybe faster way to get windows ready
sources is vcpkg:

https://vcpkg.roundtrip.dev/ports/libxpm

only changes required are for static lib names, as we use _a to
differentiate. lib for dll or static links.

best.

1 year ago by Christoph M. Becker — view source — reply

unread

I mentioned that long ago, but a maybe faster way to get windows ready
sources is vcpkg:

https://vcpkg.roundtrip.dev/ports/libxpm

only changes required are for static lib names, as we use _a to
differentiate. lib for dll or static links.

Dropping our own builds in favor of some package manager is a worthwhile
goal, but might be much harder than it seems. There was a discussion
about CMake builds[1] a few years ago, and vcpkg and Conan have been
shortly discussed there. The OP even started to provide some PRs
regarding adding further libraries to Conan, but that apparently came to
nothing.

I can't remember if I ever had a closer look at Conan, but when I was
looking into vcpkg a while ago, I've noticed a couple of problems:
besides some missing packages (even for bundled extensions), and the
naming conventions you've mentioned, maybe the biggest problem was their
build style, namely to package a library with all it's dependencies. We
would need to remove the dependencies after building and packaging to be
able to stick to our distribution style. It seems to me that vcpkg is a
great tool for applications, but not necessarily suitable for for us.

[1] https://externals.io/message/116655

Christoph

1 year ago by Pierre Joye — view source — reply

unread

I mentioned that long ago, but a maybe faster way to get windows ready
sources is vcpkg:

https://vcpkg.roundtrip.dev/ports/libxpm

only changes required are for static lib names, as we use _a to
differentiate. lib for dll or static links.

Dropping our own builds in favor of some package manager is a worthwhile
goal, but might be much harder than it seems.

sorry, I was not asking to use vcpkg, it has been discussed before as you
remind us in this reply :)

But the src are windows build ready and many of our less maintained deps
are maintained there.

best,

1 year ago by Christoph M. Becker — view source — reply

unread

The GD PHP extension defines "imagecreatefromxpm" functions. How do you
manage it if XPM support is disabled?

Oh, wow, gd/config.w32 needs to be fixed (anyway). As is, if libxpm is
not available, ext/gd can't be built, what makes no sense, since the
code which needs libxpm is already guarded by HAVE_GD_XPM.

Just for the record, that is supposed to be fixed with
https://github.com/php/php-src/pull/15846.

Christoph

1 year ago by Derick Rethans — view source — reply

unread

Thank you for this suggestion. I have one question.

The GD PHP extension defines "imagecreatefromxpm" functions. How do you manage it if XPM support is disabled?

The function should not exist in that case, probably commented out through an #ifdef.

cheers
Derick