fix: re-enable PKCS#1 v1.5 padding for signatures

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<decryption>
+(padding oracle), not B<signatures>.  Version 0.38 re-enables
+C<use_pkcs1_padding()> for use with C<sign()> and C<verify()>, while keeping
+it disabled for C<encrypt()> and C<decrypt()>.  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<use_xxx_padding> methods.
 
-Note, PKCS1_OAEP can only be used for encryption.  You must call
-C<use_pkcs1_pss_padding> prior to signing operations.
+Note, PKCS1_OAEP can only be used for encryption.  Call
+C<use_pkcs1_pss_padding> or C<use_pkcs1_padding> 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<use_no_padding> can be used for encryption or signature operations
-B<use_pkcs1_pss_padding> is used for signature operations and
+B<use_pkcs1_padding> can be used for signature operations (C<sign()> and
+C<verify()>).  PKCS#1 v1.5 encryption is disabled due to the Marvin attack.
+B<use_pkcs1_pss_padding> is the recommended replacement for signatures.
 B<use_pkcs1_oaep_padding> is used for encryption operations.
 
 On OpenSSL 3.x, the appropriate padding is set for each operation unless
-B<use_no_padding> is called before either operation.
-
-B<Note:> while C<pkcs1-pss> is the effective replacement for <pkcs1> 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<use_no_padding> or B<use_pkcs1_padding> 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<PKCS #1 v1.5> padding for B<signature operations only>.  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<Marvin Attack|https://github.com/tomato42/marvin-toolkit/blob/master/README.md>
+B<Note>: PKCS#1 v1.5 B<encryption> is disabled because it is vulnerable to
+the L<Marvin Attack|https://github.com/tomato42/marvin-toolkit/blob/master/README.md>
+(a timing side-channel on decryption padding validation).  Calling
+C<encrypt()> or C<decrypt()> with this padding will croak.  Use
+C<use_pkcs1_oaep_padding()> for encryption.
 
 =item use_pkcs1_oaep_padding
 

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;
@@ -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(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().");
+    }
+
     CHECK_NEW(to, size, UNSIGNED_CHAR);
 #if OPENSSL_VERSION_NUMBER >= 0x30000000L
 
@@ -344,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);
@@ -811,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
@@ -828,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
@@ -845,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
@@ -858,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
@@ -983,7 +992,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 +1045,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 +1122,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);

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}, # pad value only affects plaintext length; sign() hashes input so value is arbitrary (must be non-zero)
                        #'sslv23'      => {'sign' => 0, 'encrypt' => 0, 'pad' => 11},
                     );
 

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