also zero pad DHE public key in ClientKeyExchange message for interop #12331

heinzelotto · 2020-06-30T21:16:34Z

The Windows SChannel TLS stack has problems when the DHE pub key is encoded with fewer bytes than the DHE prime.

In #1320 padding has already been added to work around this problem for the OpenSSL server case, and I experimented a little and Microsoft seems to have also fixed this in their client code.

I guess that OpenSSL server <-> windows client is the more common case, but the opposite case still causes handshakes to randomly fail when a DHE suite is selected. By adding padding for the OpenSSL client key exchange I hope to fix that.

This is my first contribution to OpenSSL, I tried to follow the style guide and conventions. Any corrections are much appreciated.

ssl/statem/statem_clnt.c

davidben

Looks reasonable to me, but I'm not as familiar with the new provider-based APIs and WPACKET, so you should get a second review from someone more actively involved with the project.

ssl/statem/statem_clnt.c

heinzelotto · 2020-07-11T11:12:28Z

@mattcaswell are you familiar with the WPACKET API and would you taking a look?

kaduk

The code looks fine to me; I just note some optional stylistic tweaks.
(Matt is on holiday at the moment, as I undestand it.)

kaduk · 2020-07-12T00:31:42Z

ssl/statem/statem_clnt.c

+    /*
+     * for interoperability with some versions of the Microsoft TLS
+     * stack, we need to zero pad the DHE pub key to the same length
+     * as the prime, so use the length of the prime here


If we have to make other changes, we could use a capital 'F' to start the comment and a period at the end, to make it a complete sentence, but it's not worth changing just to fix that.
(And the ServerKeyExchange case doesn't do either, anyway.)

kaduk · 2020-07-12T00:39:36Z

ssl/statem/statem_clnt.c

    EVP_PKEY *ckey = NULL, *skey = NULL;
    unsigned char *keybytes = NULL;
+    int np;


I'd consider a different name for this variable, maybe prime_len -- I'm not actually sure what "np" is supposed to indicate or stand for.

kaduk · 2020-07-12T00:43:02Z

I'm going to dismiss @davidben 's review, since it was before the force-push.
(In some sense it doesn't matter, since we're both committers and this needs an OTC review in order to get merged.)

invalidated by force-push

kaduk · 2020-07-12T01:10:02Z

(the travis failures are the spurious "../../_srcdir/config: no such file or directory" that we've been seeing lately)

…interop

heinzelotto · 2020-07-13T19:21:45Z

Thank you @kaduk for your comments. I wanted to ask whether there were any necessary steps on my side once marked as "approval: otc review pending"? And I figured that if I am going to do an additional round trip anyway, I will just add the changes you suggested. (Hopefully I did the fixup! correctly)

kaduk · 2020-07-17T17:13:25Z

@heinzelotto the fixup looks good, thank you.
With the "approval: otc review pending" set we are not waiting on anything else from you (unless that review produces additional feedback/requested changes). Unfortunately, OTC time is in high demand right now, with the work towards the 3.0 release, so I don't have a great estimate for you.

t8m · 2020-07-20T11:14:36Z

ssl/statem/statem_clnt.c

        SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_CKE_DHE,
                 ERR_R_INTERNAL_ERROR);
        goto err;
    }

-    BN_bn2bin(pub_key, keybytes);
+    BN_bn2binpad(DH_get0_pub_key(dh_clnt), keybytes, prime_len);


Please add check for the return value. Although it is in practice "can't happen" condition, if prime_len is lower than the length of the actual pub_key, the function will fail. You can move the call to the if condition above.

…interop

kaduk

The fixup (check BN_bn2binpad return value) looks good.
The Travis failures are all the (unrelated) 'pem_to_der_deserializer_functions' declaration issue.

openssl-machine · 2020-08-26T00:07:58Z

This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago

t8m

LGTM

openssl-machine · 2020-09-01T12:00:17Z

This pull request is ready to merge

Reviewed-by: Ben Kaduk <kaduk@mit.edu> Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from #12331)

t8m · 2020-09-01T12:29:14Z

Merged to master. Thank you for your contribution and patience when waiting for the reviews.

Reviewed-by: Ben Kaduk <kaduk@mit.edu> Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#12331)

paveljanik · 2021-02-04T20:40:37Z

This makes openssl as the client send out zero padded DHE pubkey in TLSv1.2 which means it breaks the standard as leading zeros should be stripped in TLSv1.2 (as opposite to TLSv1.3 where they should be left in the stream!). Is this really what you need/want?

davidben · 2021-02-04T21:08:20Z

The spec says the resulting secret has leading zeros removed. I believe RFC5246 is silent as to whether the public key is padded. (Which is a mistake. It should have picked one, and the correct one to pick is fixed-width. But a bit late to fix that now, so implementations have to pick one.)

paveljanik · 2021-02-04T21:13:04Z

So on the server side, you have to support (try...) both because your clients could have picked padding and not-padding? ;-)

davidben · 2021-02-04T21:14:43Z

No, just have your parser tolerate leading zeros, as OpenSSL does. The resulting secret is different because it's a KDF input.

(Alternatively, stop doing TLS 1.2 DHE as it's beyond saving.)

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Jun 30, 2020

davidben reviewed Jun 30, 2020

View reviewed changes

ssl/statem/statem_clnt.c Outdated Show resolved Hide resolved

heinzelotto closed this Jul 1, 2020

heinzelotto reopened this Jul 1, 2020

openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Jul 1, 2020

heinzelotto force-pushed the master branch 2 times, most recently from 7737456 to 4ca393b Compare July 1, 2020 18:56

heinzelotto requested a review from davidben July 2, 2020 14:45

davidben previously approved these changes Jul 6, 2020

View reviewed changes

ssl/statem/statem_clnt.c Outdated Show resolved Hide resolved

ssl/statem/statem_clnt.c Outdated Show resolved Hide resolved

also zero pad DHE public key in ClientKeyExchange message for interop

5540c67

heinzelotto force-pushed the master branch from 4ca393b to 5540c67 Compare July 11, 2020 11:06

kaduk approved these changes Jul 12, 2020

View reviewed changes

kaduk added approval: otc review pending This pull request needs review by an OTC member branch: master Merge to master branch triaged: feature The issue/pr requests/adds a feature labels Jul 12, 2020

fixup! also zero pad DHE public key in ClientKeyExchange message for …

725a899

…interop

t8m reviewed Jul 20, 2020

View reviewed changes

fixup! also zero pad DHE public key in ClientKeyExchange message for …

e9b2726

…interop

kaduk approved these changes Jul 26, 2020

View reviewed changes

t8m approved these changes Aug 31, 2020

View reviewed changes

t8m added approval: done This pull request has the required number of approvals and removed approval: otc review pending This pull request needs review by an OTC member labels Aug 31, 2020

openssl-machine removed the approval: done This pull request has the required number of approvals label Sep 1, 2020

openssl-machine added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Sep 1, 2020

openssl-machine pushed a commit that referenced this pull request Sep 1, 2020

also zero pad DHE public key in ClientKeyExchange message for interop

807b0a1

Reviewed-by: Ben Kaduk <kaduk@mit.edu> Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from #12331)

t8m closed this Sep 1, 2020

kroeckx mentioned this pull request Nov 29, 2020

boringssl tests failing #13556

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

also zero pad DHE public key in ClientKeyExchange message for interop #12331

also zero pad DHE public key in ClientKeyExchange message for interop #12331

heinzelotto commented Jun 30, 2020

davidben left a comment

heinzelotto commented Jul 11, 2020

kaduk left a comment

kaduk Jul 12, 2020

kaduk Jul 12, 2020

kaduk commented Jul 12, 2020

kaduk commented Jul 12, 2020

heinzelotto commented Jul 13, 2020

kaduk commented Jul 17, 2020

t8m Jul 20, 2020

kaduk left a comment

openssl-machine commented Aug 26, 2020

t8m left a comment

openssl-machine commented Sep 1, 2020

t8m commented Sep 1, 2020

paveljanik commented Feb 4, 2021

davidben commented Feb 4, 2021

paveljanik commented Feb 4, 2021

davidben commented Feb 4, 2021

also zero pad DHE public key in ClientKeyExchange message for interop #12331

also zero pad DHE public key in ClientKeyExchange message for interop #12331

Conversation

heinzelotto commented Jun 30, 2020

davidben left a comment

Choose a reason for hiding this comment

heinzelotto commented Jul 11, 2020

kaduk left a comment

Choose a reason for hiding this comment

kaduk Jul 12, 2020

Choose a reason for hiding this comment

kaduk Jul 12, 2020

Choose a reason for hiding this comment

kaduk commented Jul 12, 2020

kaduk commented Jul 12, 2020

heinzelotto commented Jul 13, 2020

kaduk commented Jul 17, 2020

t8m Jul 20, 2020

Choose a reason for hiding this comment

kaduk left a comment

Choose a reason for hiding this comment

openssl-machine commented Aug 26, 2020

t8m left a comment

Choose a reason for hiding this comment

openssl-machine commented Sep 1, 2020

t8m commented Sep 1, 2020

paveljanik commented Feb 4, 2021

davidben commented Feb 4, 2021

paveljanik commented Feb 4, 2021

davidben commented Feb 4, 2021