Merge pull request #1635 from terrafrost/moosa-1.0

cleanup RSA PKCS#1 v1.5 signature verification (CVE-2021-30130)

phpseclib/Crypt/RSA.php

@@ -3001,6 +3001,59 @@ class Crypt_RSA
         return $em;
     }
 
+    /**
+     * EMSA-PKCS1-V1_5-ENCODE (without NULL)
+     *
+     * Quoting https://tools.ietf.org/html/rfc8017#page-65,
+     *
+     * "The parameters field associated with id-sha1, id-sha224, id-sha256,
+     *  id-sha384, id-sha512, id-sha512/224, and id-sha512/256 should
+     *  generally be omitted, but if present, it shall have a value of type
+     *  NULL"
+     *
+     * @access private
+     * @param string $m
+     * @param int $emLen
+     * @return string
+     */
+    function _emsa_pkcs1_v1_5_encode_without_null($m, $emLen)
+    {
+        $h = $this->hash->hash($m);
+        if ($h === false) {
+            return false;
+        }
+
+        switch ($this->hashName) {
+            case 'sha1':
+                $t = pack('H*', '301f300706052b0e03021a0414');
+                break;
+            case 'sha256':
+                $t = pack('H*', '302f300b06096086480165030402010420');
+                break;
+            case 'sha384':
+                $t = pack('H*', '303f300b06096086480165030402020430');
+                break;
+            case 'sha512':
+                $t = pack('H*', '304f300b06096086480165030402030440');
+                break;
+            default:
+                return false;
+        }
+        $t.= $h;
+        $tLen = strlen($t);
+
+        if ($emLen < $tLen + 11) {
+            user_error('Intended encoded message length too short');
+            return false;
+        }
+
+        $ps = str_repeat(chr(0xFF), $emLen - $tLen - 3);
+
+        $em = "\0\1$ps\0$t";
+
+        return $em;
+    }
+
     /**
      * RSASSA-PKCS1-V1_5-SIGN
      *
@@ -3067,13 +3120,17 @@ class Crypt_RSA
         // EMSA-PKCS1-v1_5 encoding
 
         $em2 = $this->_emsa_pkcs1_v1_5_encode($m, $this->k);
-        if ($em2 === false) {
+        $em3 = $this->_emsa_pkcs1_v1_5_encode_without_null($m, $this->k);
+
+        if ($em2 === false && $em3 === false) {
             user_error('RSA modulus too short');
             return false;
         }
 
         // Compare
-        return $this->_equals($em, $em2);
+
+        return ($em2 !== false && $this->_equals($em, $em2)) ||
+               ($em3 !== false && $this->_equals($em, $em3));
     }
 
     /**

phpseclib/File/ASN1.php

@@ -317,7 +317,7 @@ class File_ASN1
         $current = array('start' => $start);
 
         $type = ord($encoded[$encoded_pos++]);
-        $start++;
+        $startOffset = 1;
 
         $constructed = ($type >> 5) & 1;
 
@@ -327,13 +327,20 @@ class File_ASN1
             // process septets (since the eighth bit is ignored, it's not an octet)
             do {
                 $temp = ord($encoded[$encoded_pos++]);
+                $startOffset++;
                 $loop = $temp >> 7;
                 $tag <<= 7;
-                $tag |= $temp & 0x7F;
+                $temp &= 0x7F;
-                $start++;
+                // "bits 7 to 1 of the first subsequent octet shall not all be zero"
+                if ($startOffset == 2 && $temp == 0) {
+                    return false;
+                }
+                $tag |= $temp;
             } while ($loop);
         }
 
+        $start+= $startOffset;
+
         // Length, as discussed in paragraph 8.1.3 of X.690-0207.pdf#page=13
         $length = ord($encoded[$encoded_pos++]);
         $start++;
@@ -426,13 +433,16 @@ class File_ASN1
         switch ($tag) {
             case FILE_ASN1_TYPE_BOOLEAN:
                 // "The contents octets shall consist of a single octet." -- paragraph 8.2.1
-                //if (strlen($content) != 1) {
+                if ($constructed || strlen($content) != 1) {
-                //    return false;
+                    return false;
-                //}
+                }
                 $current['content'] = (bool) ord($content[$content_pos]);
                 break;
             case FILE_ASN1_TYPE_INTEGER:
             case FILE_ASN1_TYPE_ENUMERATED:
+                if ($constructed) {
+                    return false;
+                }
                 $current['content'] = new Math_BigInteger(substr($content, $content_pos), -256);
                 break;
             case FILE_ASN1_TYPE_REAL: // not currently supported
@@ -452,15 +462,15 @@ class File_ASN1
                     $last = count($temp) - 1;
                     for ($i = 0; $i < $last; $i++) {
                         // all subtags should be bit strings
-                        //if ($temp[$i]['type'] != FILE_ASN1_TYPE_BIT_STRING) {
+                        if ($temp[$i]['type'] != FILE_ASN1_TYPE_BIT_STRING) {
-                        //    return false;
+                            return false;
-                        //}
+                        }
                         $current['content'].= substr($temp[$i]['content'], 1);
                     }
                     // all subtags should be bit strings
-                    //if ($temp[$last]['type'] != FILE_ASN1_TYPE_BIT_STRING) {
+                    if ($temp[$last]['type'] != FILE_ASN1_TYPE_BIT_STRING) {
-                    //    return false;
+                        return false;
-                    //}
+                    }
                     $current['content'] = $temp[$last]['content'][0] . $current['content'] . substr($temp[$i]['content'], 1);
                 }
                 break;
@@ -477,9 +487,9 @@ class File_ASN1
                         }
                         $content_pos += $temp['length'];
                         // all subtags should be octet strings
-                        //if ($temp['type'] != FILE_ASN1_TYPE_OCTET_STRING) {
+                        if ($temp['type'] != FILE_ASN1_TYPE_OCTET_STRING) {
-                        //    return false;
+                            return false;
-                        //}
+                        }
                         $current['content'].= $temp['content'];
                         $length+= $temp['length'];
                     }
@@ -490,12 +500,15 @@ class File_ASN1
                 break;
             case FILE_ASN1_TYPE_NULL:
                 // "The contents octets shall not contain any octets." -- paragraph 8.8.2
-                //if (strlen($content)) {
+                if ($constructed || strlen($content)) {
-                //    return false;
+                    return false;
-                //}
+                }
                 break;
             case FILE_ASN1_TYPE_SEQUENCE:
             case FILE_ASN1_TYPE_SET:
+                if (!$constructed) {
+                    return false;
+                }
                 $offset = 0;
                 $current['content'] = array();
                 $content_len = strlen($content);
@@ -516,7 +529,13 @@ class File_ASN1
                 }
                 break;
             case FILE_ASN1_TYPE_OBJECT_IDENTIFIER:
+                if ($constructed) {
+                    return false;
+                }
                 $current['content'] = $this->_decodeOID(substr($content, $content_pos));
+                if ($current['content'] === false) {
+                    return false;
+                }
                 break;
             /* Each character string type shall be encoded as if it had been declared:
                [UNIVERSAL x] IMPLICIT OCTET STRING
@@ -546,14 +565,22 @@ class File_ASN1
             case FILE_ASN1_TYPE_UTF8_STRING:
                 // ????
             case FILE_ASN1_TYPE_BMP_STRING:
+                if ($constructed) {
+                    return false;
+                }
                 $current['content'] = substr($content, $content_pos);
                 break;
             case FILE_ASN1_TYPE_UTC_TIME:
             case FILE_ASN1_TYPE_GENERALIZED_TIME:
+                if ($constructed) {
+                    return false;
+                }
                 $current['content'] = class_exists('DateTime') ?
                     $this->_decodeDateTime(substr($content, $content_pos), $tag) :
                     $this->_decodeUnixTime(substr($content, $content_pos), $tag);
+                break;
             default:
+                return false;
         }
 
         $start+= $length;
@@ -1228,6 +1255,11 @@ class File_ASN1
         $oid = array();
         $pos = 0;
         $len = strlen($content);
+
+        if (ord($content[$len - 1]) & 0x80) {
+            return false;
+        }
+
         $n = new Math_BigInteger();
         while ($pos < $len) {
             $temp = ord($content[$pos++]);

tests/Unit/Crypt/RSA/ModeTest.php

@@ -115,4 +115,26 @@ k12yS6pCS3c+1wZ9cYFVtgfpSL4XpylLe9EnRT2GRVYCqUkR4AUeTuvnAgMBAAE=
         $payload = 'eyJraWQiOiJ0RkMyVUloRnBUTV9FYTNxY09kX01xUVQxY0JCbTlrRkxTRGZlSmhzUkc4IiwiYWxnIjoiUFMyNTYifQ.eyJhcHAiOiJhY2NvdW50cG9ydGFsIiwic3ViIjoiNTliOGM4YzA5NTVhNDA5MDg2MGRmYmM3ZGQwMjVjZWEiLCJjbGlkIjoiZTQ5ZTA2N2JiMTFjNDcyMmEzNGIyYjNiOGE2YTYzNTUiLCJhbSI6InBhc3N3b3JkIiwicCI6ImVOcDFrRUZQd3pBTWhmXC9QdEVOYU5kQkc2bUZDNHNpbENNNXU0aTNXMHFSS0hFVDU5V1JzcXpZRUp4XC84M3ZQbkIxcUg3Rm5CZVNabEtNME9saGVZVUVWTXlHOEVUOEZnWDI4dkdqWG4wWkcrV2hSK01rWVBicGZacHI2U3E0N0RFYjBLYkRFT21CSUZuOTZKN1ZDaWg1Q2p4dWNRZDJmdHJlMCt2cSthZFFObUluK0poWEl0UlBvQ0xya1wvZ05VV3N3T09vSVwva0Q5ZVk4c05jRHFPUzNkanFWb3RPU21oRUo5b0hZZmFqZmpSRzFGSWpGRFwvOExtT2pKbVF3d0tBMnQ0aXJBQ2NncHo0dzBuN3BtXC84YXV2T0dFM2twVFZ2d0IzdzlQZk1YZnJJUTBhejRsaEtIdVBUMU42XC9sb1FJPSIsImlhaSI6IjU5YjhjOGMwOTU1YTQwOTA4NjBkZmJjN2RkMDI1Y2VhIiwiY2xzdmMiOiJhY2NvdW50cG9ydGFsIiwibHB2IjoxNTQ3Njc1NDM4LCJ0IjoicyIsImljIjp0cnVlLCJleHAiOjE1NDc3MDQyMzgsImlhdCI6MTU0NzY3NTQzOCwianRpIjoiZTE0N2UzM2UzNzVhNDkyNWJjMzdjZTRjMDIwMmJjNDYifQ';
         $this->assertTrue($rsa->verify($payload, $sig));
     }
+
+    public function testPKCS1SigWithoutNull()
+    {
+        $rsa = new Crypt_RSA();
+        $rsa->loadKey(array(
+            'n' => new Math_BigInteger('0xE932AC92252F585B3A80A4DD76A897C8B7652952FE788F6EC8DD640587A1EE5647670A8AD
+4C2BE0F9FA6E49C605ADF77B5174230AF7BD50E5D6D6D6D28CCF0A886A514CC72E51D209CC7
+72A52EF419F6A953F3135929588EBE9B351FCA61CED78F346FE00DBB6306E5C2A4C6DFC3779
+AF85AB417371CF34D8387B9B30AE46D7A5FF5A655B8D8455F1B94AE736989D60A6F2FD5CADB
+FFBD504C5A756A2E6BB5CECC13BCA7503F6DF8B52ACE5C410997E98809DB4DC30D943DE4E81
+2A47553DCE54844A78E36401D13F77DC650619FED88D8B3926E3D8E319C80C744779AC5D6AB
+E252896950917476ECE5E8FC27D5F053D6018D91B502C4787558A002B9283DA7', 16),
+            'e' => new Math_BigInteger('3')
+        ));
+
+        $message = 'hello world!';
+        $signature = pack('H*', 'a0073057133ff3758e7e111b4d7441f1d8cbe4b2dd5ee4316a14264290dee5ed7f175716639bd9bb43a14e4f9fcb9e84dedd35e2205caac04828b2c053f68176d971ea88534dd2eeec903043c3469fc69c206b2a8694fd262488441ed8852280c3d4994e9d42bd1d575c7024095f1a20665925c2175e089c0d731471f6cc145404edf5559fd2276e45e448086f71c78d0cc6628fad394a34e51e8c10bc39bfe09ed2f5f742cc68bee899d0a41e4c75b7b80afd1c321d89ccd9fe8197c44624d91cc935dfa48de3c201099b5b417be748aef29248527e8bbb173cab76b48478d4177b338fe1f1244e64d7d23f07add560d5ad50b68d6649a49d7bc3db686daaa7');
+
+        $rsa->setSignatureMode(CRYPT_RSA_SIGNATURE_PKCS1);
+        $rsa->setHash('sha256');
+        $this->assertTrue($rsa->verify($message, $signature));
+    }
 }

tests/Unit/File/ASN1Test.php

@@ -392,4 +392,60 @@ class Unit_File_ASN1Test extends PhpseclibTestCase
 
         $this->assertIsArray($a);
     }
+
+    public function testNullGarbage()
+    {
+        $asn1 = new File_ASN1();
+
+        $em = pack('H*', '3080305c0609608648016503040201054f8888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888804207509e5bda0c762d2bac7f90d758b5b2263fa01ccbc542ab5e3df163be08e6ca9');
+        $decoded = $asn1->decodeBER($em);
+        $this->assertFalse($decoded[0]);
+
+        $em = pack('H*', '3080307f0609608648016503040201057288888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888804207509e5bda0c762d2bac7f90d758b5b2263fa01ccbc542ab5e3df163be08e6ca90000');
+        $decoded = $asn1->decodeBER($em);
+        $this->assertFalse($decoded[0]);
+    }
+
+    public function testOIDGarbage()
+    {
+        $asn1 = new File_ASN1();
+
+        $em = pack('H*', '3080305c065860864801650304020188888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888050004207509e5bda0c762d2bac7f90d758b5b2263fa01ccbc542ab5e3df163be08e6ca9');
+        $decoded = $asn1->decodeBER($em);
+        $this->assertFalse($decoded[0]);
+
+        $em = pack('H*', '3080307f067d608648016503040201888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888804207509e5bda0c762d2bac7f90d758b5b2263fa01ccbc542ab5e3df163be08e6ca9');
+        $decoded = $asn1->decodeBER($em);
+        $this->assertFalse($decoded[0]);
+    }
+
+    public function testConstructedMismatch()
+    {
+        $asn1 = new File_ASN1();
+
+        $em = pack('H*', '1031300d0609608648016503040201050004207509e5bda0c762d2bac7f90d758b5b2263fa01ccbc542ab5e3df163be08e6ca9');
+        $decoded = $asn1->decodeBER($em);
+        $this->assertFalse($decoded[0]);
+
+        $em = pack('H*', '3031100d0609608648016503040201050004207509e5bda0c762d2bac7f90d758b5b2263fa01ccbc542ab5e3df163be08e6ca9');
+        $decoded = $asn1->decodeBER($em);
+        $this->assertFalse($decoded[0]);
+
+        $em = pack('H*', '3031300d2609608648016503040201050004207509e5bda0c762d2bac7f90d758b5b2263fa01ccbc542ab5e3df163be08e6ca9');
+        $decoded = $asn1->decodeBER($em);
+        $this->assertFalse($decoded[0]);
+
+        $em = pack('H*', '3031300d06096086480165030402012d0004207509e5bda0c762d2bac7f90d758b5b2263fa01ccbc542ab5e3df163be08e6ca9');
+        $decoded = $asn1->decodeBER($em);
+        $this->assertFalse($decoded[0]);
+    }
+
+    public function testBadTagSecondOctet()
+    {
+        $asn1 = new File_ASN1();
+
+        $em = pack('H*', '3033300f1f808080060960864801650304020104207509e5bda0c762d2bac7f90d758b5b2263fa01ccbc542ab5e3df163be08e6ca9');
+        $decoded = $asn1->decodeBER($em);
+        $this->assertFalse($decoded[0]);
+    }
 }