Skip to content

v3.0.x: TLSv1.3 support for EAP-{TLS,TTLS,PEAP} #3516

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jul 7, 2020

Conversation

jimdigriz
Copy link
Contributor

@jimdigriz jimdigriz commented Jul 6, 2020

Picking up from where #2449 left off and aligning to draft-ietf-emu-eap-tls13 (TLS) and draft-ietf-emu-tls-eap-types (TTLS/PEAP) we have this patch set.

TLSv1.3 unit tests disabled for now (though they work) as a patched eapol_test from https://gitlab.com/jimdigriz/hostap/-/compare/master...tls13 is required.

Copy link
Member

@alandekok alandekok left a comment

Choose a reason for hiding this comment

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

I'd just drop this commit. The session resumption cache includes TLS attributes, i.e. expiry, cert information, etc. If we don't cache those attributes, then the user could get back online after his certificate has expired. Which is a security violation.

@jimdigriz
Copy link
Contributor Author

I'd just drop this commit. The session resumption cache includes TLS attributes, i.e. expiry, cert information, etc. If we don't cache those attributes, then the user could get back online after his certificate has expired. Which is a security violation.

Which commit? Nothing was reference. Do you mean 'tls: fix session resumption' (8cf675f)?

@jimdigriz
Copy link
Contributor Author

I'd just drop this commit. The session resumption cache includes TLS attributes, i.e. expiry, cert information, etc. If we don't cache those attributes, then the user could get back online after his certificate has expired. Which is a security violation.
I'd just drop this commit. The session resumption cache includes TLS attributes, i.e. expiry, cert information, etc. If we don't cache those attributes, then the user could get back online after his certificate has expired. Which is a security violation.

Which commit? Nothing was reference. Do you mean 'tls: fix session resumption' (8cf675f)?

Removed, and reworked the unit test so to tickle an attribute so that session resumption works.

@jimdigriz
Copy link
Contributor Author

The only outstanding task I know of is to discard any early_data

@jimdigriz jimdigriz force-pushed the tls13 branch 2 times, most recently from c2e9f67 to 195eb03 Compare July 6, 2020 16:18
@arr2036
Copy link
Member

arr2036 commented Jul 6, 2020

This is meant to be a patch series for TLS 1.3. Strip out the commits not related to TLS 1.3.

@jimdigriz jimdigriz changed the title TLSv1.3 support for EAP-{TLS,TTLS,PEAP} v3.0.x: TLSv1.3 support for EAP-{TLS,TTLS,PEAP} Jul 6, 2020
@jimdigriz jimdigriz force-pushed the tls13 branch 2 times, most recently from 2524e3b to 5a0bce7 Compare July 6, 2020 17:52
@jimdigriz
Copy link
Contributor Author

This is meant to be a patch series for TLS 1.3. Strip out the commits not related to TLS 1.3.

Done

@jimdigriz
Copy link
Contributor Author

Don't merge this, I have found some bugs...

draft-ietf-emu-eap-tls13 (tls) and draft-ietf-emu-tls-eap-types (ttls/peap)
@jimdigriz
Copy link
Contributor Author

jimdigriz commented Jul 6, 2020

Okay, added SSL_CTX_set_max_early_data(..., 0) (openssl/openssl@c39e404) to avoid early data as per https://tools.ietf.org/html/draft-ietf-emu-eap-tls13-10#section-2.1.1 and I tickled out the bug I found in EAP-TTLS (label not being set on a path).

Should be ready for consideration/review.

The other patches that were in here I will include in another PR along with a fix for #1756

eaptls_gen_eap_key(handler->request->reply, tls_session->ssl,
handler->type);
rad_assert(context_tls13[0] == handler->type); /* expected by eaptls_gen_eap_key */
eaptls_gen_eap_key(handler->request->reply, tls_session->ssl, context_tls13, context_tls13_size);
Copy link
Member

@alandekok alandekok Jul 6, 2020

Choose a reason for hiding this comment

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

