From a1e16797cacfa8559dcbfa0263fcd123d3b5c53b Mon Sep 17 00:00:00 2001 From: John Sterling Date: Sun, 19 Jun 2016 00:34:49 -0400 Subject: [PATCH 1/2] Improve performance of File\ASN1->_decode_ber() for large data This removes the use of _string_shift() which copies the (potentially large) latter part of the input data repeatedly, in favor of maintaining a position var and using string indexing or substr() to only copy the (relatively small) current data as it is parsed. --- phpseclib/File/ASN1.php | 63 ++++++++++++++++++++++------------------- 1 file changed, 34 insertions(+), 29 deletions(-) diff --git a/phpseclib/File/ASN1.php b/phpseclib/File/ASN1.php index f5c3e448..ed36b3b3 100644 --- a/phpseclib/File/ASN1.php +++ b/phpseclib/File/ASN1.php @@ -285,14 +285,15 @@ class File_ASN1 * * @param string $encoded * @param int $start + * @param int $encoded_pos * @return array * @access private */ - function _decode_ber($encoded, $start = 0) + function _decode_ber($encoded, $start = 0, $encoded_pos = 0) { $current = array('start' => $start); - $type = ord($this->_string_shift($encoded)); + $type = ord($encoded[$encoded_pos++]); $start++; $constructed = ($type >> 5) & 1; @@ -304,23 +305,24 @@ class File_ASN1 do { $loop = ord($encoded[0]) >> 7; $tag <<= 7; - $tag |= ord($this->_string_shift($encoded)) & 0x7F; + $tag |= ord($encoded[$encoded_pos++]) & 0x7F; $start++; } while ($loop); } // Length, as discussed in paragraph 8.1.3 of X.690-0207.pdf#page=13 - $length = ord($this->_string_shift($encoded)); + $length = ord($encoded[$encoded_pos++]); $start++; if ($length == 0x80) { // indefinite length // "[A sender shall] use the indefinite form (see 8.1.3.6) if the encoding is constructed and is not all // immediately available." -- paragraph 8.1.3.2.c - $length = strlen($encoded); + $length = strlen($encoded) - $encoded_pos; } elseif ($length & 0x80) { // definite length, long form // technically, the long form of the length can be represented by up to 126 octets (bytes), but we'll only // support it up to four. $length&= 0x7F; - $temp = $this->_string_shift($encoded, $length); + $temp = substr($encoded, $encoded_pos, $length); + $encoded_pos += $length; // tags of indefinte length don't really have a header length; this length includes the tag $current+= array('headerlength' => $length + 2); $start+= $length; @@ -329,11 +331,12 @@ class File_ASN1 $current+= array('headerlength' => 2); } - if ($length > strlen($encoded)) { + if ($length > (strlen($encoded) - $encoded_pos)) { return false; } - $content = $this->_string_shift($encoded, $length); + $content = substr($encoded, $encoded_pos, $length); + $content_pos = 0; // at this point $length can be overwritten. it's only accurate for definite length things as is @@ -366,7 +369,7 @@ class File_ASN1 $temp = $this->_decode_ber($content, $start); $length = $temp['length']; // end-of-content octets - see paragraph 8.1.5 - if (substr($content, $length, 2) == "\0\0") { + if (substr($content, $content_pos + $length, 2) == "\0\0") { $length+= 2; $start+= $length; $newcontent[] = $temp; @@ -375,7 +378,7 @@ class File_ASN1 $start+= $length; $remainingLength-= $length; $newcontent[] = $temp; - $this->_string_shift($content, $length); + $content_pos += $length; } return array( @@ -399,11 +402,11 @@ class File_ASN1 //if (strlen($content) != 1) { // return false; //} - $current['content'] = (bool) ord($content[0]); + $current['content'] = (bool) ord($content[$content_pos]); break; case FILE_ASN1_TYPE_INTEGER: case FILE_ASN1_TYPE_ENUMERATED: - $current['content'] = new Math_BigInteger($content, -256); + $current['content'] = new Math_BigInteger(substr($content, $content_pos), -256); break; case FILE_ASN1_TYPE_REAL: // not currently supported return false; @@ -412,10 +415,10 @@ class File_ASN1 // the number of unused bits in the final subsequent octet. The number shall be in the range zero to // seven. if (!$constructed) { - $current['content'] = $content; + $current['content'] = substr($content, $content_pos); } else { - $temp = $this->_decode_ber($content, $start); - $length-= strlen($content); + $temp = $this->_decode_ber($content, $start, $content_pos); + $length-= (strlen($content) - $content_pos); $last = count($temp) - 1; for ($i = 0; $i < $last; $i++) { // all subtags should be bit strings @@ -433,13 +436,13 @@ class File_ASN1 break; case FILE_ASN1_TYPE_OCTET_STRING: if (!$constructed) { - $current['content'] = $content; + $current['content'] = substr($content, $content_pos); } else { $current['content'] = ''; $length = 0; - while (substr($content, 0, 2) != "\0\0") { - $temp = $this->_decode_ber($content, $length + $start); - $this->_string_shift($content, $temp['length']); + while (substr($content, $content_pos, 2) != "\0\0") { + $temp = $this->_decode_ber($content, $length + $start, $content_pos); + $content_pos += $temp['length']; // all subtags should be octet strings //if ($temp['type'] != FILE_ASN1_TYPE_OCTET_STRING) { // return false; @@ -447,7 +450,7 @@ class File_ASN1 $current['content'].= $temp['content']; $length+= $temp['length']; } - if (substr($content, 0, 2) == "\0\0") { + if (substr($content, $content_pos, 2) == "\0\0") { $length+= 2; // +2 for the EOC } } @@ -462,26 +465,28 @@ class File_ASN1 case FILE_ASN1_TYPE_SET: $offset = 0; $current['content'] = array(); - while (strlen($content)) { + $content_len = strlen($content); + while ($content_pos < $content_len) { // if indefinite length construction was used and we have an end-of-content string next // see paragraphs 8.1.1.3, 8.1.3.2, 8.1.3.6, 8.1.5, and (for an example) 8.6.4.2 - if (!isset($current['headerlength']) && substr($content, 0, 2) == "\0\0") { + if (!isset($current['headerlength']) && substr($content, $content_pos, 2) == "\0\0") { $length = $offset + 2; // +2 for the EOC break 2; } - $temp = $this->_decode_ber($content, $start + $offset); - $this->_string_shift($content, $temp['length']); + $temp = $this->_decode_ber($content, $start + $offset, $content_pos); + $content_pos += $temp['length']; $current['content'][] = $temp; $offset+= $temp['length']; } break; case FILE_ASN1_TYPE_OBJECT_IDENTIFIER: - $temp = ord($this->_string_shift($content)); + $temp = ord($content[$content_pos++]); $current['content'] = sprintf('%d.%d', floor($temp / 40), $temp % 40); $valuen = 0; // process septets - while (strlen($content)) { - $temp = ord($this->_string_shift($content)); + $content_len = strlen($content); + while ($content_pos < $content_len) { + $temp = ord($content[$content_pos++]); $valuen <<= 7; $valuen |= $temp & 0x7F; if (~$temp & 0x80) { @@ -522,11 +527,11 @@ class File_ASN1 case FILE_ASN1_TYPE_UTF8_STRING: // ???? case FILE_ASN1_TYPE_BMP_STRING: - $current['content'] = $content; + $current['content'] = substr($content, $content_pos); break; case FILE_ASN1_TYPE_UTC_TIME: case FILE_ASN1_TYPE_GENERALIZED_TIME: - $current['content'] = $this->_decodeTime($content, $tag); + $current['content'] = $this->_decodeTime(substr($content, $content_pos), $tag); default: } From 88ce26f8ca405a7572ef584b63131e0ef3c4b170 Mon Sep 17 00:00:00 2001 From: John Sterling Date: Sat, 18 Jun 2016 23:03:20 -0400 Subject: [PATCH 2/2] Improve performance of File\X509->_mapInExtensions() for large arrays This avoids passing array references by-value to is_array() (which would trigger a copy) by refactoring _subArray() into a separate is_array() check on a by-value var, and a separate unchecked reference return. --- phpseclib/File/X509.php | 95 +++++++++++++++++++++++++++++++++++++---- 1 file changed, 86 insertions(+), 9 deletions(-) diff --git a/phpseclib/File/X509.php b/phpseclib/File/X509.php index 70cad035..91d90429 100644 --- a/phpseclib/File/X509.php +++ b/phpseclib/File/X509.php @@ -1505,7 +1505,9 @@ class File_X509 $this->signatureSubject = substr($cert, $decoded[0]['content'][0]['start'], $decoded[0]['content'][0]['length']); - $this->_mapInExtensions($x509, 'tbsCertificate/extensions', $asn1); + if ($this->_isSubArrayValid($x509, 'tbsCertificate/extensions')) { + $this->_mapInExtensions($x509, 'tbsCertificate/extensions', $asn1); + } $this->_mapInDNs($x509, 'tbsCertificate/issuer/rdnSequence', $asn1); $this->_mapInDNs($x509, 'tbsCertificate/subject/rdnSequence', $asn1); @@ -1609,9 +1611,9 @@ class File_X509 */ function _mapInExtensions(&$root, $path, $asn1) { - $extensions = &$this->_subArray($root, $path); + $extensions = &$this->_subArrayUnchecked($root, $path); - if (is_array($extensions)) { + if ($extensions) { for ($i = 0; $i < count($extensions); $i++) { $id = $extensions[$i]['extnId']; $value = &$extensions[$i]['extnValue']; @@ -1743,7 +1745,7 @@ class File_X509 if ($mapped !== false) { $values[$j] = $mapped; } - if ($id == 'pkcs-9-at-extensionRequest') { + if ($id == 'pkcs-9-at-extensionRequest' && $this->_isSubArrayValid($values, $j)) { $this->_mapInExtensions($values, $j, $asn1); } } elseif ($map) { @@ -3276,11 +3278,18 @@ class File_X509 $this->signatureSubject = substr($orig, $decoded[0]['content'][0]['start'], $decoded[0]['content'][0]['length']); $this->_mapInDNs($crl, 'tbsCertList/issuer/rdnSequence', $asn1); - $this->_mapInExtensions($crl, 'tbsCertList/crlExtensions', $asn1); - $rclist = &$this->_subArray($crl, 'tbsCertList/revokedCertificates'); - if (is_array($rclist)) { - foreach ($rclist as $i => $extension) { - $this->_mapInExtensions($rclist, "$i/crlEntryExtensions", $asn1); + if ($this->_isSubArrayValid($crl, 'tbsCertList/crlExtensions')) { + $this->_mapInExtensions($crl, 'tbsCertList/crlExtensions', $asn1); + } + if ($this->_isSubArrayValid($crl, 'tbsCertList/revokedCertificates')) { + $rclist_ref = &$this->_subArrayUnchecked($crl, 'tbsCertList/revokedCertificates'); + if ($rclist_ref) { + $rclist = $crl['tbsCertList']['revokedCertificates']; + foreach ($rclist as $i => $extension) { + if ($this->_isSubArrayValid($rclist, "$i/crlEntryExtensions", $asn1)) { + $this->_mapInExtensions($rclist_ref, "$i/crlEntryExtensions", $asn1); + } + } } } @@ -3903,6 +3912,74 @@ class File_X509 $this->caFlag = true; } + /** + * Check for validity of subarray + * + * This is intended for use in conjunction with _subArrayUnchecked(), + * implementing the checks included in _subArray() but without copying + * a potentially large array by passing its reference by-value to is_array(). + * + * @param array $root + * @param string $path + * @return boolean + * @access private + */ + function _isSubArrayValid($root, $path) + { + if (!is_array($root)) { + return false; + } + + foreach (explode('/', $path) as $i) { + if (!is_array($root)) { + return false; + } + + if (!isset($root[$i])) { + return true; + } + + $root = $root[$i]; + } + + return true; + } + + /** + * Get a reference to a subarray + * + * This variant of _subArray() does no is_array() checking, + * so $root should be checked with _isSubArrayValid() first. + * + * This is here for performance reasons: + * Passing a reference (i.e. $root) by-value (i.e. to is_array()) + * creates a copy. If $root is an especially large array, this is expensive. + * + * @param array $root + * @param string $path absolute path with / as component separator + * @param bool $create optional + * @access private + * @return array|false + */ + function &_subArrayUnchecked(&$root, $path, $create = false) + { + $false = false; + + foreach (explode('/', $path) as $i) { + if (!isset($root[$i])) { + if (!$create) { + return $false; + } + + $root[$i] = array(); + } + + $root = &$root[$i]; + } + + return $root; + } + /** * Get a reference to a subarray *