Title: Musings on the security fixes from SPIP 4.1.8
Date: 2023-02-28 18:15

Yesterday, the SPIP team released [SPIP
4.1.8](https://git.spip.net/spip/spip/src/branch/4.1/CHANGELOG.md#4-1-8-2023-02-27),
fixing a critical security issue. The bug in question was introduced by
[4b83cb23ccbaa433fedc51040479230115bb4b5c](https://git.spip.net/spip/spip/commit/4b83cb23ccbaa433fedc51040479230115bb4b5c),
the 17<sup>th</sup> of Match 2010. SPIP uses
[`eval`](https://www.php.net/manual/en/function.eval.php) for its templating
system, so being able to inject `<?php` in it is game over: this is exactly
what happened here and it was
[patched](https://git.spip.net/spip/spip/commit/0e6c4f114acf362f85122d078572f1b8d856318a)
like this:

```diff
function protege_champ($texte) {
	if (is_array($texte)) {
		return array_map('protege_champ', $texte);
	} elseif ($texte === null) {
		return $texte;
	} elseif (is_bool($texte)) {
		return $texte ? '1' : '';
	} elseif (is_string($texte) and $texte) {
-		if (preg_match(',^[abis]:\d+[:;],', $texte) and @unserialize($texte) !== false) {
-			// ne pas corrompre une valeur serialize
-			return $texte;
-		} elseif (strpbrk($texte, "&\"'<>") !== false) {
+		if (strpbrk($texte, "&\"'<>") !== false) {
			return spip_htmlspecialchars($texte, ENT_QUOTES);
		}
	}

	return $texte;
}
```

The exploit is
[obvious](https://github.com/rapid7/metasploit-framework/pull/17711): pass a
string that can be unserialized, containing your payload in php, like
`s:22:"<?php system('id');?>";`. Various people have been sitting on this one
since years.

Amusingly, there is a denial of service that was always present for
sufficiently nested arrays, because of the recursive call, on which [php can
choke with a segfault](https://bugs.php.net/bug.php?id=64196).

What is more worrying, is that the patch is making use of the
[`strpbrk`](https://www.php.net/manual/en/function.strpbrk.php), which can be
made to return false should its first parameter contain a NULL byte before the
characters being looked for. But there is a twist: SPIP has a "[security
screen](https://www.spip.net/en_article4201.html)", a simili web application
firewall, a single file that can be updated independently, to mitigate known
vulnerabilities, giving the time to the administrator to upgrade at a later
point.

It contains [this particular
check](https://git.spip.net/spip/spip/src/branch/master/config/ecran_securite.php#L406-L416),
preventing us from injecting NULL-bytes:

```php
if (strpos(
	(function_exists('get_magic_quotes_gpc') and @get_magic_quotes_gpc())
		? stripslashes(serialize($_REQUEST))
		: serialize($_REQUEST),
	chr(0)
) !== false) {
	$ecran_securite_raison = "%00";
}
```

This is quite brittle, since the format used by
[`serialize`](https://www.php.net/manual/en/function.serialize.php) is more or
less opaque, and could absolutely change its representation to encode NULL byte
at some point. Interestingly, serialized classes *do* contain NULL-bytes, but
more on this later.

Anyway, this is the
[snippet](https://git.spip.net/spip/spip/commit/2e4b6c441369bec04c8af137adb2583dae2ec11f
) add to the security screen to mitigate the vulnerability:

```php
if (
	isset($_REQUEST['formulaire_action_args'])
) {
	foreach ($_REQUEST as $k => $v) {
		if (is_string($v)
		  and strpos($v, ':') !== false
		  and strpos($v, '"') !==false
		  and preg_match(',[bidsaO]:,', $v)
		  and @unserialize($v)) {
			$_REQUEST[$k] = htmlentities($v);
			if (isset($_POST[$k])) $_POST[$k] = $_REQUEST[$k];
			if (isset($_GET[$k])) $_GET[$k] = $_REQUEST[$k];
		}
	}
}
```

A couple of things are standing out:

1. `strpos` could be bypassed by NULL-byte injection, should this snippet be
   placed before the aforementioned one. This isn't the case.
2. The needle argument in `preg_match` doesn't include [all the possible type
   specifiers](https://www.phpinternalsbook.com/php5/classes_objects/serialization.html),
   so a payload like `oubli=C:11:"HashContext":21:{<?php system('id');?>}`
   would bypass it, albeit it wouldn't be executed since it doesn't match
   `preg_match(',^[abis]:\d+[:;],', $texte)`.
3. The call to
   [`unserialize`](https://www.php.net/manual/en/function.unserialize.php) is
   scary, if only for the big fat red warning in PHP's documentation: "Do not
   pass untrusted user input to unserialize()". Fortunately, there is not a
   single call to magic methods in the whole SPIP core: no `__wakeup`,
   `__destruct`, ……  but some extensions are making use of those. Oh, and also,
   odds are that there are
   [still](https://www.evonide.com/fuzzing-unserialize/) some exploitable
   memory corruptions in it ;)

This is all super-brittle, and I've been told that more low-hanging
unauthenticated RCE are lurking in the dark. The right move would be to move
away from SPIP, but this isn't always possible. Then there is of course
[snuffleupagus](https://github.com/jvoisin/snuffleupagus), but given the
architecture of SPIP, it's non-trivial to mitigate things there, but all hope
is not lost: One can use an [allow list in
`eval`](https://snuffleupagus.readthedocs.io/config.html#eval-white-and-blacklist),
with something like `sp.eval_whitelist.list("strlen,strcmp,echo");` to prevent
code execution via template injection. Moreover, dangerous functions like
`system`, `shell_exec`, …… should have their calls logged via
`sp.disable_function.function("system").simulation();`. But keep in mind that
bypasses are both plenty and not particularly hard. For this particular
vulnerability, [`sp.unserialize_hmac.enable();`](https://snuffleupagus.readthedocs.io/features.html#unserialize-related-magic)
does mitigate it, by side-effect.

Thanks to [Laluka](https://thinkloveshare.com/about/) for being my rubber duck
debugger when it comes to SPIP.