imageloadfont: Fix font loading in opposite endianness format

[thg2k]

The ability to load fonts of the opposite endianness used to work until php 7.4.0 due to commit 88b6037. The same commit introduced a vulnerability due to a missing overflow check that was fixed later in commit d50532b, but the feature remains broken due to the early overflow check.

Furthermore, the initial implementation was missing the conversion (FLIPWORD macro) for the offset field, thus only fonts with zero offset (starting character) could be loaded correctly.

Note that the missed conversion of the offset endianness is not exploitable as there is no way to craft a font that passes the overflow checks before and after the endianness swapping.

This patch fixes the functionality in a secure way, by guessing the endianness based on the number of characters (typically 256), and then performing the overflow check.

[cmb69]

Thank you for the PR!

However, I'm not sure if trying to cater to endianess makes sense. FLIPWORD assumes SIZEOF_INT == 4, what may not be given, and using signed ints for the header values doesn't look particularly clever anyway. I'd rather deprecate that function, in favor of using TrueType fonts.

Or is anybody still using it? Are you using it?

[thg2k]

Or is anybody still using it? Are you using it?

Yes, and I think it's a pretty handy fallback when freetype is not available, otherwise there is no easy way to print text on images. Also, it's fast, easy to use, very predictable and reproducible.

I'd rather fix it properly. What would be the right way to do it regardless of the int size?

[thg2k]

On a second thought, it's not only the FLIPWORD macro, if sizeof(int) == 8 the whole function would be broken. But honestly are there such systems around? Does the rest of PHP work with that size?

I think the parsing logic can be made independent by reading into a buffer made of uint32_t and then copying them to the GdFont struct after sanitizing it. If you think it makes sense, i'm happy to do that.

[cmb69]

Yes, and I think it's a pretty handy fallback when freetype is not available, otherwise there is no easy way to print text on images. Also, it's fast, easy to use, very predictable and reproducible.

And ugly, but that are a lot of GD drawing primitives anyway.

What would be the right way to do it regardless of the int size?

I think FLIPWORD would need to be converted to an (inline) function, looping over all bytes of the int and swapping them.

But the signedness might be an issue, too. For portable image fonts, we already assume two's complement implementation, which may not be granted. Then, flipping the bytes makes even more assumptions about the implementation.

[thg2k]

@cmb69 I gave it another shot.. do you think this would way it would be OK?

Also is it really necessary to loop over php_stream_read() to accumulate the required data? I got rid of that piece of code but I can restore it if needed.

[cmb69]

I don't think we can just change the specification of the function; in this case that architectures with SIZEOF_INT != 4 would no longer be supported. That would require at least some discussion on the internals mailing list.

And thinking about deprecating imageloadfont() again: I think this is the only reasonable way to proceed; after all, that function is almost useless for anybody who need a character set with more than 256 bytes. Even, for instance, German and Italian texts would need to be converted into something useable from UTF-8. That doesn't make sense nowadays.

Note that I'm not suggesting to deprecate the rest of the GdFont support. There are 5 built-in fonts, that can still be used, and that code is written and maintained by libgd developers, and unless they deprecate that, we can stick with it.