Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 16 commits into from Apr 6, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
b90c332
ASN1: don't allow last octet in OID to have MSB set
terrafrost Apr 2, 2021
95f597c
ASN1: tweaks to tag decoding
terrafrost Apr 2, 2021
a589442
ASN1: uncomment out extra validation code
terrafrost Apr 3, 2021
dbd210c
Tests/ASN1: add test for null garbage bytes
terrafrost Apr 3, 2021
6be326e
Tests/ASN1: add test for OID garbage bytes
terrafrost Apr 3, 2021
8b8cbec
ASN1: make sure constructed bit is what it ought to be
terrafrost Apr 3, 2021
c1d21ed
Tests/ASN1: add tests for constructed mismatches
terrafrost Apr 3, 2021
10cee45
Tests/ASN1: test for bad tag
terrafrost Apr 3, 2021
e02c845
ASN1: fail when encountering unsupported tags
terrafrost Apr 3, 2021
9af9d69
RSA: support "strict" PKCS1 sigs without NULLs
terrafrost Apr 3, 2021
7a5846c
Test/RSA: add test for "strict" PKCS1 sigs without NULLs
terrafrost Apr 3, 2021
4a3e08c
RSA: tweak to new validation method
terrafrost Apr 3, 2021
5fca2bc
Tests/RSA: test requires sha256
terrafrost Apr 3, 2021
581fbdb
CS adjustments
terrafrost Apr 3, 2021
8af4280
RSA: misc fixes for 'without NULL' PKCS1 signature validation
terrafrost Apr 3, 2021
149b4d2
RSA: fix for PHP8
terrafrost Apr 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
61 changes: 59 additions & 2 deletions phpseclib/Crypt/RSA.php
View file
Open in desktop
Expand Up @@ -3001,6 +3001,59 @@ function _emsa_pkcs1_v1_5_encode($m, $emLen)
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
*
Expand Down Expand Up @@ -3067,13 +3120,17 @@ function _rsassa_pkcs1_v1_5_verify($m, $s)
// 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));
}

/**
Expand Down
68 changes: 50 additions & 18 deletions phpseclib/File/ASN1.php
View file
Open in desktop
Expand Up @@ -317,7 +317,7 @@ function _decode_ber($encoded, $start = 0, $encoded_pos = 0)
$current = array('start' => $start);

$type = ord($encoded[$encoded_pos++]);
$start++;
$startOffset = 1;

$constructed = ($type >> 5) & 1;

Expand All @@ -327,13 +327,20 @@ function _decode_ber($encoded, $start = 0, $encoded_pos = 0)
// 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;
$start++;
$temp &= 0x7F;
// "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++;
Expand Down Expand Up @@ -426,13 +433,16 @@ function _decode_ber($encoded, $start = 0, $encoded_pos = 0)
switch ($tag) {
case FILE_ASN1_TYPE_BOOLEAN:
// "The contents octets shall consist of a single octet." -- paragraph 8.2.1
//if (strlen($content) != 1) {
// return false;
//}
if ($constructed || strlen($content) != 1) {
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
Expand All @@ -452,15 +462,15 @@ function _decode_ber($encoded, $start = 0, $encoded_pos = 0)
$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) {
// return false;
//}
if ($temp[$i]['type'] != FILE_ASN1_TYPE_BIT_STRING) {
return false;
}
$current['content'].= substr($temp[$i]['content'], 1);
}
// all subtags should be bit strings
//if ($temp[$last]['type'] != FILE_ASN1_TYPE_BIT_STRING) {
// return false;
//}
if ($temp[$last]['type'] != FILE_ASN1_TYPE_BIT_STRING) {
return false;
}
$current['content'] = $temp[$last]['content'][0] . $current['content'] . substr($temp[$i]['content'], 1);
}
break;
Expand All @@ -477,9 +487,9 @@ function _decode_ber($encoded, $start = 0, $encoded_pos = 0)
}
$content_pos += $temp['length'];
// all subtags should be octet strings
//if ($temp['type'] != FILE_ASN1_TYPE_OCTET_STRING) {
// return false;
//}
if ($temp['type'] != FILE_ASN1_TYPE_OCTET_STRING) {
return false;
}
$current['content'].= $temp['content'];
$length+= $temp['length'];
}
Expand All @@ -490,12 +500,15 @@ function _decode_ber($encoded, $start = 0, $encoded_pos = 0)
break;
case FILE_ASN1_TYPE_NULL:
// "The contents octets shall not contain any octets." -- paragraph 8.8.2
//if (strlen($content)) {
// return false;
//}
if ($constructed || strlen($content)) {
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);
Expand All @@ -516,7 +529,13 @@ function _decode_ber($encoded, $start = 0, $encoded_pos = 0)
}
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
Expand Down Expand Up @@ -546,14 +565,22 @@ function _decode_ber($encoded, $start = 0, $encoded_pos = 0)
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;
Expand Down Expand Up @@ -1228,6 +1255,11 @@ function _decodeOID($content)
$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++]);
Expand Down
22 changes: 22 additions & 0 deletions tests/Unit/Crypt/RSA/ModeTest.php
View file
Open in desktop
Expand Up @@ -115,4 +115,26 @@ public function testPSSSigsWithNonPowerOf2Key()
$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));
}
}
56 changes: 56 additions & 0 deletions tests/Unit/File/ASN1Test.php
View file
Open in desktop
Expand Up @@ -392,4 +392,60 @@ public function testExplicitImplicitDate()

$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]);
}
}