eaptls_gen_key() always dereferences context, which should be handler->type. So it's not clear why this change is necessary. Why not just leave the old code for eaptls_gen_key() as-is?

Copy link
Contributor Author

@jimdigriz jimdigriz Jul 7, 2020

Choose a reason for hiding this comment

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

Resolved by eced45c f97c486

/*
* EAP (hopefully) does not have middlebox deployments
*/
#ifdef SSL_OP_ENABLE_MIDDLEBOX_COMPAT
Copy link
Member

Choose a reason for hiding this comment

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

I'd add a comment about "If set then dummy Change Cipher Spec (CCS) messages are sent in TLSv1.3. This has the effect of making TLSv1.3 look more like TLSv1.2 so that middleboxes that do not understand TLSv1.3 will not drop the connection. This isn't needed for EAP-TLS, so we disable it"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved by becf52a

@@ -3294,6 +3301,10 @@ SSL_CTX *tls_init_ctx(fr_tls_server_conf_t *conf, int client)

SSL_CTX_set_options(ctx, ctx_options);

#if OPENSSL_VERSION_NUMBER >= 0x10100000L && OPENSSL_VERSION_NUMBER < 0x10101000L
SSL_CTX_set_max_early_data(ctx, 0);
Copy link
Member

Choose a reason for hiding this comment

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

I'd add a comment "TLS 1.3 introduces the concept of early data (also known as zero round trip data or 0-RTT data). Early data allows a client to send data to a server in the first round trip of a connection, without waiting for the TLS handshake to complete if the client has spoken to the same server recently. This doesn't work for EAP, so we disable early data"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved by 3abc316


prf_size = strlen(prf_label);
len = strlen(label);
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 not sure why variable names were changed, but whatever.

Copy link
Contributor Author

@jimdigriz jimdigriz Jul 7, 2020

Choose a reason for hiding this comment

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

@@ -219,18 +232,21 @@ void eapttls_gen_challenge(SSL *s, uint8_t *buffer, size_t size)
memcpy(p, s->s3->client_random, SSL3_RANDOM_SIZE);
p += SSL3_RANDOM_SIZE;
memcpy(p, s->s3->server_random, SSL3_RANDOM_SIZE);
p += SSL3_RANDOM_SIZE;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this line is necessary. Compilers may complain about unused assignments

Copy link
Contributor Author

@jimdigriz jimdigriz Jul 7, 2020

Choose a reason for hiding this comment

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

Resolved by 5deea57

* TLSv1.3 exports a different key depending on the length
* requested so ask for *exactly* what the spec requires
*/
eapttls_gen_challenge(ssl, challenge, vp->vp_length + 1);
Copy link
Member

Choose a reason for hiding this comment

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

https://tools.ietf.org/html/rfc5281#section-11.2.4 says 16 octets of challenge, followed by 1 octet of ident.

We might want to check the ident, too...

Copy link
Contributor Author

@jimdigriz jimdigriz Jul 7, 2020

Choose a reason for hiding this comment

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

The comment above this (vintage deKok circa 2003): "We're a little forgiving in that we have loose checks on the length, and we do NOT check the Id (first octet of the response to the challenge). But if the client gets the challenge correct, we're not too worried about the Id."

I suspect this is because this code is in the middle of the VP iterator and may not have seen the challenge yet. Do you want me to break the check out to actually look at the Id?

@alandekok alandekok merged commit e8b4d28 into FreeRADIUS:v3.0.x Jul 7, 2020
@jimdigriz jimdigriz deleted the tls13 branch July 7, 2020 11:16
michael-dev pushed a commit to michael-dev/freeradius-server that referenced this pull request Sep 9, 2020
* eap-tls: tls version 1.3

draft-ietf-emu-eap-tls13 (tls) and draft-ietf-emu-tls-eap-types (ttls/peap)

* SQUASH: remove unneeded line, compilers complain about more readable code rather than sensibly optimise away

FreeRADIUS#3516 (review)

* SQUASH: comment about early data

* SQUASH: middleware boxen comment

* SQUASH: method of using context is too confusing retain original behaviour
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants