8000 Merge pull request #1635 from terrafrost/moosa-1.0 · phpseclib/phpseclib@05550b9 · GitHub
[go: up one dir, main page]
Skip to content

Commit 05550b9

Browse files
terrafrostterrafrost
authored
Merge pull request #1635 from terrafrost/moosa-1.0
cleanup RSA PKCS#1 v1.5 signature verification (CVE-2021-30130)
2 parents 2d6705d + 149b4d2 commit 05550b9

File tree

4 files changed

+187
-20
lines changed

4 files changed

+187
-20
lines changed

‎phpseclib/Crypt/RSA.php

+59-2
Original file line numberDiff line numberDiff line change
@@ -3001,6 +3001,59 @@ function _emsa_pkcs1_v1_5_encode($m, $emLen)
30013001
return $em;
30023002
}
30033003

3004+
/**
3005+
* EMSA-PKCS1-V1_5-ENCODE (without NULL)
3006+
*
3007+
* Quoting https://tools.ietf.org/html/rfc8017#page-65,
3008+
*
3009+
* "The parameters field associated with id-sha1, id-sha224, id-sha256,
3010+
* id-sha384, id-sha512, id-sha512/224, and id-sha512/256 should
3011+
* generally be omitted, but if present, it shall have a value of type
3012+
* NULL"
3013+
*
3014+
* @access private
3015+
* @param string $m
3016+
* @param int $emLen
3017+
* @return string
3018+
*/
3019+
function _emsa_pkcs1_v1_5_encode_without_null($m, $emLen)
3020+
{
3021+
$h = $this->hash->hash($m);
3022+
if ($h === false) {
3023+
return false;
3024+
}
3025+
3026+
switch ($this->hashName) {
3027+
case 'sha1':
3028+
$t = pack('H*', '301f300706052b0e03021a0414');
3029+
break;
3030+
case 'sha256':
3031+
$t = pack('H*', '302f300b06096086480165030402010420');
3032+
break;
3033+
case 'sha384':
3034+
$t = pack('H*', '303f300b06096086480165030402020430');
3035+
break;
3036+
case 'sha512':
3037+
$t = pack('H*', '304f300b06096086480165030402030440');
3038+
break;
3039+
default:
3040+
return false;
3041+
}
3042+
$t.= $h;
3043+
$tLen = strlen($t);
3044+
3045+
if ($emLen < $tLen + 11) {
3046+
user_error('Intended encoded message length too short');
3047+
return false;
3048+
}
3049+
3050+
$ps = str_repeat(chr(0xFF), $emLen - $tLen - 3);
3051+
3052+
$em = "\0\1$ps\0$t";
3053+
3054+
return $em;
3055+
}
3056+
30043057
/**
30053058
* RSASSA-PKCS1-V1_5-SIGN
30063059
*
@@ -3067,13 +3120,17 @@ function _rsassa_pkcs1_v1_5_verify($m, $s)
30673120
// EMSA-PKCS1-v1_5 encoding
30683121

30693122
$em2 = $this->_emsa_pkcs1_v1_5_encode($m, $this->k);
3070-
if ($em2 === false) {
3123+
$em3 = $this->_emsa_pkcs1_v1_5_encode_without_null($m, $this->k);
3124+
3125+
if ($em2 === false && $em3 === false) {
30713126
user_error('RSA modulus too short');
30723127
return false;
30733128
}
30743129

30753130
// Compare
3076-
return $this->_equals($em, $em2);
3131+
3132+
return ($em2 !== false && $this->_equals($em, $em2)) ||
3133+
($em3 !== false && $this->_equals($em, $em3));
30773134
}
30783135

30793136
/**

‎phpseclib/File/ASN1.php

+50-18
Original file line numberDiff line numberDiff line change
@@ -317,7 +317,7 @@ function _decode_ber($encoded, $start = 0, $encoded_pos = 0)
317317
$current = array('start' => $start);
318318

319319
$type = ord($encoded[$encoded_pos++]);
320-
$start++;
320+
$startOffset = 1;
321321

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

@@ -327,13 +327,20 @@ function _decode_ber($encoded, $start = 0, $encoded_pos = 0)
327327
// process septets (since the eighth bit is ignored, it's not an octet)
328328
do {
329329
$temp = ord($encoded[$encoded_pos++]);
330+
$startOffset++;
330331
$loop = $temp >> 7;
331332
$tag <<= 7;
332-
$tag |= $temp & 0x7F;
333-
$start++;
333+
$temp &= 0x7F;
334+
// "bits 7 to 1 of the first subsequent octet shall not all be zero"
335+
if ($startOffset == 2 && $temp == 0) {
336+
return false;
337+
}
338+
$tag |= $temp;
334339
} while ($loop);
335340
}
336341

342+
$start+= $startOffset;
343+
337344
// Length, as discussed in paragraph 8.1.3 of X.690-0207.pdf#page=13
338345
$length = ord($encoded[$encoded_pos++]);
339346
$start++;
@@ -426,13 +433,16 @@ function _decode_ber($encoded, $start = 0, $encoded_pos = 0)
426433
switch ($tag) {
427434
case FILE_ASN1_TYPE_BOOLEAN:
428435
// "The contents octets shall consist of a single octet." -- paragraph 8.2.1
429-
//if (strlen($content) != 1) {
430-
// return false;
431-
//}
436+
if ($constructed || strlen($content) != 1) {
437+
return false;
438+
}
432439
$current['content'] = (bool) ord($content[$content_pos]);
433440
break;
434441
case FILE_ASN1_TYPE_INTEGER:
435442
case FILE_ASN1_TYPE_ENUMERATED:
443+
if ($constructed) {
444+
return false;
445+
}
436446
$current['content'] = new Math_BigInteger(substr($content, $content_pos), -256);
437447
break;
438448
case FILE_ASN1_TYPE_REAL: // not currently supported
@@ -452,15 +462,15 @@ function _decode_ber($encoded, $start = 0, $encoded_pos = 0)
452462
$last = count($temp) - 1;
453463
for ($i = 0; $i < $last; $i++) {
454464
// all subtags should be bit strings
455-
//if ($temp[$i]['type'] != FILE_ASN1_TYPE_BIT_STRING) {
456-
// return false;
457-
//}
465+
if ($temp[$i]['type'] != FILE_ASN1_TYPE_BIT_STRING) {
466+
return false;
467+
}
458468
$current['content'].= substr($temp[$i]['content'], 1);
459469
}
460470
// all subtags should be bit strings
461-
//if ($temp[$last]['type'] != FILE_ASN1_TYPE_BIT_STRING) {
462-
// return false;
463-
//}
471+
if ($temp[$last]['type'] != FILE_ASN1_TYPE_BIT_STRING) {
472+
return false;
473+
}
464474
$current['content'] = $temp[$last]['content'][0] . $current['content'] . substr($temp[$i]['content'], 1);
465475
}
466476
break;
@@ -477,9 +487,9 @@ function _decode_ber($encoded, $start = 0, $encoded_pos = 0)
477487
}
478488
$content_pos += $temp['length'];
479489
// all subtags should be octet strings
480-
//if ($temp['type'] != FILE_ASN1_TYPE_OCTET_STRING) {
481-
// return false;
482-
//}
490+
if ($temp['type'] != FILE_ASN1_TYPE_OCTET_STRING) {
491+
return false;
492+
}
483493
$current['content'].= $temp['content'];
484494
$length+= $temp['length'];
485495
}
@@ -490,12 +500,15 @@ function _decode_ber($encoded, $start = 0, $encoded_pos = 0)
490500
break;
491501
case FILE_ASN1_TYPE_NULL:
492502
// "The contents octets shall not contain any octets." -- paragraph 8.8.2
493-
//if (strlen($content)) {
494-
// return false;
495-
//}
503+
if ($constructed || strlen($content)) {
504+
return false;
505+
}
496506
break;
497507
case FILE_ASN1_TYPE_SEQUENCE:
498508
case FILE_ASN1_TYPE_SET:
509+
if (!$constructed) {
510+
return false;
511+
}
499512
$offset = 0;
500513
$current['content'] = array();
501514
$content_len = strlen($content);
@@ -516,7 +529,13 @@ function _decode_ber($encoded, $start = 0, $encoded_pos = 0)
516529
}
517530
break;
518531
case FILE_ASN1_TYPE_OBJECT_IDENTIFIER:
532+
if ($constructed) {
533+
return false;
534+
}
519535
$current['content'] = $this->_decodeOID(substr($content, $content_pos));
536+
if ($current['content'] === false) {
537+
return false;
538+
}
520539
break;
521540
/* Each character string type shall be encoded as if it had been declared:
522541
[UNIVERSAL x] IMPLICIT OCTET STRING
@@ -546,14 +565,22 @@ function _decode_ber($encoded, $start = 0, $encoded_pos = 0)
546565
case FILE_ASN1_TYPE_UTF8_STRING:
547566
// ????
548567
case FILE_ASN1_TYPE_BMP_STRING:
568+
if ($constructed) {
569+
return false;
570+
}
549571
$current['content'] = substr($content, $content_pos);
550572
break;
551573
case FILE_ASN1_TYPE_UTC_TIME:
552574
case FILE_ASN1_TYPE_GENERALIZED_TIME:
575+
if ($constructed) {
576+
return false;
577+
}
553578
$current['content'] = class_exists('DateTime') ?
554579
$this->_decodeDateTime(substr($content, $content_pos), $tag) :
555580
$this->_decodeUnixTime(substr($content, $content_pos), $tag);
581+
break;
556582
default:
583+
return false;
557584
}
558585

559586
$start+= $length;
@@ -1228,6 +1255,11 @@ function _decodeOID($content)
12281255
$oid = array();
12291256
$pos = 0;
12301257
$len = strlen($content);
1258+
1259+
if (ord($content[$len - 1]) & 0x80) {
1260+
return false;
1261+
}
1262+
12311263
$n = new Math_BigInteger();
12321264
while ($pos < $len) {
12331265
$temp = ord($content[$pos++]);

‎tests/Unit/Crypt/RSA/ModeTest.php

+22
Original file line numberDiff line numberDiff line change
@@ -115,4 +115,26 @@ public function testPSSSigsWithNonPowerOf2Key()
115115
$payload = 'eyJraWQiOiJ0RkMyVUloRnBUTV9FYTNxY09kX01xUVQxY0JCbTlrRkxTRGZlSmhzUkc4IiwiYWxnIjoiUFMyNTYifQ.eyJhcHAiOiJhY2NvdW50cG9ydGFsIiwic3ViIjoiNTliOGM4YzA5NTVhNDA5MDg2MGRmYmM3ZGQwMjVjZWEiLCJjbGlkIjoiZTQ5ZTA2N2JiMTFjNDcyMmEzNGIyYjNiOGE2YTYzNTUiLCJhbSI6InBhc3N3b3JkIiwicCI6ImVOcDFrRUZQd3pBTWhmXC9QdEVOYU5kQkc2bUZDNHNpbENNNXU0aTNXMHFSS0hFVDU5V1JzcXpZRUp4XC84M3ZQbkIxcUg3Rm5CZVNabEtNME9saGVZVUVWTXlHOEVUOEZnWDI4dkdqWG4wWkcrV2hSK01rWVBicGZacHI2U3E0N0RFYjBLYkRFT21CSUZuOTZKN1ZDaWg1Q2p4dWNRZDJmdHJlMCt2cSthZFFObUluK0poWEl0UlBvQ0xya1wvZ05VV3N3T09vSVwva0Q5ZVk4c05jRHFPUzNkanFWb3RPU21oRUo5b0hZZmFqZmpSRzFGSWpGRFwvOExtT2pKbVF3d0tBMnQ0aXJBQ2NncHo0dzBuN3BtXC84YXV2T0dFM2twVFZ2d0IzdzlQZk1YZnJJUTBhejRsaEtIdVBUMU42XC9sb1FJPSIsImlhaSI6IjU5YjhjOGMwOTU1YTQwOTA4NjBkZmJjN2RkMDI1Y2VhIiwiY2xzdmMiOiJhY2NvdW50cG9ydGFsIiwibHB2IjoxNTQ3Njc1NDM4LCJ0IjoicyIsImljIjp0cnVlLCJleHAiOjE1NDc3MDQyMzgsImlhdCI6MTU0NzY3NTQzOCwianRpIjoiZTE0N2UzM2UzNzVhNDkyNWJjMzdjZTRjMDIwMmJjNDYifQ';
116116
$this->assertTrue($rsa->verify($payload, $sig));
117117
}
118+
119+
public function testPKCS1SigWithoutNull()
120+
{
121+
$rsa = new Crypt_RSA();
122+
$rsa->loadKey(array(
123+
'n' => new Math_BigInteger('0xE932AC92252F585B3A80A4DD76A897C8B7652952FE788F6EC8DD640587A1EE5647670A8AD
124+
4C2BE0F9FA6E49C605ADF77B5174230AF7BD50E5D6D6D6D28CCF0A886A514CC72E51D209CC7
125+
72A52EF419F6A953F3135929588EBE9B351FCA61CED78F346FE00DBB6306E5C2A4C6DFC3779
126+
AF85AB417371CF34D8387B9B30AE46D7A5FF5A655B8D8455F1B94AE736989D60A6F2F 10000 D5CADB
127+
FFBD504C5A756A2E6BB5CECC13BCA7503F6DF8B52ACE5C410997E98809DB4DC30D943DE4E81
128+
2A47553DCE54844A78E36401D13F77DC650619FED88D8B3926E3D8E319C80C744779AC5D6AB
129+
E252896950917476ECE5E8FC27D5F053D6018D91B502C4787558A002B9283DA7', 16),
130+
'e' => new Math_BigInteger('3')
131+
));
132+
133+
$message = 'hello world!';
134+
$signature = pack('H*', 'a0073057133ff3758e7e111b4d7441f1d8cbe4b2dd5ee4316a14264290dee5ed7f175716639bd9bb43a14e4f9fcb9e84dedd35e2205caac04828b2c053f68176d971ea88534dd2eeec903043c3469fc69c206b2a8694fd262488441ed8852280c3d4994e9d42bd1d575c7024095f1a20665925c2175e089c0d731471f6cc145404edf5559fd2276e45e448086f71c78d0cc6628fad394a34e51e8c10bc39bfe09ed2f5f742cc68bee899d0a41e4c75b7b80afd1c321d89ccd9fe8197c44624d91cc935dfa48de3c201099b5b417be748aef29248527e8bbb173cab76b48478d4177b338fe1f1244e64d7d23f07add560d5ad50b68d6649a49d7bc3db686daaa7');
135+
136+
$rsa->setSignatureMode(CRYPT_RSA_SIGNATURE_PKCS1);
137+
$rsa->setHash('sha256');
138+
$this->assertTrue($rsa->verify($message, $signature));
139+
}
118140
}

‎tests/Unit/File/ASN1Test.php

+56
Original file line numberDiff line numberDiff line change
@@ -392,4 +392,60 @@ public function testExplicitImplicitDate()
392392

393393
$this->assertIsArray($a);
394394
}
395+
396+
public function testNullGarbage()
397+
{
398+
$asn1 = new File_ASN1();
399+
400+
$em = pack('H*', '3080305c0609608648016503040201054f8888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888804207509e5bda0c762d2bac7f90d758b5b2263fa01ccbc542ab5e3df163be08e6ca9');
401+
$decoded = $asn1->decodeBER($em);
402+
$this->assertFalse($decoded[0]);
403+
404+
$em = pack('H*', '3080307f0609608648016503040201057288888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888804207509e5bda0c762d2bac7f90d758b5b2263fa01ccbc542ab5e3df163be08e6ca90000');
405+
$decoded = $asn1->decodeBER($em);
406+
$this->assertFalse($decoded[0]);
407+
}
408+
409+
public function testOIDGarbage()
410+
{
411+
$asn1 = new File_ASN1();
412+
413+
$em = pack('H*', '3080305c065860864801650304020188888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888050004207509e5bda0c762d2bac7f90d758b5b2263fa01ccbc542ab5e3df163be08e6ca9');
414+
$decoded = $asn1->decodeBER($em);
415+
$this->assertFalse($decoded[0]);
416+
417+
$em = pack('H*', '3080307f067d608648016503040201888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888888804207509e5bda0c762d2bac7f90d758b5b2263fa01ccbc542ab5e3df163be08e6ca9');
418+
$decoded = $asn1->decodeBER($em);
419+
$this->assertFalse($decoded[0]);
420+
}
421+
422+
public function testConstructedMismatch()
423+
{
424+
$asn1 = new File_ASN1();
425+
426+
$em = pack('H*', '1031300d0609608648016503040201050004207509e5bda0c762d2bac7f90d758b5b2263fa01ccbc542ab5e3df163be08e6ca9');
427+
$decoded = $asn1->decodeBER($em);
428+
$this->assertFalse($decoded[0]);
429+
430+
$em = pack('H*', '3031100d0609608648016503040201050004207509e5bda0c762d2bac7f90d758b5b2263fa01ccbc542ab5e3df163be08e6ca9');
431+
$decoded = $asn1->decodeBER($em);
432+
$this->assertFalse($decoded[0]);
433+
434+
$em = pack('H*', '3031300d2609608648016503040201050004207509e5bda0c762d2bac7f90d758b5b2263fa01ccbc542ab5e3df163be08e6ca9');
435+
$decoded = $asn1->decodeBER($em);
436+
$this->assertFalse($decoded[0]);
437+
438+
$em = pack('H*', '3031300d06096086480165030402012d0004207509e5bda0c762d2bac7f90d758b5b2263fa01ccbc542ab5e3df163be08e6ca9');
439+
$decoded = $asn1->decodeBER($em);
440+
$this->assertFalse($decoded[0]);
441+
}
442+
443+
public function testBadTagSecondOctet()
444+
{
445+
$asn1 = new File_ASN1();
446+
447+
$em = pack('H*', '3033300f1f808080060960864801650304020104207509e5bda0c762d2bac7f90d758b5b2263fa01ccbc542ab5e3df163be08e6ca9');
448+
$decoded = $asn1->decodeBER($em);
449+
$this->assertFalse($decoded[0]);
450+
}
395451
}

0 commit comments

Comments
 (0)
0