EAP-TLS bug fixes and improvements - #625
Open
paulusmack wants to merge 7 commits into
Open
Conversation
This adds enforcement for the following rules in RFC 5216, which defines EAP-TLS. * Response packets must be of the same type as the Request that they are responding to, or Nak. * The peer can't respond with Nak after previously agreeing to some type of authentication by sending a Response of that type. Also remove a redundant test for !explicit_remote - if we're in eapIdentify state, we just sent an Identity request, and the peer mustn't Nak that (regardless of whether explicit_remote is set). Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
… 1.3 RFC9190, which describes modifications to the EAP-TLS protocol to adapt to TLS v1.3, requires the server to send a zero byte through the encrypted tunnel as a protected status indication. This is implemented here in eaptls_send() after the SSL_read() that serves to advance the TLS handshake process and before reading the data to be sent to the peer. The byte is sent using SSL_write(). The client receives this byte via the SSL_read() call in eaptls_receive(). The eaptls_is_init_finished() function now checks that the zero byte has been received rather than checking for a session ticket. It also doesn't return a finished indication if there is still data to be sent; if it did, we would send an ack instead of sending the data. On the server side, we tell OpenSSL to send one session ticket to the peer, as RFC9190 requires that. We don't do anything with it on either the server or the client side since we don't support session resumption. This has some other minor fixes, comments updates, extra debug, etc., such as not trying to send anything when eaptls_send() returns failure due to no data being available. Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
Instead of doing BIO_read() into a large on-stack buffer and then copying that to a buffer on the heap, this instead uses BIO_pending() to find out how much data is available, allocates the buffer, and then does the BIO_read() directly into the heap buffer. Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
With OpenSSL v3, TLS 1.0 and 1.1 are disabled by default, and hence authentication will fail if the max-tls-version option is used to specify use of TLS 1.0 or 1.1. This adds an OpenSSL call to reduce the security level if TLS 1.0 or 1.1 is requested and OpenSSL v3 is in use, which should enable the use of those protocols. However, global operating-system policy might still prevent the use of TLS 1.0 or 1.1 (for example on Fedora). Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
When pppd is authenticating the peer using EAP-TLS, after the TLS handshake finishes successfully, the EAP state machine goes into state eapTlsRecvClient waiting for an EAP-TLS Ack from the peer indicating that mutual authentication was successful. When that Ack arrives, we send an EAP-Success packet and free the TLS session data structure, but stay in eapTlsRecvClient state. Normally the client doesn't then send any further EAP packets. But a malicious client that sent another EAP-TLS Ack with a suitable ID value (equal to one more than the ID on the EAP-Success packet) would cause the code to redo eaptls_gen_mppe_keys() using the freed session pointer, and then free the TLS session data structure again, leading to memory corruption and pppd crashing with SIGSEGV. The same effect would happen if the peer sent a malformed short EAP-TLS packet followed by a legitimate EAP-TLS Ack. This fixes it by (a) going to eapOpen state when the EAP-Success packet is sent (as in the CHAP-MD5 case), (b) setting the session pointer to NULL after freeing it (both in the server and the client case), and (c) not freeing the state when a malformed packet is received. Additionally we drop any EAP-TLS or Nak Response packet when authentication is finished (the state machine is in eapOpen or eapBadAuth state). Thanks to the Kimi Security Team for finding and reporting this. Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
Collaborator
Author
|
@jjkeijser @enaess review appreciated |
Collaborator
Author
|
With TLS 1.3, one thing to note is that because this now sends and receives the 0x00 byte through the tunnel as the protected status indication specified by RFC9190 when TLS 1.3 is being used, authentication won't complete properly where this new code is the client and our old experimental code (as in 2.4.9 up to 2.5.3) is the server. Hopefully that isn't a problem in practice since people shouldn't have been using 'max-tls-version 1.3' in production. |
Member
|
@jjkeijser, @enaess: Can you check this new @paulusmack PR? Linked to: |
…e handling The definition of eap_client_active() was different depending on whether EAP-TLS was enabled or not; with it enabled, eapOpen and eapBadAuth counted as active, whereas without it, they didn't. This complicated analysis of the code and made the behavior of eap_success() and eap_failure() differ depending on the configuration. To fix this, the eap_client_active() definition is changed to make all states other than eapInitial, eapPending, eapClosed, eapOpen and eapBadAuth be active states, regardless of whether EAP-TLS is enabled or not. (For the EAP-TLS disabled case, this turns out to be effectively the same as the previous definition.) The definition of eap_server_active() was incorrect because it didn't include the eapMSCHAPv2Chall state as active. This fixes it by making the last state which is active be eapMSCHAPv2Chall rather than eapMD5Chall. The result turns out to be equivalent to eap_client_active() in fact. Also fix eap_success() and eap_failure() so that they never call auth_withpeer_success/fail more than once. An EAP-Success message in eapSuccess state, and an EAP-Failure in eapBadAuth state, are silently discarded. EAP-Success messages are discarded with a debug print in all inactive states, or in any state other than eapTlsRecvSuccess if EAP-TLS authentication is in use. EAP-Failure messages are discarded with a debug print in eapOpen state, and silently discarded in all other non-active states. Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
This adds code in the EAP client path to check that after the peer sends one type of authentication request, it doesn't then switch to a different type. If it does, requests of the new type are discarded with a warning. This is implemented with a new state called eapAuthRecv, where we have received and responded to some type of Request with a Response (rather than a Nak) and so we only expect to see more Requests of that type or Success or Failure messages. This also fixes a couple of places where we were sending a Nak with suggested type SRP. Since we don't implement SRP, this is pretty useless. Having the eapAuthRecv state means we can now detect if the peer sends a MS-CHAPv2 CHAP_SUCCESS or CHAP_FAILURE message without having first send a CHAP_CHALLENGE, which could (in the CHAP_SUCCESS case) be an attempt to evade mutual authentication. Such messages are discarded without acting on them. In the failure case the open-coded equivalent of eap_failure() at the label client_failure has been replaced with a call to eap_failure(), since that does the auth_withpeer_fail() call that was missing in the open-coded version. In the PEAP case, if peap_process() returns an error, we can only send a Nak in response to the first PEAP request. Instead we fail the authentication by calling eap_failure(). Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This updates EAP-TLS code to make it compliant with RFC9190, i.e., work correctly with TLS v1.3, and to fix a nasty potential use-after-free and double-free which is triggerable by the peer sending an extra Ack after it gets the Success packet.