feat: RSA_PKCS1_WITH_TLS_PADDING padding developped in the provider #392

sebastienandert · 2024-05-14T13:33:29Z

Hi,

I work in a company and for these 3 last months, we have worked on the embedding of the opensslv3 functions in our application.
The aim of these implementation is to access to our HSM (Eviden) via opensslv3 functions in PKCS11 using your provider.

Unfortunately, a test failed due to a function not implemented in your provider.

Missing function in the provider

When we connect an openssl s_client to an openssl s_server using these 2 commands :
./openssl s_server -port 4456 -provider pkcs11 -key "pkcs11:object=nom;id=1122334455-000000000000000000000000-98" -cert /int1/pki/store/user/crt/C2P_HSM_20261205.crt
and
./openssl s_client -connect 127.0.0.1:4456 -CApath /int1/pki/store/ac/crt_rehash -cipher AES128-GCM-SHA256

We get the following error coming from Openssl libraries :

-----------------------------------------------------------------
Using default temp DH parameters
ACCEPT
ERROR
0011F0435A7F0000:error:1C8000A5:Provider routines:p11prov_rsaenc_set_ctx_params:illegal or unsupported padding mode:asymmetric_cipher.c:491:
0011F0435A7F0000:error:0A000093:SSL routines:tls_process_cke_rsa:decryption failed:ssl/statem/statem_srvr.c:2946:
CONNECTION CLOSED

In the openssl sources, this error comes from the function : 

openssl-3.0.11/ssl/statem/statem_srvr.c:tls_process_cke_rsa:2909
    if (EVP_PKEY_decrypt_init(ctx) <= 0
            || EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_WITH_TLS_PADDING) <= 0) {
        SLfatal(s, SSL_AD_DECRYPT_ERROR, SSL_R_DECRYPTION_FAILED);
        goto err;
    }
-----------------------------------------------------------------

After inverstigating on this issue, it appears that ciphers using RSA key exchange type involves to use the padding : RSA_PKCS1_WITH_TLS_PADDING, which is not supported by your provider.
hence this error raises by your provider :

-----------------------------------------------------------------
0011F0435A7F0000:error:1C8000A5:Provider routines:p11prov_rsaenc_set_ctx_params:illegal or unsupported padding mode:asymmetric_cipher.c:491:

if (mechtype == CK_UNAVAILABLE_INFORMATION) {
    P11PROV_debug("mechtype == CK_UNAVAILABLE_INFORMATION");
    ERR_raise(ERR_LIB_PROV, PROV_R_ILLEGAL_OR_UNSUPPORTED_PADDING_MODE);
    return RET_OSSL_ERR;
}
-----------------------------------------------------------------

Then, we have implemented this unsupported padding (mandatory for our needs) in order to continue using you provider.

It's now 1 month our improved provider runs under our UAT environment without error anymore.
Of course we would like to share you our implementation and to know if you could integrate it in your sources in a near futur ?

Thanks,
Kind regards,

Signed-off-by: Sebastien ANDERT <sebastien.andert@worldline.com>

simo5

Thanks for this PR, there is a lot to adjust, especially the actual core of the function is completely side-channel unsafe and will require careful rewrite.

Please avoid churn unnecessary to implement the actual feature when submitting a PR, it just makes it harded to review.

(Also I will not accept regressing from basic initialization features, if you need to support an old compiler that can't deal with initializers, then please keep that as a local patch to your environment for now)

simo5 · 2024-05-14T13:58:56Z

src/asymmetric_cipher.c

+
+    if (want == 0)
+        return;
+    fd = open ("/dev/urandom", O_RDONLY, 0);


OpenSSL provid its own random functions, please use those.

You should access them via the provider interface not calling the high level RAND_bytes() function call, though.

If you do not know how to do that let me know and I will provide you an example by fixing #280

simo5 · 2024-05-14T13:59:23Z

src/asymmetric_cipher.c

+    }
+}
+
+static void p11prov_rsaenc_unpad_rand (unsigned char *to, size_t want)


please use uint8_t * if you want bytes

simo5 · 2024-05-14T14:01:02Z

src/asymmetric_cipher.c

+     * So, remain controls are about length and starting bytes
+     */
+    cond  = constant_equal (ret, CKR_OK);
+    if (cond) {


This is clearly not constant time, and defeats everything that follows

simo5 · 2024-05-14T14:02:43Z

src/asymmetric_cipher.c

+        /* Failure somewhere
+         * locally compute a random value of *datalen long
+         */
+        p11prov_rsaenc_unpad_rand (data, computedlen);


this needs to be called uncondtionally before checking the return value, otherwise you will create another side-channel timing signal again.

Then the buffer need to be swapped in constant time based on the value of cond

simo5 · 2024-05-14T14:28:55Z

src/asymmetric_cipher.c

+            pOut     = locOut;
+        } else {
+            pOut     = out;
+        }


