fixes for #13767 #13785

baentsch · 2021-01-05T16:04:48Z

Implements TLS1.3 ToDos (group name retrieval) and fixes incorrect EC output reference in apps/lib/s_cb.c
Eliminates incorrect OPENSSL_NO_EC and OPENSSL_NO_DH code guards for group-accessor functions and data structures

Checklist

documentation is added or updated
tests are added or updated

t8m · 2021-01-05T16:20:28Z

There is no real commit message. The Fixes #... should be on a separate line in commit message body, the commit header line should summarize the change with commit message body giving more details.

Also please unless absolutely necessary you should use a real e-mail address for commits - and it needs to be the address that you submitted with the ICLA. Otherwise the cla-check will fail.

mattcaswell · 2021-01-05T16:21:25Z

Close/reopen to kick CLA bot

mattcaswell · 2021-01-05T16:22:06Z

I added the anonymous github email address as an alias so that the CLA check passes.

baentsch · 2021-01-05T16:54:12Z

There is no real commit message. The Fixes #... should be on a separate line in commit message body, the commit header line should summarize the change with commit message body giving more details.

Also please unless absolutely necessary you should use a real e-mail address for commits - and it needs to be the address that you submitted with the ICLA. Otherwise the cla-check will fail.

Thanks for the guidance. Looks like I'm already relying too much on the git UI and less on its command line features. Will remember the next time. I also changed user.email to a less anonymous one now.

t8m · 2021-01-05T17:21:19Z

To fix the commit message you can use git commit --amend command. Then you just have to do git push --force.

baentsch · 2021-01-06T06:59:15Z

To fix the commit message you can use git commit --amend command. Then you just have to do git push --force.

Thanks. So done.

romen · 2021-01-06T09:19:30Z

I agree with @kaduk comment about separating the different (related) concerns in different commits: you could edit your git history with an interactive rebase and then force-push to this same branch if you are willing to do so.

This way the history of feedback given in this PR is not lost!

Also, thanks again for working on this issue!

t8m · 2021-01-06T09:39:34Z

@romen Your comment seems misplaced from another PR? I do not even see @kaduk commenting here.

romen · 2021-01-06T09:43:18Z

There was discussion in the related issue: #13767 (comment)

t8m

Good work. I have a few nits and one concern about the SSL_groupid_to_name definition.

t8m · 2021-01-06T09:41:54Z

apps/lib/s_cb.c

Should be || instead?

Nope: If we have a name we want to show that (instead of the group ID).

Then it should simply be if (gname == NULL).

That would then change the output (i.e., externally visible behaviour) of the function compared to before: If the returned NID does not have the TLSEXT_nid_unknown bit set (and no name has been found), it would simply (unbeknownst to the user) output the NID instead of the group_id. Until now, this case became visible by receiving the output (nil). I don't know whether this can happen (having no group name and TLSEXT_nid_unknown unset), but the additional guard made sense to me from a safety perspective. But I'd surely follow your judgement in this regard, so will change if I don't get additional feedback.

There is simply no reason to output nil in such case. It does not provide any useful information.

It only helped me pinpoint quickly missing support for non-built-in groups; but as I'm all for changing something that wasn't sensible, if (gname == NULL) it will be.

SSL_groupid_to_name should work and give a sensible name for all outputs from SSL_get1_groups and similar functions. Therefore it should be able to handle just a nid, or a group id with the TLSEXT_nid_unknown bit set. So I don't think this code printing out the hex is necessary. Either gname is non-null in which case we print it, or alternatively it is NULL in which case an error has occurred.

ACK. Test removed. Error (NULL) will become printed.

t8m · 2021-01-06T09:42:09Z

apps/lib/s_cb.c

Same as above.

see above: Very much simplified.

t8m · 2021-01-06T09:44:23Z

test/sslapitest.c

Please wrap the line.

Oops - will do - clearly didn't read https://www.openssl.org/policies/codingstyle.html in time :-(

As noted above this only works (by accident) if SSL_get_shared_group happens to return a group with the TLSEXT_nid_unknown bit set. Otherwise it will return NULL.

I think we should have another test of this function that checks that a "built-in" group does the right thing.

So added (a set of them, actually).

t8m · 2021-01-06T09:46:10Z

include/openssl/ssl.h.in

Please add space after char

And remove the space after *, i..e.

const char *SSL_groupid_to_name(SSL *s, int group_id);

t8m · 2021-01-06T09:46:30Z

ssl/s3_lib.c

Please add space after char

t8m · 2021-01-06T09:56:22Z

doc/man3/SSL_groupid_to_name.pod

This is my biggest concern about this PR. This returns the algorithm name which is actually not that interesting by itself. We want the real group name to be presented. I think either the tlsname or realname of the group would be more useful.

It should always be the tlsname IMO.

t8m · 2021-01-06T09:57:02Z

doc/man3/SSL_groupid_to_name.pod

t8m · 2021-01-06T09:57:52Z

doc/man3/SSL_groupid_to_name.pod

I<group_id>
However it would be also better to say, what the group_id number actually is. That it is the TLS group ID.

I don't think that is the correct semantics for this function. It should accept the nid, or the group id with the TLSEXT_nid_unknown bit set - but not the plain group id. That way it can be used to print the name on the results of functions like SSL_get_shared_group. For this reason I would call the function SSL_group_to_name() and not SSL_groupid_to_name().

t8m · 2021-01-06T09:58:17Z

doc/man3/SSL_groupid_to_name.pod

Please no B<> around NULL.

baentsch · 2021-01-06T10:28:32Z

you could edit your git history with an interactive rebase and then force-push to this same branch if you are willing to do so.

Well, then I must admit I'm apparently not sufficiently educated in rebasing: I thought one can use rebasing only to "fiddle with" separate commits if they already exist, but not retroactively create new "piece-part" commits after having done everything in one go. Or is the idea to git reset --hard the branch, add piece-part commits until we arrive at the same state again (heeding the feedback above), and then push --force again? Or do I misunderstand you @romen?

romen · 2021-01-06T10:34:03Z

Yes, it is basically the same thing with just one commit, I guess I am mostly using the interactive rebase as that way it is easier to retain commit messages and to abort and go back to original state if I make some mistake.
You can use whatever strategy you are more confident with!

mattcaswell · 2021-01-06T15:01:46Z

apps/lib/s_cb.c

This step should not be necessary. If we its a supported group, then we should know the name. A gname of NULL would indicate some error in that case.

mattcaswell · 2021-01-06T15:05:49Z

apps/lib/s_cb.c

SSL_groupid_to_name should work and give a sensible name for all outputs from SSL_get1_groups and similar functions. Therefore it should be able to handle just a nid, or a group id with the TLSEXT_nid_unknown bit set. So I don't think this code printing out the hex is necessary. Either gname is non-null in which case we print it, or alternatively it is NULL in which case an error has occurred.

mattcaswell · 2021-01-06T15:06:49Z

doc/man3/SSL_groupid_to_name.pod

It should always be the tlsname IMO.

mattcaswell · 2021-01-06T15:08:26Z

doc/man3/SSL_groupid_to_name.pod

I don't think that is the correct semantics for this function. It should accept the nid, or the group id with the TLSEXT_nid_unknown bit set - but not the plain group id. That way it can be used to print the name on the results of functions like SSL_get_shared_group. For this reason I would call the function SSL_group_to_name() and not SSL_groupid_to_name().

mattcaswell · 2021-01-06T15:09:35Z

doc/man3/SSL_groupid_to_name.pod

mattcaswell · 2021-01-06T15:10:08Z

include/openssl/ssl.h.in

And remove the space after *, i..e.

const char *SSL_groupid_to_name(SSL *s, int group_id);

mattcaswell · 2021-01-06T15:15:27Z

ssl/s3_lib.c

This works by accident. tls1_group_id_lookup takes a uint16_t as its second parameter. Here you are doing an implicit cast from int to uint16_t. That means if the input value happens to have the TLSEXT_nid_unknown bit set then that bit gets dropped and it works. This is why your test works. But it won't work for real nids - which it should do.

I think we should introduce a new internal function tls1_nid2group which works a bit like tls1_nid2group_id but returns a TLS_GROUP_INFO * and is able to handle the TLSEXT_nid_unknown semantics.

Agreed. Please check new code as to whether it correctly captures the above. I refrained from adding another function as tls1_nid2group_id could be utilized suitably.

Addressed your correct comments with the update - in a slightly different way (not adding a new function, but using an existing one).

A final "etiquette" question: Please let me know whether I should click "Resolve conversation" if I think I followed through or whether the person raising the objection will do that after reviewing my comments/code changes.

Feel free to click "resolve" if you believe that you've fully fixed an issue. Rest assured that a reviewer will "unresolve" if they disagree :-)

A final "etiquette" question: Please let me know whether I should click "Resolve conversation" if I think I followed through or whether the person raising the objection will do that after reviewing my comments/code changes.

I don't think we have a codified practice on this, my personal rule of thumb is that, as the PR author, I tend to click "Resolve conversation" myself only in the trivial ones (e.g. "there is a missing space here", "typo there") after writing a "Done/Fixed/Addressed" comment.
Instead, I leave it to whomever started the discussion for the other cases, especially if I'd like to have some explicit confirmation that my fix fully addressed the issue as intended by the reviewer.

But again, this is just my rule of thumb.

mattcaswell · 2021-01-06T15:24:52Z

test/sslapitest.c

As noted above this only works (by accident) if SSL_get_shared_group happens to return a group with the TLSEXT_nid_unknown bit set. Otherwise it will return NULL.

I think we should have another test of this function that checks that a "built-in" group does the right thing.

