Workaround for PHP HTML Entity Encoder Heap Overflow Vulnerability

[JavaWoman]

In November 2006, a "highly critical" (Secunia, see [3]) vulnerability in PHP's handling of htmlentities() and htmlspecialchars() was announced (see refs) and fixed by PHP in release 5.2; the  release announcement for PHP 5.2 makes mention of this fix. However, version 4.4.x was vulnerable as well, and although version 4.4.5 has been released since, its  release announcement makes no mention of any fix for this vulnerability. Ref [4] does mention an unofficial patch available from  http://cvs.php.net/.
Many hosters do not even provide version 5.x, and quite a number of them are not particularly quick at upgrading PHP versions, let alone applying unofficial patches. Only installations running on FreeBSD or OpenBSD have a degree of protection ([4]) but it can be safely assumed that is a minority.

The conclusion must be that many of our users' installations will be vulnerable, including practically all running PHP 4.x.

Looking at the details in the Hardened-PHP description ([4]), it seems possible to provide at least a degree of protection by creating a wrapper around the PHP functions that first "manually" replaces the characters leading to a possible buffer overflow. Since it seems all versions below 5.2 currently must be assumed to be vulnerable (the unofficial 4.4.x patch can't be detected), we can simply use a version_compare() against 5.2 to decide whether to do this "manual" replacement.

Such a workaround may not be 100% reliable (no guarantees!) but could at least give a degree of protection for our users' installations.

References:
[1]  http://www.ubuntu.com/usn/usn-375-1
[2]  http://www.securityfocus.com/bid/20879/info
[3]  http://secunia.com/advisories/22653/
[4]  http://www.hardened-php.net/advisory_132006.138.html

[JavaWoman]

Forgot to mention: usage of version_compare will (to a degree) be dependent on #426 and at least on the installer (#141, #97...) providing the current PHP version running.

[JavaWoman]

Unicode characters to be replaced "manually" by corresponding entity refs are those of which the name is longer than 6 characters, which makes the whole entity ref (adding & at the start and ; at the end) longer than the assumed maximum of 8 characters. These are all in the " 24.3 Character entity references for symbols, mathematical symbols, and Greek letters" range; relevant lines from that document:

<!ENTITY Epsilon  CDATA "&#917;" -- greek capital letter epsilon, U+0395 -->
<!ENTITY Omicron  CDATA "&#927;" -- greek capital letter omicron, U+039F -->
<!ENTITY Upsilon  CDATA "&#933;" -- greek capital letter upsilon, U+03A5 ISOgrk3 -->
<!ENTITY epsilon  CDATA "&#949;" -- greek small letter epsilon, U+03B5 ISOgrk3 -->
<!ENTITY omicron  CDATA "&#959;" -- greek small letter omicron, U+03BF NEW --> 
<!ENTITY upsilon  CDATA "&#965;" -- greek small letter upsilon, U+03C5 ISOgrk3 -->
<!ENTITY thetasym CDATA "&#977;" -- greek small letter theta symbol, U+03D1 NEW -->
<!ENTITY alefsym  CDATA "&#8501;" -- alef symbol = first transfinite cardinal, U+2135 NEW -->

[anonymous]

Replying to JavaWoman:
Oops - corrected link: [ 24.3 Character entity references for symbols, mathematical symbols, and Greek letters]

[JavaWoman]

Analysis done, here are the results:

1. We do not actually use htmlentities() anywhere.
2. We do use htmlspecialchars() which is also implicated in the vulnerability.

1. Is actually a good thing, as htmlentities() will replace a number of 'extended ASCII' and Unicode characters with their named character entity references -- and except for the small set of four to escape 'special characters' named character entity references are actually invalid in XML, and thus also in XHTML. Since we generate XHTML code, as well as various XML formats, we should not even be using this.

2. Is more interesting: Why would htmlspecialchars() be even implicated? According to the  PHP documentation for this function:

The translations performed are:

• '&' (ampersand) becomes '&'
• '"' (double quote) becomes '"' when ENT_NOQUOTES is not set.
• "'" (single quote) becomes ''' only when ENT_QUOTES is set.
• '<' (less than) becomes '&lt;'
• '>' (greater than) becomes '&gt;'

(...)

Note that this function does not translate anything beyond what is listed above.

Unfortunately, in my own programming I found that this is (at least in some cases) not entirely true and other characters may be turned into entities -- which is precisely why the vulnerability applies here, too. The fact that the vulnerability reports implicate this function as well and the  Release announcement for PHP 5.2 includes it in the statement about solved security problems suggests that this is indeed the case!

We actually use htmlspecialchars() quite extensively, mostly via out own wrapper function htmlspecialchars_ent() (which avoids 'double-escaping already existing entity references) but in a few cases directly (e.g., in the edit handler). Additionally, the 3rd party tool GeSHi makes heavy use of htmlspecialchars().

The most straightforward way to solve this is obviously to create our own replacement function for htmlspecialchars() which does do exactly what it says on the label, and use this wherever htmlspecialchars() is used now. That is:

• Wakka.class.php (in htmlspecialchars_ent())
• install.php and writeconfig.php (installer)
• edit.php (page handler)
• ini.php & code.php (formatters)
• geshi.php (3rd party plugin)

Since actually overriding PHP internal function is possible only via the PECL runkit extension which is not included in a standard distribution, this implies not only changing our own code, but patching geshi.php as well. This means extra attention will be needed when upgrading our GeSHi version, to re-apply the same changes (and check if there are any new occurrences).

(Note: since Secunia classified this as "highly critical", I'm updating our own Severity to "Critical".)

[DarTar]

Replying to JavaWoman:

Additionally, the 3rd party tool GeSHi makes heavy use of htmlspecialchars(). This means extra attention will be needed when upgrading our GeSHi version, to re-apply the same changes (and check if there are any new occurrences).

Thanks for your detailed analysis! Maybe you could drop a line to the  GeSHi maintainer on this? He's a nice guy and pretty responsive. This would save us the bother of patching GeSHi ourselves.

[JavaWoman]

Minor correction: XHTML actually allows named entity references; I was mentally jumping ahead a bit, since XML only has defined entities for the special characters < > & " and ' . If we embed content in XML (such as RSS and MM XML) it's easier not to have to convert again from named entities to numeric entities.

[JavaWoman]

[317] is a first step towards a solution: a new function hsc_secure() to replace htmlspecialchars(). The name for the function was inspired by waawaamilk (aka Nigel) and kindly provided by BenBE (both from GeSHi), who have been involved meanwhile after Nils alerted them before I had a chance to do that :).

GeSHi will likely use this function as well.

[JavaWoman]

[320] implements the new hsc_secure() method in most cases - only exception: geshi.php; I'll hold back on that in case the GeSHi developers come with a patched version before we release this.

[DarTar]

So embedded HTML now works fine?

[JavaWoman]

Replying to DarTar:

So embedded HTML now works fine?

There is no change to embedded HTML per se - the Formatter still uses htmlspecialchars_ent() - which now uses the new hsc_secure(); the next commit will have some changes in how htmlspecialchars_ent() is implemented, but behavior should not change (we're not really touching the Formatter now).

[JavaWoman]

[323] also includes preliminary changes for new implementation of htmlspecialchars_ent() - test and cleanup to follow (testing one thing I find bugs in the other... getting somewhat mixed up between #427 and #340 now ;))

[JavaWoman]

[324] has the last bits and cleanup.

This leaves only GeSHi - if necessary this can be patched if we want to release before there is a new maintenance release of GeSHi 1.0.7.x (which should use the same replacement function we do).

[JavaWoman]

[325] has a few more minor tweaks and cleanup

[JavaWoman]

[326] - missed one file, added now

[JavaWoman]

waawaamilk (nigel) just alerted me on #wikka that GeSHi version 1.0.7.18 is now available. Once that's included (how?) this issue is fixed.

Note: I don't know what our procedure is for including new versions of 3rd-party software in SVN. SVN has specific features for 3rd-party libraries - are we using any of that? Anyone want to take this on?

[Nigel]

The feature is the 'svn:externals' property. I'm not sure how useful that is for you, as there is no moving tag that tracks the latest stable release. You could, in theory, track the RELEASE_1_0_7_STABLE branch however. That generally only gets action once or twice between releases, then a whole bunch just before.

[DarTar]

Replying to JavaWoman:

waawaamilk (nigel) just alerted me on #wikka that GeSHi version 1.0.7.18 is now available. Once that's included (how?) this issue is fixed.

Good—this is for Nils, our GeSHi upgrade person.

Note: I don't know what our procedure is for including new versions of 3rd-party software in SVN. SVN has specific features for 3rd-party libraries - are we using any of that? Anyone want to take this on?

We are not using any 3rd-party specific feature at the moment. A short explanation why this would be useful is welcome.

[DarTar]

Replying to Nigel:

The feature is the 'svn:externals' property.

I've quickly scanned checked the docs for svn:externals, useful when one wants different subdirectories to come from different locations in a repository, or perhaps from different repositories altogether. I don't quite understand how we could use this SVN property since we currently upgrade 3rd-party libraries only when stable packages are available. This may change the day we want to keep track of development versions of 3rd-party libraries from within the repositories, but I wonder if there's a need for this at the moment. Thoughts?

[JavaWoman]

My memory set a "useful" flag when first reading about SVN's capabilities w.r.t. external libraries, but I don't remember details. My conclusion is that we need to study (again) SVN's capabilities for this. I propose that we leave this (study and optional implementation) for 1.1.7, and for the 1.1.6.3 security release proceed as we've done so far.

[JavaWoman]

milestone changed to security release 1.1.6.3

[JavaWoman]

[337] adds the new GeSHi 1.0.7.18 to trunk (thanks, Nils!); all that's left now is for this to be ported to the 1.1.6.3 branch, and then we can close this issue.

[JavaWoman]

[340] is effectively applying [285] for 1.1.6.3

[JavaWoman]

[345] applies [280]:[283] diff from trunk:

• (no ticket) fixes uninitialized varable $written (caused warning)

[JavaWoman]

[346] applies [77]:[161] diff from trunk

[JavaWoman]

[347] applies [298]:[317] diff from trunk:

• adds new hsc_secure() method #427
• some minor comment/whitespace fixes
• had to conflict resolution manually (don't know why)

[JavaWoman]

[348] applies [4]:[184] diff from trunk (in preparation for next step for #427)

• NOTE: referred to wrong ticket # in commit message

[JavaWoman]

[349] applies [319]:[320] diff from trunk

Some manual work here to exclude changes in trunk made for i18n

[JavaWoman]

[350] applies [294]:[320] diff manually (too many conflicts)

NOTE: there was one conflict but the whole fiel was shown as a single changed, conflicting block.. the trick here is to aplpy changes manually, save, and then do a compare with the repository version to check all is OK. Tedious, but necessary.

[JavaWoman]

[351] applies [318]:[320] diff from trunk - again with some manual conflict resolution

[JavaWoman]

[352] applies [311]:[320] diff from trunk

[JavaWoman]

[353] applies [161]:[320] diff from trunk

[JavaWoman]

[354] applies [321]:[323] diff from trunk (again with manual conflict resolutions); also fixes RSS errors (#340)

[JavaWoman]

[353] applies [4]:[324] diff from trunk:

• implementing hsc_secure() #427
• adding page docblock

[JavaWoman]

[356] applies [320]:324] diff manually (conflict: one big change) for #427

• updates docblock with @uses tag for hsc_secure() (and more)
• some cleanup of commented-out old code
• minor comment additions

[JavaWoman]

[357] applies [161]:[324] diff from trunk: changed interface for htmlspecialchars_ent()

[JavaWoman]

[358] applies [323]:[324] diff from trunk: cleanup of modified htmlspecialchars_ent() implementation

[JavaWoman]

[359] applies [4]:[161] diff from trunk:

• implementation of modified htmlspecialchars_ent() - step 1 #427

[JavaWoman]

[360] applies [294]:[324] diff from trunk (with more manual conflict resolution):

• implementation of modified htmlspecialchars_ent() - step 2 #427

[JavaWoman]

[361] applies [323]:[324] diff from trunk:

• cleanup after modified htmlspecialchars_ent() implementation #427

[JavaWoman]

[362] applies [320]:[324] diff from trunk (#427):

• finish modified htmlspecialchars_ent()
• cleanup after modified htmlspecialchars_ent() implementation

[JavaWoman]

[363] 1.1.6.3: applies diff [324]:[325] from trunk (with manual conflict resolution) :

• adding page docblock #427
• a little cleanup

[JavaWoman]

[364] applies [324]:[325] diff from trunk:

• finishing different implementation of htmlspecialchars_ent()

[JavaWoman]

[365] applies [324]:[325] diff from trunk:

• finishing modified implementation of htmlspecialchars_ent()

[JavaWoman]

[366] applies [324]:[325] diff from trunk:

• finishing modified implementation of htmlspecialchars_ent()

[NilsLindenberg]

To close the vulnerability in GeSHi, too, version 1.0.7.18 has been added to 1.1.6.3:

[370], [371] and [372]: language files udated.

[373]: new language files added

[374]: update of geshi.php and (by mistake) doubling of files in the GeSHI/docs directory, which have been removed in [375], [376], [377] and [378]

[379] and [380]: preparation for case-change of the GeSHi docs, which have been updated in [381]

[382]: update of FormattingRules page (installer/upgrader)

[DarTar]

(In [419]) Fixes wrong call to new hsc_secure() method in the installer. Fixes also an issue with the ordering of clauses in the installer that resulted in FormattingRules being created twice on upgrading - refs #427 and #451

[DarTar]

I'm closing this ticket, since all the necessary code changes have been ported from trunk, including the GeSHi upgrade.