From a000cbd7dd7cdb35be6099faaa3cf6d239b32af7 Mon Sep 17 00:00:00 2001 From: Toddr Bot Date: Sat, 14 Mar 2026 22:29:13 +0000 Subject: [PATCH 1/2] fix: re-enable PKCS#1 v1.5 padding for signatures (closes #61) The Marvin attack (CVE-2023-6129) targets PKCS#1 v1.5 *decryption* padding oracles, not signatures. Version 0.35 disabled use_pkcs1_padding() entirely, breaking every CPAN module that signs with RSASSA-PKCS1-v1.5 (RS256): Net::ACME2, Google::SAML::Response, Crypt::LE, JSON::WebToken, and others. This commit: - Re-enables use_pkcs1_padding() to set RSA_PKCS1_PADDING - Respects PKCS#1 v1.5 in sign()/verify() on OpenSSL 3.x instead of forcing PSS for all non-NO_PADDING modes - Blocks PKCS#1 v1.5 for encrypt()/decrypt() with a clear error message about the Marvin attack (all OpenSSL versions) - Updates documentation and tests Co-Authored-By: Claude Opus 4.6 --- RSA.pm | 43 ++++++++++++++++++++----------------------- RSA.xs | 12 +++++++++--- t/padding.t | 33 ++++++++++++++++----------------- t/rsa.t | 11 +++++++++-- 4 files changed, 54 insertions(+), 45 deletions(-) diff --git a/RSA.pm b/RSA.pm index f7131a4..9e12017 100644 --- a/RSA.pm +++ b/RSA.pm @@ -82,12 +82,12 @@ Crypt::OpenSSL::RSA - RSA encoding and decoding, using the openSSL libraries =head1 SECURITY -Version 0.35 makes the use of PKCS#1 v1.5 padding a fatal error. It is -very difficult to implement PKCS#1 v1.5 padding securely. If you are still -using RSA in in general, you should be looking at alternative encryption -algorithms. Version 0.36 implements RSA-PSS padding (PKCS#1 v2.1) and makes -setting an invalid padding a fatal error. Note, PKCS1_OAEP can only be used -for encryption and PKCS1_PSS can only be used for signing. +Version 0.35 disabled PKCS#1 v1.5 padding entirely to mitigate the Marvin +attack. However, the Marvin attack only affects PKCS#1 v1.5 B +(padding oracle), not B. Version 0.38 re-enables +C for use with C and C, while keeping +it disabled for C and C. PKCS1_OAEP should be used +for encryption and either PKCS1_PSS or PKCS1 can be used for signing. =head1 DESCRIPTION @@ -114,8 +114,8 @@ C<-----BEGIN...-----> and C<-----END...-----> lines. The padding is set to PKCS1_OAEP, but can be changed with the C methods. -Note, PKCS1_OAEP can only be used for encryption. You must call -C prior to signing operations. +Note, PKCS1_OAEP can only be used for encryption. Call +C or C prior to signing operations. =item new_private_key @@ -244,20 +244,13 @@ Check the signature on a text. =head1 Padding Methods -Versions prior to 0.35 allowed using pkcs1 padding for both encryption -and signature operations but has been disabled for security reasons. - -While B can be used for encryption or signature operations -B is used for signature operations and +B can be used for signature operations (C and +C). PKCS#1 v1.5 encryption is disabled due to the Marvin attack. +B is the recommended replacement for signatures. B is used for encryption operations. On OpenSSL 3.x, the appropriate padding is set for each operation unless -B is called before either operation. - -B while C is the effective replacement for your -use case may require some additional steps. JSON Web Tokens (JWT) for -instance require the algorithm to be changed from "RS256" for "pkcs1" -(SHA1256) to "PS256" for "pkcs1-pss" (SHA-256 and MGF1 with SHA-256) +B or B is called before the operation. =over @@ -269,11 +262,15 @@ Encrypting user data directly with RSA is insecure. =item use_pkcs1_padding -PKCS #1 v1.5 padding has been disabled as it is nearly impossible to use this -padding method in a secure manner. It is known to be vulnerable to timing -based side channel attacks. use_pkcs1_padding() results in a fatal error. +Use C padding for B. PKCS#1 v1.5 +signatures (RSASSA-PKCS1-v1.5) are secure and widely required by protocols +such as JWT RS256, ACME (RFC 8555), and SAML. -L +B: PKCS#1 v1.5 B is disabled because it is vulnerable to +the L +(a timing side-channel on decryption padding validation). Calling +C or C with this padding will croak. Use +C for encryption. =item use_pkcs1_oaep_padding diff --git a/RSA.xs b/RSA.xs index 41bd247..01e57b4 100644 --- a/RSA.xs +++ b/RSA.xs @@ -327,6 +327,12 @@ SV* rsa_crypt(rsaData* p_rsa, SV* p_from, from = (unsigned char*) SvPV(p_from, from_length); size = EVP_PKEY_get_size(p_rsa->rsa); + + if(p_rsa->padding == RSA_PKCS1_PADDING) { + croak("PKCS#1 v1.5 padding for encryption is vulnerable to the Marvin attack. " + "Use use_pkcs1_oaep_padding() for encryption, or use_pkcs1_padding() with sign()/verify()."); + } + CHECK_NEW(to, size, UNSIGNED_CHAR); #if OPENSSL_VERSION_NUMBER >= 0x30000000L @@ -983,7 +989,7 @@ void use_pkcs1_padding(p_rsa) rsaData* p_rsa; CODE: - croak("PKCS#1 1.5 is disabled as it is known to be vulnerable to marvin attacks."); + p_rsa->padding = RSA_PKCS1_PADDING; void use_pkcs1_oaep_padding(p_rsa) @@ -1036,7 +1042,7 @@ sign(p_rsa, text_SV) THROW(ctx); THROW(EVP_PKEY_sign_init(ctx)); sign_pad = p_rsa->padding; - if (p_rsa->padding != RSA_NO_PADDING) { + if (p_rsa->padding != RSA_NO_PADDING && p_rsa->padding != RSA_PKCS1_PADDING) { sign_pad = RSA_PKCS1_PSS_PADDING; } THROW(EVP_PKEY_CTX_set_rsa_padding(ctx, sign_pad) > 0); @@ -1113,7 +1119,7 @@ PPCODE: THROW(ctx); THROW(EVP_PKEY_verify_init(ctx) == 1); verify_pad = p_rsa->padding; - if (p_rsa->padding != RSA_NO_PADDING) { + if (p_rsa->padding != RSA_NO_PADDING && p_rsa->padding != RSA_PKCS1_PADDING) { verify_pad = RSA_PKCS1_PSS_PADDING; } THROW(EVP_PKEY_CTX_set_rsa_padding(ctx, verify_pad) > 0); diff --git a/t/padding.t b/t/padding.t index c04e059..0f198f1 100644 --- a/t/padding.t +++ b/t/padding.t @@ -8,7 +8,7 @@ use Crypt::OpenSSL::Guess qw(openssl_version); my ($major, $minor, $patch) = openssl_version; BEGIN { - plan tests => 87 + ( UNIVERSAL::can( "Crypt::OpenSSL::RSA", "use_sha512_hash" ) ? 4 * 5 : 0 ); + plan tests => 123 + ( UNIVERSAL::can( "Crypt::OpenSSL::RSA", "use_sha512_hash" ) ? 4 * 5 : 0 ); } sub _Test_Encrypt_And_Decrypt { @@ -81,24 +81,23 @@ is( $rsa_priv->decrypt( $rsa_priv->encrypt($plaintext) ), $plaintext, "private k my $rsa_pub = Crypt::OpenSSL::RSA->new_public_key($public_key_string); -my @unsupported_paddings = qw/pkcs1 sslv23/; - $plaintext .= $plaintext x 5; -# pkcs1 sslv23 are unsupported methods -foreach my $pad (@unsupported_paddings) { - my $method = "use_${pad}_padding"; - SKIP: { - skip "OpenSSL version less than 3.0 supports sslv23", 1 - if $major lt '3.0' && $pad eq 'sslv23'; - eval { - $rsa->$method; - }; - ok($@, "Padding method $pad unsupported"); - } +# sslv23 is unsupported on OpenSSL 3.x +SKIP: { + skip "OpenSSL version less than 3.0 supports sslv23", 1 + if $major lt '3.0'; + eval { + $rsa->use_sslv23_padding; + }; + ok($@, "Padding method sslv23 unsupported on OpenSSL 3.x"); } -my @supported_paddings = qw/no pkcs1_pss pkcs1_oaep/; -# no pkcs1_pss pkcs1_oaep are supported methods +# pkcs1 is supported (for signatures, not encryption) +eval { $rsa->use_pkcs1_padding; }; +ok(!$@, "Padding method pkcs1 supported"); + +my @supported_paddings = qw/no pkcs1 pkcs1_pss pkcs1_oaep/; +# no pkcs1 pkcs1_pss pkcs1_oaep are supported methods foreach my $pad (@supported_paddings) { my $method = "use_${pad}_padding"; eval { @@ -113,7 +112,7 @@ my %padding_methods = ( 'no' => {'sign' => 1, 'encrypt' => 1, 'pad' => 0}, 'pkcs1_pss' => {'sign' => 1, 'encrypt' => 0, 'pad' => 1}, 'pkcs1_oaep' => {'sign' => 0, 'encrypt' => 1, 'pad' => 42}, - 'pkcs1' => {'sign' => 0, 'encrypt' => 0, 'pad' => 11}, + 'pkcs1' => {'sign' => 1, 'encrypt' => 0, 'pad' => 11}, #'sslv23' => {'sign' => 0, 'encrypt' => 0, 'pad' => 11}, ); diff --git a/t/rsa.t b/t/rsa.t index 9bfd7fc..5436caf 100644 --- a/t/rsa.t +++ b/t/rsa.t @@ -6,7 +6,7 @@ use Crypt::OpenSSL::RSA; use Crypt::OpenSSL::Guess qw(openssl_version); BEGIN { - plan tests => 67 + ( UNIVERSAL::can( "Crypt::OpenSSL::RSA", "use_sha512_hash" ) ? 4 * 5 : 0 ) + ( UNIVERSAL::can( "Crypt::OpenSSL::RSA", "use_whirlpool_hash" ) ? 1 * 5 : 0 ); + plan tests => 78 + ( UNIVERSAL::can( "Crypt::OpenSSL::RSA", "use_sha512_hash" ) ? 8 * 5 : 0 ) + ( UNIVERSAL::can( "Crypt::OpenSSL::RSA", "use_whirlpool_hash" ) ? 1 * 5 : 0 ); } sub _Test_Encrypt_And_Decrypt { @@ -122,7 +122,7 @@ _check_for_croak( $plaintext .= $plaintext x 5; -my @paddings = qw/pkcs1_oaep pkcs1_pss/; +my @paddings = qw/pkcs1 pkcs1_oaep pkcs1_pss/; foreach my $padding (@paddings) { my $p = "use_${padding}_padding"; @@ -174,6 +174,13 @@ if ( UNIVERSAL::can( "Crypt::OpenSSL::RSA", "use_whirlpool_hash" ) ) { _Test_Sign_And_Verify( $plaintext, $rsa, $rsa_pub, "whirlpool" ); } +# PKCS#1 v1.5 encryption must croak (Marvin attack) +$rsa->use_pkcs1_padding(); +_check_for_croak( + sub { $rsa->encrypt("test") }, + "Marvin attack" +); + # check subclassing eval { Crypt::OpenSSL::RSA::Subpackage->generate_key(512); }; From 790297b229b27aba3d2500cb95fcd1904d1dfe5a Mon Sep 17 00:00:00 2001 From: Toddr Bot Date: Mon, 16 Mar 2026 15:07:35 +0000 Subject: [PATCH 2/2] rebase: apply review feedback on #103 --- RSA.xs | 27 +++++++++++++++------------ t/padding.t | 2 +- 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/RSA.xs b/RSA.xs index 01e57b4..f2ee49e 100644 --- a/RSA.xs +++ b/RSA.xs @@ -311,11 +311,11 @@ EVP_PKEY* _load_rsa_key(SV* p_keyStringSv, SV* rsa_crypt(rsaData* p_rsa, SV* p_from, int (*p_crypt)(EVP_PKEY_CTX*, unsigned char*, size_t*, const unsigned char*, size_t), - int (*init_crypt)(EVP_PKEY_CTX*), int public) + int (*init_crypt)(EVP_PKEY_CTX*), int public, int is_encrypt) #else SV* rsa_crypt(rsaData* p_rsa, SV* p_from, - int (*p_crypt)(int, const unsigned char*, unsigned char*, RSA*, int)) + int (*p_crypt)(int, const unsigned char*, unsigned char*, RSA*, int), int is_encrypt) #endif { STRLEN from_length; @@ -328,7 +328,7 @@ SV* rsa_crypt(rsaData* p_rsa, SV* p_from, from = (unsigned char*) SvPV(p_from, from_length); size = EVP_PKEY_get_size(p_rsa->rsa); - if(p_rsa->padding == RSA_PKCS1_PADDING) { + if(is_encrypt && p_rsa->padding == RSA_PKCS1_PADDING) { croak("PKCS#1 v1.5 padding for encryption is vulnerable to the Marvin attack. " "Use use_pkcs1_oaep_padding() for encryption, or use_pkcs1_padding() with sign()/verify()."); } @@ -350,8 +350,11 @@ SV* rsa_crypt(rsaData* p_rsa, SV* p_from, THROW(ctx); THROW(init_crypt(ctx) == 1); + /* After the PKCS1 and PSS guards above, the only reachable padding + values here are RSA_NO_PADDING and RSA_PKCS1_OAEP_PADDING (for + encrypt/decrypt) or RSA_PKCS1_PADDING (for private_encrypt/public_decrypt). */ crypt_pad = p_rsa->padding; - if (p_rsa->padding != RSA_NO_PADDING) { + if (is_encrypt && p_rsa->padding != RSA_NO_PADDING) { crypt_pad = RSA_PKCS1_OAEP_PADDING; } THROW(EVP_PKEY_CTX_set_rsa_padding(ctx, crypt_pad) > 0); @@ -817,9 +820,9 @@ encrypt(p_rsa, p_plaintext) SV* p_plaintext; CODE: #if OPENSSL_VERSION_NUMBER >= 0x30000000L - RETVAL = rsa_crypt(p_rsa, p_plaintext, EVP_PKEY_encrypt, EVP_PKEY_encrypt_init, 1 /* public */); + RETVAL = rsa_crypt(p_rsa, p_plaintext, EVP_PKEY_encrypt, EVP_PKEY_encrypt_init, 1 /* public */, 1 /* is_encrypt */); #else - RETVAL = rsa_crypt(p_rsa, p_plaintext, RSA_public_encrypt); + RETVAL = rsa_crypt(p_rsa, p_plaintext, RSA_public_encrypt, 1 /* is_encrypt */); #endif OUTPUT: RETVAL @@ -834,9 +837,9 @@ decrypt(p_rsa, p_ciphertext) croak("Public keys cannot decrypt"); } #if OPENSSL_VERSION_NUMBER >= 0x30000000L - RETVAL = rsa_crypt(p_rsa, p_ciphertext, EVP_PKEY_decrypt, EVP_PKEY_decrypt_init, 0 /* private */); + RETVAL = rsa_crypt(p_rsa, p_ciphertext, EVP_PKEY_decrypt, EVP_PKEY_decrypt_init, 0 /* private */, 1 /* is_encrypt */); #else - RETVAL = rsa_crypt(p_rsa, p_ciphertext, RSA_private_decrypt); + RETVAL = rsa_crypt(p_rsa, p_ciphertext, RSA_private_decrypt, 1 /* is_encrypt */); #endif OUTPUT: RETVAL @@ -851,9 +854,9 @@ private_encrypt(p_rsa, p_plaintext) croak("Public keys cannot private_encrypt"); } #if OPENSSL_VERSION_NUMBER >= 0x30000000L - RETVAL = rsa_crypt(p_rsa, p_plaintext, EVP_PKEY_sign, EVP_PKEY_sign_init, 0 /* private */); + RETVAL = rsa_crypt(p_rsa, p_plaintext, EVP_PKEY_sign, EVP_PKEY_sign_init, 0 /* private */, 0 /* is_encrypt */); #else - RETVAL = rsa_crypt(p_rsa, p_plaintext, RSA_private_encrypt); + RETVAL = rsa_crypt(p_rsa, p_plaintext, RSA_private_encrypt, 0 /* is_encrypt */); #endif OUTPUT: RETVAL @@ -864,9 +867,9 @@ public_decrypt(p_rsa, p_ciphertext) SV* p_ciphertext; CODE: #if OPENSSL_VERSION_NUMBER >= 0x30000000L - RETVAL = rsa_crypt(p_rsa, p_ciphertext, EVP_PKEY_verify_recover, EVP_PKEY_verify_recover_init, 1 /*public */); + RETVAL = rsa_crypt(p_rsa, p_ciphertext, EVP_PKEY_verify_recover, EVP_PKEY_verify_recover_init, 1 /*public */, 0 /* is_encrypt */); #else - RETVAL = rsa_crypt(p_rsa, p_ciphertext, RSA_public_decrypt); + RETVAL = rsa_crypt(p_rsa, p_ciphertext, RSA_public_decrypt, 0 /* is_encrypt */); #endif OUTPUT: RETVAL diff --git a/t/padding.t b/t/padding.t index 0f198f1..c11f5c3 100644 --- a/t/padding.t +++ b/t/padding.t @@ -112,7 +112,7 @@ my %padding_methods = ( 'no' => {'sign' => 1, 'encrypt' => 1, 'pad' => 0}, 'pkcs1_pss' => {'sign' => 1, 'encrypt' => 0, 'pad' => 1}, 'pkcs1_oaep' => {'sign' => 0, 'encrypt' => 1, 'pad' => 42}, - 'pkcs1' => {'sign' => 1, 'encrypt' => 0, 'pad' => 11}, + 'pkcs1' => {'sign' => 1, 'encrypt' => 0, 'pad' => 11}, # pad value only affects plaintext length; sign() hashes input so value is arbitrary (must be non-zero) #'sslv23' => {'sign' => 0, 'encrypt' => 0, 'pad' => 11}, );