Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug 64823: ZTS GD fails to to find system TrueType font #17366

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Jan 5, 2025

First, the $fontfile parameter actually supports a semicolon delimited list of fonts (as documented[1]); thus passing the full string to VCWD_REALPATH() or php_check_open_basedir() makes no sense; we could pass the individual parts, but …

Second, libgd uses an elaborate font detection. There is a hard- coded DEFAULT_PATHDEFAULT_FONTPATH which can be overridden by the environment variable GDFONTPATH. Semantics are like the PATH environment variable. If DEFAULT_PATHDEFAULT_FONTPATH was still exposed (it is no longer as of libgd 2.1.0[2]), we could take that into account, but …

External libgd can be configured with font-config support, so font aliases and even lookup patterns are supported. There is no way to cater to that upfront.

Thus, we no longer interfere with libgd's font lookup. Checking the realpath was already doubtful (we didn't even use the resolved path). Lifting the open_basedir restriction is a bit more delicate, but the manual still states that open_basedir would not apply, and more relevant, not much harm can be done, because libgd only passes the found font to FT_New_Face() which likely fails for any non font files without any error which could reveal sensitive information. And the font file is never written.

It should be noted that this solves lookup of system fonts, does not change the behavior for absolute font paths, but still does not resolve issues with relative paths to font files in ZTS environments using external libgd (our bundled libgd has a workaround for that). This particular issue cannot be solved, so users of ZTS builds still need to add realpath(.) to the GDFONTPATH as documented in the manual (or pass absolute paths as $fontfile).

[1] https://www.php.net/imagettftext
[2] libgd/libgd@2a921c8


Targeting master due to the removal of the open_basedir check.

First, the `$fontfile` parameter actually supports a semicolon
delimited list of fonts (as documented[1]); thus passing the full
string to `VCWD_REALPATH()` or `php_check_open_basedir()` makes no
sense; we could pass the individual parts, but …

Second, libgd uses an elaborate font detection.  There is a hard-
coded `DEFAULT_PATH` which can be overridden by the environment
variable `GDFONTPATH`.  Semantics are like the `PATH` environment
variable.  If `DEFAULT_PATH` was still exposed (it is no longer as of
libgd 2.1.0[2]), we could take that into account, but …

External libgd can be configured with font-config support, so font
aliases and even lookup patterns are supported.  There is no way to
cater to that upfront.

Thus, we no longer interfere with libgd's font lookup.  Checking the
realpath was already doubtful (we didn't even use the resolved path).
Lifting the open_basedir restriction is a bit more delicate, but the
manual still states that open_basedir would not apply, and more
relevant, not much harm can be done, because libgd only passes the
found font to `FT_New_Face()` which likely fails for any non font files
without any error which could reveal sensitive information.  And the
font file is never written.

It should be noted that this solves lookup of system fonts, does not
change the behavior for absolute font paths, but still does not resolve
issues with relative paths to font files in ZTS environments using
external libgd (our bundled libgd has a workaround for that).  This
particular issue cannot be solved, so users of ZTS builds still need to
add `realpath(.)` to the `GDFONTPATH` as documented in the manual (or
pass absolute paths as `$fontfile`).

[1] <https://www.php.net/imagettftext>
[2] <libgd/libgd@2a921c8>
@cmb69
Copy link
Member Author

cmb69 commented Jan 5, 2025

Hmm, maybe use Ubuntu or Noto to fix the test on Ubuntu. Maybe works also for CirleCI (but there we're using bundled libgd).

Ugh, we're using bundled libgd for macOS CI, but this lacks the proper DEFAULT_FONTPATH of external libgd. Should port libgd/libgd@2a921c8.

@cmb69
Copy link
Member Author

cmb69 commented Jan 6, 2025

In the meantime I've learned that

  • ext/gd does not support fontconfig at all (you need to either pass a flag to gdImageStringFTEx() or call gdFTUseFontConfig())
  • at least the default paths for *nix probably make not much sense nowadays; for instance on Debian Bookworm, I have the actual DejaVuSans.ttf in /usr/share/fonts/truetype/dejavu; looking in /usr/share/fonts/truetype won't find anything

So I don't think a test makes much sense. We would either need to know where the font is stored on the executing system and set GDFONTPATH accordingly (but we don't know that), or we would need a SKIPIF section checking whether the font is found, what would defeat the purpose of the test.

@cmb69 cmb69 marked this pull request as ready for review January 6, 2025 18:51
@cmb69 cmb69 requested a review from devnexen as a code owner January 6, 2025 18:51
@cmb69
Copy link
Member Author

cmb69 commented Jan 6, 2025

I'll leave this open for a while, since removing the open_basedir check might be objectionable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants