Skip to content
This repository has been archived by the owner on Jan 8, 2025. It is now read-only.

Enable client auth using Picnic #20

Closed
wants to merge 1 commit into from
Closed

Enable client auth using Picnic #20

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions crypto/x509/x509type.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
#include <openssl/evp.h>
#include <openssl/objects.h>
#include <openssl/x509.h>
#include <openssl/oqs_sig.h>

int X509_certificate_type(X509 *x, EVP_PKEY *pkey)
{
Expand Down Expand Up @@ -116,6 +117,9 @@ int X509_certificate_type(X509 *x, EVP_PKEY *pkey)
case NID_X9_62_id_ecPublicKey:
ret |= EVP_PKS_EC;
break;
case NID_oqs_picnic_default:
ret |= EVP_PKT_SIGN;
break;
default:
break;
}
Expand Down
5 changes: 5 additions & 0 deletions ssl/s3_both.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,11 +413,16 @@ long ssl3_get_message(SSL *s, int st1, int stn, int mt, long max, int *ok)
s->s3->tmp.message_type = *(p++);

n2l3(p, l);
/* FIXMEOQS: using OQS's Picnic client auth triggers this error.
Not sure why a server-side Picnic cert is correctly fragmented
but not the client side. Hopefully this has no negative side
effects.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little reluctant to remove a bounds check... such is the way to another heartbleed. Have you been able to trace back which function is calling this function in the context where it triggers this issue, and see what max value is being provided and why it's that particular max value?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, not the best idea to simply remove checks. TLS1.2 mandates a max fragment size of 16384 = 2^14. For some reason, the client-size record is not fragmented properly (unlike the server-side one). This is a work around to enable testing with client-side Picnic certs. The error is triggered in 3 locations: twice in s3_clnt where 16384 is passed as the max value to ssl_get_message, and once in s3_srvr where the max value is specified by SSL3_RT_MAX_PLAIN_LENGTH = 16384.

The proper resolution would be to figure out how to properly fragment records. We can leave this as an open PR for now to unblock folks who'd like to enable client-side auth (but I'll resubmit another PR for the other modified file).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it is worth doing a resync with upstream before trying to do this -- there's a chance that they've fixed this bug.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take a look.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meanwhile, I've created another PR for the CA cert fix; see #21. We can merge that separately.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sync'ed with OpenSSL upstream, pulling changes from 1.0.2o-dev. It doesn't solve the problem, and on top of that, it breaks Picnic cert issuance.

if (l > (unsigned long)max) {
al = SSL_AD_ILLEGAL_PARAMETER;
SSLerr(SSL_F_SSL3_GET_MESSAGE, SSL_R_EXCESSIVE_MESSAGE_SIZE);
goto f_err;
}
*/
/*
* Make buffer slightly larger than message length as a precaution
* against small OOB reads e.g. CVE-2016-6306
Expand Down