Function SSL_group_to_name() added, together with documentation and tests. This now permits displaying names of internal and external provider-implemented groups. Partial fix of openssl#13767

mattcaswell

LGTM. Just a few minor nits remaining.

ssl/s3_lib.c

test/sslapitest.c

mattcaswell

Approved subject to the nit below being fixed. Needs a second review.

mattcaswell · 2021-01-07T15:18:59Z

test/sslapitest.c

-    if (!TEST_true(!strcmp(group_name, SSL_group_to_name(serverssl,
-                                         SSL_get_shared_group(serverssl, 0)))))
+    if (!TEST_str_eq(group_name, SSL_group_to_name(serverssl,
+                                         SSL_get_shared_group(serverssl, 0))))


Indentation looks a little odd here

Yeah, better split the line after the group_name. However this would mean with proper indent that the second line will be still slightly longer than 80 chars. But I think that should be acceptable.

Done -- indeed slightly above the 80... but if you'd think that's OK, indentation certainly is more logical now.

romen

LGTM, thanks!

t8m

LGTM now. I'll mark as approval done and leave it up to @mattcaswell to clear it if he thinks the indentation/line split needs to be still improved.

romen · 2021-01-07T16:27:36Z

Also, whoever merges this, please remember to squash the commits together!

@baentsch one nice way I learned about while working on openssl, to keep the "addressing feedback" commits separated but still have the commits squashed when we will merge is: git commit --fixup=<commit_ref> (you can also add -e if you still want to edit the commit message to include some notes, but they are going o be discarded when rebasing with git rebase --autosquash upon merge).

mattcaswell · 2021-01-07T16:30:57Z

LGTM now. I'll mark as approval done and leave it up to @mattcaswell to clear it if he thinks the indentation/line split needs to be still improved.

My approval stands

openssl-machine · 2021-01-08T17:00:17Z

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

mattcaswell · 2021-01-08T17:27:46Z

Pushed! Thanks for your contribution.

Function SSL_group_to_name() added, together with documentation and tests. This now permits displaying names of internal and external provider-implemented groups. Partial fix of #13767 Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #13785)

kaduk · 2021-01-08T21:52:57Z

Also, whoever merges this, please remember to squash the commits together!

@baentsch one nice way I learned about while working on openssl, to keep the "addressing feedback" commits separated but still have the commits squashed when we will merge is: git commit --fixup=<commit_ref> (you can also add -e if you still want to edit the commit message to include some notes, but they are going o be discarded when rebasing with git rebase --autosquash upon merge).

@romen There's also git commit --squash=<commit_ref> that lets you write commit messages that will not be combined, not discarded, with git rebase --autosquash.

@bbbrumley