Sorry but I do not get why you are doing this.

This interface needs to be called with enough space for the key size, at the top when called with out == NULL we return exactly size = p11prov_obj_get_key_size(encctx->key); as the amount of memory to call us with.

That value is store in outsize though, not out_size which is the receiving variable that will be returned via *outlen.

What we need to do is just check that outsize is big enough if out != NULL and return an error there if it is not with ERR_raise(ERR_LIB_PROV, PROV_R_BAD_LENGTH); as the error to put on the stack.

Now I see that upstream they return a much shorter length if prsactx->pad_mode == RSA_PKCS1_WITH_TLS_PADDING .. if tat is needed for opensl compatibility reasons, then please check for that specific case only at the start of the function (once we verify outsize is at least SSL_MAX_MASTER_KEY_LENGTH long), and malloc a buffer of p11prov_obj_get_key_size(encctx->key) size before continuing.

All of this stuff needs to be behind pad_mode == RSA_PKCS1_WITH_TLS_PADDING checks.

simo5 · 2024-05-14T15:13:30Z

src/interface.c

-            "C_GetInterface() not available. Falling back to "
-            "C_GetFunctionList(): %s",
-            err);
+        err = dlerror();


this change does not belong in this PR, please revert it.

this comment applies to all the changes in this file

simo5 · 2024-05-14T15:14:09Z

src/interface.c

    CK_RV ret;

    if (!mctx) {
        return CKR_GENERAL_ERROR;
    }

+    memset (&args, 0, sizeof (args));
+    memset (&ck_info, 0, sizeof (ck_info));
    ret = MUTEX_LOCK(mctx);


don't do this the above initializations are just fine

simo5 · 2024-05-14T15:15:15Z

src/kdf.c

@@ -349,7 +349,11 @@ static int p11prov_hkdf_get_ctx_params(void *ctx, OSSL_PARAM *params)
    if (p) {
        size_t ret_size = 0;
        if (hkdfctx->params.bExpand != CK_FALSE) {
-            ret_size = SIZE_MAX;
+#ifndef SIZE_MAX


If you do not have SIZE_MAX define it in a header, not in here.
What platform lacks this in a header?

simo5 · 2024-05-14T15:16:47Z

src/objects.c

@@ -991,7 +991,8 @@ CK_RV p11prov_obj_find(P11PROV_CTX *provctx, P11PROV_SESSION *session,
    P11PROV_debug("Find objects [class=%lu, id-len=%lu, label=%s]", class,
                  id.ulValueLen,
                  label.type == CKA_LABEL ? (char *)label.pValue : "None");
-
+
+    memset (template, 0, sizeof (template));


again no, the original initialization is just fine.

I won't repeat it again for each occurence, but please revert all of these changes

simo5 · 2024-05-14T15:17:54Z

src/provider.c

+     */
+    return ;
+}
+


Please remove this

tomato42 · 2024-05-30T16:51:32Z

@sebastienandert wrote:

It's now 1 month our improved provider runs under our UAT environment without error anymore.

there's a subtle difference between "it works" and "it's secure": https://people.redhat.com/~hkario/marvin/
this code is almost certainly vulnerable to side-channel attacks even if the HSM performs the private key operations in constant time (and that's an "if" in itself)

simo5 · 2024-05-31T16:58:23Z

Given I saw no answers to the review, and the proposed change required major overhauls pretty much everywhere I went ahead and implemented it diffrently in PR #402

Closing this one, thanks for the contribution.

feat: RSA_PKCS1_WITH_TLS_PADDING padding developped in the provider

eb3f02a

Signed-off-by: Sebastien ANDERT <sebastien.andert@worldline.com>

simo5 requested changes May 14, 2024

View reviewed changes

simo5 mentioned this pull request May 31, 2024

Add support for legacy RSA_PKCS1_WITH_TLS_PADDING #402

Merged

10 tasks

simo5 closed this May 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: RSA_PKCS1_WITH_TLS_PADDING padding developped in the provider #392

feat: RSA_PKCS1_WITH_TLS_PADDING padding developped in the provider #392

sebastienandert commented May 14, 2024

simo5 left a comment

simo5 May 14, 2024

simo5 May 14, 2024

simo5 May 14, 2024

simo5 May 14, 2024

simo5 May 14, 2024

simo5 May 14, 2024

simo5 May 14, 2024

simo5 May 14, 2024

simo5 May 14, 2024

simo5 May 14, 2024

simo5 May 14, 2024

simo5 May 14, 2024

tomato42 commented May 30, 2024

simo5 commented May 31, 2024

feat: RSA_PKCS1_WITH_TLS_PADDING padding developped in the provider #392

feat: RSA_PKCS1_WITH_TLS_PADDING padding developped in the provider #392

Conversation

sebastienandert commented May 14, 2024

Missing function in the provider

simo5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomato42 commented May 30, 2024

simo5 commented May 31, 2024