Fix a type confusion on cookie encryption

Cookies can be arrays (session_id[]=x), and
we called `Z_STRLEN_P(pDest)` without checking if `Z_TYPE_P(pDest) == IS_STRING`,
leading to a type confusion, leaking at least `HashTable->arData`, and the rest
of the code is going to corrupt some stuff, leading to a crash.

While exploitation can't be ruled out, it looks stupidly harder. The array will
be decoded as base64 into another variable, decrypted, and have this value
written back to the array. To obtain a controlled read, an attacker would have
to bruteforce decryption to find an encrypted value that could be properly
decrypted, as we're using authenticated encryption. The next steps are
depending on the heap's layout, which should be pretty deterministic/simple as
cookie decryption happens at RINIT. But since the heap overflow is happening in
the cookies, odds are that they're stored somewhere with other data like
POST/GET/… and thus nothing super-duper juicy. While remote heap exploitation
in PHP have been done in the past, this primitive looks a tad too limited to
yield something powerful enough to pop a shell.

Reported-by: Vozec

1 files changed, 5 insertions, 0 deletions

diff --git a/src/sp_cookie_encryption.c b/src/sp_cookie_encryption.c
index 0c14c70..7b3e088 100644
--- a/src/sp_cookie_encryption.c
+++ b/src/sp_cookie_encryption.c
@@ -31,6 +31,11 @@ int decrypt_cookie(zval *pDest, int num_args, va_list args,
     return ZEND_HASH_APPLY_KEEP;
   }
 
+  /* Cookies can be arrays, like session_id[]=x */
+  if (Z_TYPE_P(pDest) != IS_STRING) {
+    return ZEND_HASH_APPLY_KEEP;
+  }
+
   /* If the cookie has no value, it shouldn't be encrypted. */
   if (0 == Z_STRLEN_P(pDest)) {
     return ZEND_HASH_APPLY_KEEP;