* Update copyright years of auto-generated headers (make update) Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com> (Merged from openssl#13764) * crypto/win: Don't use disallowed APIs on UWP CreateFiber and ConvertThreadToFiber are not allowed in Windows Store (Universal Windows Platform) apps since they have been replaced by their Ex variants which have a new dwFlags parameter. This flag allows the fiber to do floating-point arithmetic in the fiber on x86, which would silently cause corruption otherwise since the floating-point state is not switched by default. Switch to these "new" APIs which were added in Vista. See: https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createfiberex#parameters Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from openssl#12400) * win-onecore: Build with /APPCONTAINER for UWP compat When targeting the win-onecore configuration, we must link with /APPCONTAINER which is a requirement for submitting apps to the Windows Store. Without this, the Windows App Certificate Kit will reject the app: https://docs.microsoft.com/en-us/cpp/build/reference/appcontainer-windows-store-app Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from openssl#12400) * EVP_SIGNATURE-ED25519.pod: fix typo in algo name CLA: trivial Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13768) * 28-seclevel.cnf.in: fix typo in algo name CLA: trivial Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13768) * Updated SSL_CTX_new doc Fixes openssl#13703 Reviewed-by: Ben Kaduk <kaduk@mit.edu> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from openssl#13741) * Use CRIOGET to fetch a crypto descriptor when present. FreeBSD's current /dev/crypto implementation requires that consumers clone a separate file descriptor via the CRIOGET ioctl that can then be used with other ioctls such as CIOCGSESSION. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Ben Kaduk <kaduk@mit.edu> (Merged from openssl#13468) * Support session information on FreeBSD. FreeBSD's /dev/crypto does not provide a CIOCGSESSINFO ioctl, but it does provide other ioctls that can be used to provide similar functionality. First, FreeBSD's /dev/crypto defines a CIOCGESSION2 ioctl which accepts a 'struct session2_op'. This structure extends 'struct session_op' with a 'crid' member which can be used to either request an individual driver by id, or a class of drivers via flags. To determine if the available drivers for a given algorithm are accelerated or not, use CIOCGESSION2 to first attempt to create an accelerated (hardware) session. If that fails, fall back to attempting a software session. In addition, when requesting a new cipher session, use the current setting of the 'use_softdrivers' flag to determine the value assigned to 'crid' when invoking CIOCGSESSION2. Finally, use the returned 'crid' value from CIOCGSESSION2 to look up the name of the associated driver via the CIOCFINDDEV ioctl. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Ben Kaduk <kaduk@mit.edu> (Merged from openssl#13468) * Mac M1 setting change proposal. Running tests takes very long with the current setting while it takes a lot shorter time with this change. Reviewed-by: Ben Kaduk <kaduk@mit.edu> Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13771) * Only perform special TLS handling if TLS has been configured Skip over special TLS steps for stream ciphers if we haven't been configured for TLS. Fixes openssl#12528 Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> Reviewed-by: Ben Kaduk <kaduk@mit.edu> (Merged from openssl#13774) * Update copyright year Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com> (Merged from openssl#13800) * Prepare for release of 3.0 alpha 10 Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com> * Prepare for 3.0 alpha 11 Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com> * Fix set_ciphersuites ignore unknown ciphers. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#12100) * Add a CHANGES entry for ignore unknown ciphers in set_ciphersuites. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#12100) * Fixed error and return code. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#12100) * Remove extra space. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#12100) * Ensure DTLS free functions can handle NULL Our free functions should be able to deal with the case where the object being freed is NULL. This turns out to not be quite the case for DTLS related objects. Fixes openssl#13649 Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13655) * [crypto/dh] side channel hardening for computing DH shared keys Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com> Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13783) * Adding TLS group name retrieval Function SSL_group_to_name() added, together with documentation and tests. This now permits displaying names of internal and external provider-implemented groups. Partial fix of openssl#13767 Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from openssl#13785) * [test] Add `pkey -check` validation tests Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13359) * [apps/pkey] Return error on failed `-[pub]check` Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13359) * [test][pkey_check] Add invalid SM2 key test SM2 private keys have different validation requirements than EC keys: this test checks one corner case highlighted in openssl#8435 As @bbbrumley mentioned in openssl#8435 (comment) this only fixes the absence of a regression test for validation of this kind of boundary issues for decoded SM2 keys. Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13359) * Add SM2 private key range validation According to the relevant standards, the valid range for SM2 private keys is [1, n-1), where n is the order of the curve generator. For this reason we cannot reuse the EC validation function as it is, and we introduce a new internal function `sm2_key_private_check()`. Partially fixes openssl#8435 Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13359) * [test][pkey_check] Add more invalid SM2 key tests Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13359) * replace 'unsigned const char' with 'const unsigned char' The openssl code base has only a few occurrences of 'unsigned const char' (15 occurrences), compared to the more common 'const unsigned char' (4420 occurrences). While the former is not illegal C, mixing the 'const' keyword (a 'type qualifier') in between 'unsigned' and 'char' (both 'type specifiers') is a bit odd. The background for writing this patch is not to be pedantic, but because the 'opmock' program (used to mock headers for unit tests) does not accept the 'unsigned const char' construct. While this definitely is a bug in opmock or one of its dependencies, openssl is the only piece of software we are using in combination with opmock that has this construct. CLA: trivial Reviewed-by: Nicola Tuveri <nic.tuv@gmail.com> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com> (Merged from openssl#13722) * Fix simpledynamic test compilation when condigured without DSO support. This fixes this compilation error: In file included from test/simpledynamic.c:13: test/simpledynamic.h:39:35: error: unknown type name 'SD' 39 | int sd_load(const char *filename, SD *sd, int type); | ^~ test/simpledynamic.h:40:12: error: unknown type name 'SD' 40 | int sd_sym(SD sd, const char *symname, SD_SYM *sym); | ^~ test/simpledynamic.h:40:40: error: unknown type name 'SD_SYM' 40 | int sd_sym(SD sd, const char *symname, SD_SYM *sym); | ^~~~~~ test/simpledynamic.h:41:14: error: unknown type name 'SD' 41 | int sd_close(SD lib); | ^~ make[1]: *** [Makefile:24670: test/moduleloadtest-bin-simpledynamic.o] Error 1 make[1]: *** Waiting for unfinished jobs.... In file included from test/moduleloadtest.c:19: test/simpledynamic.h:39:35: error: unknown type name 'SD' 39 | int sd_load(const char *filename, SD *sd, int type); | ^~ test/simpledynamic.h:40:12: error: unknown type name 'SD' 40 | int sd_sym(SD sd, const char *symname, SD_SYM *sym); | ^~ test/simpledynamic.h:40:40: error: unknown type name 'SD_SYM' 40 | int sd_sym(SD sd, const char *symname, SD_SYM *sym); | ^~~~~~ test/simpledynamic.h:41:14: error: unknown type name 'SD' 41 | int sd_close(SD lib); | ^~ Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from openssl#13802) * Fix for negative return value from `SSL_CTX_sess_accept()` Fixes openssl#13183 From the original issue report, before this commit, on master and on 1.1.1, the issue can be detected with the following steps: - Start with a default SSL_CTX, initiate a TLS 1.3 connection with SNI, "Accept" count of default context gets incremented - After servername lookup, "Accept" count of default context gets decremented and that of SNI context is incremented - Server sends a "Hello Retry Request" - Client sends the second "Client Hello", now again "Accept" count of default context is decremented. Hence giving a negative value. This commit fixes it by adding a check on `s->hello_retry_request` in addition to `SSL_IS_FIRST_HANDSHAKE(s)`, to ensure the counter is moved only on the first ClientHello. CLA: trivial Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from openssl#13297) * doc/man7/provider.pod: updates providers to use EVP_MD_free() and EVP_CIPHER_free() instead of EVP_MD_meth_free() and EVP_CIPHER_meth_free() respectively which are used mostly by the engine (legacy) code. Signed-off-by: Sahana Prasad <sahana@redhat.com> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> (Merged from openssl#13814) * apps.c: Fix crash in case uri arg of IS_HTTP or IS_HTTPS is NULL Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13712) * apps/pkey.c: Make clear that -passout is not supported for DER output Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13712) * apps/pkey.c: Re-order help output and option documentation Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13712) * apps/pkey.c: Forther improve user guidance, also on non-sensical option combinations Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13712) * APPS: Fix confusion between program and app/command name used in diagnostic/help output Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13799) * APPS: Print help also on -h and --h; print high-level help when no cmd given Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13799) * Close /dev/crypto file descriptor after CRIOGET ioctl(). Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Ben Kaduk <kaduk@mit.edu> (Merged from openssl#13807) * v3_ocsp.c: fix indentation of include directives Fixes openssl#13820 Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from openssl#13822) * Configure: Check all SOURCE declarations, to ensure consistency If the given sources are GENERATEd, we check those generators as well. This ensures that the declarations in the diverse build.info files are consistent with existing files. Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13824) * Configure: clean away perl syntax faults The faults aren't fatal (i.e. perl just shrugs), but are curious. Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13824) * Configure: warn about duplicate GENERATE declarations in build.info files This sort of duplication is permitted, as the end result will be a single item anyway, but we might as well warn to avoid future confusion. Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13824) * Remove duplicate GENERATE declarations for .pod files Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13824) * Use centralized fetching errors We've spread around FETCH_FAILED errors in quite a few places, and that gives somewhat crude error records, as there's no way to tell if the error was unavailable algorithms or some other error at such high levels. As an alternative, we take recording of these kinds of errors down to the fetching functions, which are in a much better place to tell what kind of error it was, thereby relieving the higher level calls from having to guess. Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13467) * Clean away extraneous library specific FETCH_FAILED reason codes Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13467) * Add X509_NAME_hash_ex() to be able to check if it failed due to unsupported SHA1 Deprecate X509_NAME_hash() Document X509_NAME_hash_ex(), X509_NAME_hash(), X509_{subject,issuer}_name_hash() Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13762) * TEST: move cert, key, and CSR loading aux functions to new testutil/load.c Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13762) * Make PEM_X509_INFO_read_bio_ex() conservative on the error queue Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13762) * x509_vfy.c: Fix a regression in find_issuer() ...in case the candidate issuer cert is identical to the target cert. This is the v3.0.0 variant of openssl#13749 fixing openssl#13739 for v1.1.1. Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13762) * Fix enable-weak-ssl-ciphers Commit e260bee broke the enable-weak-ssl-ciphers option. The stitched rc4-hmac-md5 cipher implementation did not recognise the tls_version parameter, and therefore was being incorrectly handled. Fixes openssl#13795 Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> Reviewed-by: Ben Kaduk <kaduk@mit.edu> (Merged from openssl#13803) * Fix incorrect use of BN_CTX API In some edge cases BN_CTX_end was being called without first calling BN_CTX_start. This creates a situation where the state of the big number allocator is corrupted and may lead to crashes. Fixes openssl#13812 Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13813) * d2i_X509(): Make deallocation behavior consistent with d2i_X509_AUX() Partly fixes openssl#13754 Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13755) * X509_cmp(): Fix comparison in case x509v3_cache_extensions() failed to due to invalid cert This is the upstream fix for openssl#13698 reported for v1.1.1 Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13755) * apps/{req,x509,ca}.c Make sure certs have SKID and AKID X.509 extensions by default Fixes openssl#13603 Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13658) * APPS: Allow OPENSSL_CONF to be empty, not loading a config file Also document the function CONF_get1_default_config_file() Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13658) * apps/req.c: add -CA and -CAkey options; improve code and doc Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13658) * Add tests for (non-)default SKID and AKID inclusion by apps/{req,x509,ca}.c Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13658) * apps/lib/opt.c: Fix error message on unknown option/digest Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13658) * X509_PUBKEY_set(): Fix error reporting Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13658) * apps/req.c: make -subj work with -x509; clean up related code Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13658) * Add X509V3_set_issuer_pkey, needed for AKID of self-issued not self-signed cert Also clean up some related auxiliary functions and documentation Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13658) * apps/req.c: Add -copy_extensions option for use with -x509; default: none Fixes openssl#13708 Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13658) * crypto/x509: Rename v3_{skey,skid}.c, v3_{akey,akid}.c, v3_{alt,san}.c Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13658) * apps/req.c: Cosmetic improvements of code and documentation Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13658) * apps/req.c: Make sure -verify option takes effect also with -x509 Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13658) * x509v3.h.in: Deprecate CTX_TEST and replace it by X509V3_CTX_TEST Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13658) * chacha20: Properly reinitialize the cipher context with NULL key Same for chacha20-poly1305. The test_cipher_reinit and test_cipher_reinit_partialupdate is modified to test this case of cipher context reinitialization. Fixes openssl#13064 Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl#13850) * Make the OSSL_PARAM manual conform with man-pages(7) Details from man-pages(7) that are used: Formatting conventions for manual pages describing functions ... Variable names should, like argument names, be specified in italics. ... Formatting conventions (general) ... Special macros, which are usually in uppercase, are in bold. Exception: don't boldface NULL. ... Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from openssl#13848) * Make the OSSL_SELF_TEST manual conform with man-pages(7) Details from man-pages(7) that are used: Formatting conventions for manual pages describing functions ... Variable names should, like argument names, be specified in italics. ... Formatting conventions (general) ... Special macros, which are usually in uppercase, are in bold. Exception: don't boldface NULL. ... Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13849) * Make the OSSL_HTTP manual conform with man-pages(7) Details from man-pages(7) that are used: Formatting conventions for manual pages describing functions ... Variable names should, like argument names, be specified in italics. ... Formatting conventions (general) ... Special macros, which are usually in uppercase, are in bold. Exception: don't boldface NULL. ... Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13847) * Make the OSSL_PROVIDER manual conform with man-pages(7) Details from man-pages(7) that are used: Formatting conventions for manual pages describing functions ... Variable names should, like argument names, be specified in italics. ... Formatting conventions (general) ... Special macros, which are usually in uppercase, are in bold. Exception: don't boldface NULL. ... Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13845) * Make the OSSL_trace manual conform with man-pages(7) Details from man-pages(7) that are used: Formatting conventions for manual pages describing functions ... Variable names should, like argument names, be specified in italics. ... Formatting conventions (general) ... Special macros, which are usually in uppercase, are in bold. Exception: don't boldface NULL. ... Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13842) * Make header references conform with man-pages(7) in all manuals Details from man-pages(7) that are used: Formatting conventions (general) ... Filenames (whether pathnames, or references to header files) are always in italics (e.g., <stdio.h>), except in the SYNOPSIS section, where in‐ cluded files are in bold (e.g., #include <stdio.h>). When referring to a standard header file include, specify the header file surrounded by angle brackets, in the usual C way (e.g., <stdio.h>). ... Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13843) * OPENSSL_cpuid_setup FreeBSD PowerPC update Reviewed-by: Ben Kaduk <kaduk@mit.edu> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from openssl#13821) * OPENSSL_cpuid_setup FreeBSD arm update. when possible using the getauxval equivalent which has similar ids as Linux, instead of bad instructions catch approach. Reviewed-by: Ben Kaduk <kaduk@mit.edu> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from openssl#13650) * Skip BOM when reading the config file Fixes openssl#13840 Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl#13857) * Make the OSSL_CMP manual conform with man-pages(7) Details from man-pages(7) that are used: Formatting conventions for manual pages describing functions ... Variable names should, like argument names, be specified in italics. ... Formatting conventions (general) ... Special macros, which are usually in uppercase, are in bold. Exception: don't boldface NULL. ... Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13846) * find_issuer(): When returning an expired issuer, take the most recently expired one Also point out in the documenting comment that a non-expired issuer is preferred. Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13805) * Fix a crash with multi-threaded applications using the FIPS module The FIPS implementation of the ossl_ctx_thread_stop function needs to use an OSSL_LIB_CTX - but gets passed a provctx as an argument. It was assuming that these are the same thing (which was true at one point during development) - but that is no longer the case. The fix is to get the OSSL_LIB_CTX out of the provctx. Fixes openssl#13469 Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13660) * Add a test for performing work in multiple concurrent threads We test both the default provider and the fips provider Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13660) * Document the core_thread_start upcall The core_thread_start upcall previously had a placeholder in the docs. Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13660) * Lock the provider operation_bits The provider operation_bits array can see concurrent access by multiple threads and can be reallocated at any time. Therefore we need to ensure that it is appropriately locked. Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13660) * Make sure we take the ctx->lock in ossl_lib_ctx_generic_new() The function ossl_lib_ctx_generic_new() modifies the exdata. This may be simultaneously being modified by other threads and therefore we need to make sure we take the lock before doing so. Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13660) * Enable locking on the primary DRBG when we create it The primary DRBG may be shared across multiple threads and therefore we must use locking to access it. Previously we were enabling that locking lazily when we attempted to obtain one of the child DRBGs. Part of the process of enabling the lock, is to create the lock. But if we create the lock lazily then it is too late - we may race with other threads where each thread is independently attempting to enable the locking. This results in multiple locks being created - only one of which "sticks" and the rest are leaked. Instead we enable locking on the primary when we first create it. This is already locked and therefore we cannot race. Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13660) * Extend the threads test to add simple fetch from multi threads Issue openssl#13682 suggests that doing a simple fetch from multi-threads may result in issues so we add a test for that. Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13660) * Fix an issue in provider_activate_fallbacks() The above function was running while holding the store lock with a read lock. Unfortunately it actually modifies the store, so a write lock is required instead. Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13660) * Fix a failure where fetches can return NULL in multi-threaded code When a fetch is attempted simultaneously from multiple threads then both threads can attempt to construct the method. However only one of those will get added to the global evp method store. The one that "lost" the race to add the method to the global evp method store ended up with the fetch call returning NULL, instead of returning the method that was already available. Fixes openssl#13682 Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13660) * Enhance default provider documentation Bring Wiki and man page documentation in line regarding default provider fall-back behaviour. Fixes openssl#13844 Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from openssl#13859) * Correct typo in rsa_oaep.c Reviewed-by: Kurt Roeckx <kurt@roeckx.be> Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13861) * Remove unused DRBG tests. The DRBG known answer tests are performed by evp_test and the old vectors are not used. Reviewed-by: Shane Lontis <shane.lontis@oracle.com> (Merged from openssl#13867) * Document openssl thread-safety Also discuss reference-counting, mutability and safety. Thanks to David Benjamin for pointing to comment text he added to boringSSL's header files. Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13788) * Fix crypto/des/build.info !$disabled{mdc2} was used to determine if DES files should be included in providers/liblegacy.a. Use !$disabled{des} instead. Fixes openssl#13865 Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from openssl#13866) * Fix incomplete deprecation guard in test/sslapitest.c OPENSSL_NO_DEPRECATED_3_0 should be used rather than OPENSSL_NO_DEPRECATED, as the latter doesn't take the configuration option '--api=' in account. Fixes openssl#13865 Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from openssl#13866) * Allow EVP_PKEY private key objects to be created without a public component Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from openssl#13855) * X509V3_EXT_CRL_add_nconf(): Fix mem leak on error and simplify it Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#13713) * bio_lib.c: Fix error queue entries and return codes on NULL args etc. Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#13713) * replace all BIO_R_NULL_PARAMETER by ERR_R_PASSED_NULL_PARAMETER Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#13713) * Update SERVER_HELLO_MAX_LENGTH Update constant to maximum permitted by RFC 8446 Fixes openssl#13868 Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from openssl#13874) * Rename EVP_CIPHER_CTX_get_iv and EVP_CIPHER_CTX_get_iv_state for clarity To clarify the purpose of these two calls rename them to EVP_CIPHER_CTX_get_original_iv and EVP_CIPHER_CTX_get_updated_iv. Also rename the OSSL_CIPHER_PARAM_IV_STATE to OSSL_CIPHER_PARAM_UPDATED_IV to better align with the function name. Fixes openssl#13411 Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from openssl#13870) * CMS: Fix NULL access if d2i_CMS_bio() is not passed a CMS_ContentInfo**. Fixes openssl#13624 Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#13668) * Fix PKCS7 potential segfault As the code that handles libctx, propq for PKCS7 is very similar to CMS code, a similiar fix for issue openssl#13624 needs to be applied. Reviewed-by: Tim Hudson <tjh@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#13668) * Fix memory leak in mac_newctx() on error Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#13702) * DOCS: Fix the last few remaining pass phrase options references There were a few lingering older style references to the pass phrase options section, now streamlined with all the others. Fixes openssl#13883 Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from openssl#13885) * Deprecate OCSP_xxx API for OSSL_HTTP_xxx Deprecations made: OCSP_REQ_CTX typedef->OSSL_HTTP_REQ_CTX OCSP_REQ_CTX_new->OSSL_HTTP_REQ_CTX_new OCSP_REQ_CTX_free->OSSL_HTTP_REQ_CTX_free OCSP_REQ_CTX_http-> OSSL_HTTP_REQ_CTX_header OCSP_REQ_CTX_add1_header->OSSL_HTTP_REQ_CTX_add1_header OCSP_REQ_CTX_i2d->OSSL_HTTP_REQ_CTX_i2d OCSP_REQ_CTX_get0_mem_bio->OSSL_HTTP_REQ_CTX_get0_mem_bio OCSP_set_max_response_length->OSSL_HTTP_REQ_CTX_set_max_response_length OCSP_REQ_CTX_nbio_d2i->OSSL_HTTP_REQ_CTX_sendreq_d2i OCSP_REQ_CTX_nbio->OSSL_HTTP_REQ_CTX_nbio Made some editorial changes to man3/OCSP_sendreq.pod; move the NOTES text inline. Some of the original functions had no documentation: OCSP_REQ_CTX_new, OCSP_REQ_CTX_http, OCSP_REQ_CTX_get0_mem_bio, OCSP_REQ_CTX_nbio_d2i, and OCSP_REQ_CTX_nbio. Their new counterparts are now documented in doc/man3/OSSL_HTTP_REQ_CTX.pod Fixes openssl#12234 Co-authored-by: Richard Levitte <levitte@openssl.org> Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl#13742) * cleaned internal crypto references; activated all tests * clarified provider activation Co-authored-by: Dr. David von Oheimb <David.von.Oheimb@siemens.com> Co-authored-by: Nirbheek Chauhan <nirbheek.chauhan@gmail.com> Co-authored-by: Etienne Millon <me@emillon.org> Co-authored-by: bazmoz <bazmoz@protonmail.com> Co-authored-by: John Baldwin <jhb@FreeBSD.org> Co-authored-by: David CARLIER <devnexen@gmail.com> Co-authored-by: Matt Caswell <matt@openssl.org> Co-authored-by: Otto Hollmann <otto@hollmann.cz> Co-authored-by: Billy Brumley <bbrumley@gmail.com> Co-authored-by: Nicola Tuveri <nic.tuv@gmail.com> Co-authored-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com> Co-authored-by: Romain Geissler <romain.geissler@amadeus.com> Co-authored-by: anupamam13 <anuavnd@gmail.com> Co-authored-by: Sahana Prasad <sahana@redhat.com> Co-authored-by: Dr. Matthias St. Pierre <matthias.st.pierre@ncp-e.com> Co-authored-by: Richard Levitte <levitte@openssl.org> Co-authored-by: Agustin Gianni <agustingianni@gmail.com> Co-authored-by: Tomas Mraz <tmraz@fedoraproject.org> Co-authored-by: Dmitry Belyavskiy <beldmit@gmail.com> Co-authored-by: Daniel Bevenius <daniel.bevenius@gmail.com> Co-authored-by: Pauli <ppzgs1@gmail.com> Co-authored-by: Rich Salz <rsalz@akamai.com> Co-authored-by: Jon Spillett <jon.spillett@oracle.com> Co-authored-by: Shane Lontis <shane.lontis@oracle.com> Co-authored-by: Kurt Roeckx <kurt@roeckx.be>

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Jan 5, 2021

mattcaswell closed this Jan 5, 2021

mattcaswell reopened this Jan 5, 2021

openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Jan 5, 2021

baentsch force-pushed the mb-issue13767 branch from 2ec0efd to 722ef51 Compare January 6, 2021 06:57

t8m requested changes Jan 6, 2021

View reviewed changes

mattcaswell requested changes Jan 6, 2021

View reviewed changes

Adding TLS group name retrieval

a13cabd

Function SSL_group_to_name() added, together with documentation and tests. This now permits displaying names of internal and external provider-implemented groups. Partial fix of openssl#13767

baentsch force-pushed the mb-issue13767 branch from 722ef51 to a13cabd Compare January 7, 2021 09:28

mattcaswell requested changes Jan 7, 2021

View reviewed changes

ssl/s3_lib.c Outdated Show resolved Hide resolved

test/sslapitest.c Outdated Show resolved Hide resolved

test/sslapitest.c Outdated Show resolved Hide resolved

addressing PR feedback

9915f4d

baentsch requested review from mattcaswell and t8m January 7, 2021 15:05

mattcaswell requested changes Jan 7, 2021

View reviewed changes

mattcaswell added approval: review pending This pull request needs review by a committer branch: master Applies to master branch labels Jan 7, 2021

mattcaswell added this to the 3.0.0 beta1 milestone Jan 7, 2021

feedback implementation

7ab16b8

baentsch requested a review from mattcaswell January 7, 2021 15:51

mattcaswell approved these changes Jan 7, 2021

View reviewed changes

romen approved these changes Jan 7, 2021

View reviewed changes

romen added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jan 7, 2021

t8m approved these changes Jan 7, 2021

View reviewed changes

mattcaswell added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Jan 8, 2021

mattcaswell closed this Jan 8, 2021

kaduk mentioned this pull request Jan 9, 2021

Make (TLS) groups fully independent of (EC) curves #13767

Closed

Uh oh!

fixes for #13767 #13785

fixes for #13767 #13785

Uh oh!

Conversation

baentsch commented Jan 5, 2021

Checklist

Uh oh!

t8m commented Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattcaswell commented Jan 5, 2021

Uh oh!

mattcaswell commented Jan 5, 2021

Uh oh!

baentsch commented Jan 5, 2021

Uh oh!

t8m commented Jan 5, 2021

Uh oh!

baentsch commented Jan 6, 2021

Uh oh!

romen commented Jan 6, 2021

Uh oh!

t8m commented Jan 6, 2021

Uh oh!

romen commented Jan 6, 2021

Uh oh!

t8m left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t8m commented Jan 5, 2021 •

edited

Loading