Skip to content
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

Invalid ClientFinish message when EarlyData sent #48

Closed
xpepermint opened this issue Aug 5, 2022 · 7 comments
Closed

Invalid ClientFinish message when EarlyData sent #48

xpepermint opened this issue Aug 5, 2022 · 7 comments

Comments

@xpepermint
Copy link

xpepermint commented Aug 5, 2022

I've noticed an issue with, I believe, the ClientFinished record when early data are sent. It could be something wrong with the transcript hash - just guessing.

Run an example below:

require 'socket'
require 'tttls1.3'

ca_file = __dir__ + '/tmp/ca.crt'

settings2 = {
    ca_file: ca_file,
    alpn: ['http/1.1'],
    supported_groups: [TTTLS13::NamedGroup::SECP256R1],
    cipher_suites: [TTTLS13::CipherSuite::TLS_AES_256_GCM_SHA384],
    check_certificate_status: false,
}

settings1 = {
    ca_file: ca_file,
    alpn: ['http/1.1'],
    supported_groups: [TTTLS13::NamedGroup::SECP256R1],
    cipher_suites: [TTTLS13::CipherSuite::TLS_AES_256_GCM_SHA384],
    check_certificate_status: false,
    

    process_new_session_ticket: lambda do |nst, rms, cs|
        return if Time.now.to_i - nst.timestamp > nst.ticket_lifetime
        settings2[:ticket] = nst.ticket
        settings2[:resumption_master_secret] = rms
        settings2[:psk_cipher_suite] = cs
        settings2[:ticket_nonce] = nst.ticket_nonce
        settings2[:ticket_age_add] = nst.ticket_age_add
        settings2[:ticket_timestamp] = nst.timestamp
        p "NEW-SESSION-TICKET:"
        p "=> ticket: #{nst.ticket.bytes}"
        p "=> resumption_master_secret: #{rms.bytes}"
        p "=> psk_cipher_suite: #{cs}"
        p "=> ticket_nonce: #{nst.ticket_nonce.bytes}"
        p "=> ticket_age_add: #{nst.ticket_age_add.bytes}"
        p "=> ticket_timestamp: #{nst.timestamp * 1000}"
    end
}

socket = TCPSocket.new("ssltest.louis.info", 443)
client = TTTLS13::Client.new(socket, "ssltest.louis.info", settings1)
client.connect 
client.write("GET / HTTP/1.1Host: ssltest.louis.infoConnection: close\r\n\r\n\r\n")
loop {
    p client.read
    break if client.eof?
}

sleep(1)
p ''
p ''
p '---------------------------------- NEW REQUEST'
p ''
p ''


socket = TCPSocket.new("ssltest.louis.info", 443)
client = TTTLS13::Client.new(socket, "ssltest.louis.info", settings2)
client.early_data("GET / HTTP/1.1Host: ssltest.louis.infoConnection: close\r\n\r\n\r\n")
client.connect 
loop {
    p client.read
    break if client.eof?
}

You will notice that the server reponded with bad_record_mac alert.

@xpepermint
Copy link
Author

I recalculated some of the keys (e.g. "client finished") and things look ok.

I opened this issue so we might get some help: https://stackoverflow.com/questions/73268472/bad-record-mac-on-tls-v1-3-when-using-eary-data

@xpepermint
Copy link
Author

xpepermint commented Aug 7, 2022

Further development led me to new discoveries. First, here's a simple HTTP request that I'm sending to the server.

GET / HTTP/1.1\r\n
Host: $HOST\r\n
\r\n

I went "testing" all over and finally disabled the line that is sending ClientFinished message to the server. I wrapped it into an if sentence which caused EarlyData to be sent only when confirmed/accepted by the server. As the result, the Alert(20) (Bad Record MAC) error was gone.
This solution obviously worked wether it represented THE solution or was just a workaround. I tested it against 5 different websites and all worked the same way.
Let me mention that by adding Connection: close\r\n header to the request I even got Alert(0) (Close Notify) back which gracefully ended the connection.
That's not all! It turned out that ClientFinished message should not be sent or better is not expected even when using PSK without EarlyData. We can refer to the sentence from the spec below:

PSK-based authentication happens as a side effect of key exchange.

However, this solution doesn't work in the case where you want to split application data into 1) EarlyData and 2) RealApplicationData. I don't think this would ever be a real use case and it could possibly be treated as a "user-tries-to-hack" attack that a server should reject by default. Anyhow, sending EarlyData("GET / HTTP/1.1\r\n") first and then RealApplicationData("Host: $HOST\r\n\r\n") results in the Alert(20) issue again.

So ... I don't know, looks like this might be the way it is supposed to work. If this is the "expected" functionality it could make sense that client authentication happens in the first negotiation only and the resumed sessions are treated as already-authenticated channels.
Well, maybe it's just me but I don't see these clearly stated in the official spec https://datatracker.ietf.org/doc/html/draft-ietf-tls-rfc8446bis-04#section-4.4.4.

Since the solution works for the majority of the valid cases, I made a PR #49 with short notice so we can come back later if needed.

@xpepermint
Copy link
Author

I'm still not entirely sure if the issue isn't actually with the current server implementations (some old stuff or smth) so I contacted IETF guys to hopefully get support.

@xpepermint
Copy link
Author

The IETF just confirmed that the finished message must always be sent.

@xpepermint
Copy link
Author

xpepermint commented Aug 9, 2022

I noticed that the error is present only when Connection: close\r\n header is NOT present.

Works:

GET / HTTP/1.1
Host: $HOST
Connection: close

Doesn't work:

GET / HTTP/1.1
Host: $HOST

This must be HTTP related issue on the server. Since EarlyData behavior is mostly up to the implementor, it could be that these are just rules that they (server implementors) require you to follow.

Closing this for now.

@thekuwayama
Copy link
Owner

@xpepermint Thank you for contributing.
I investigated this issue and found that TTLS13::Client does not send EndOfEarlyData when 0-RTT. I will try to fix this with #50.

@xpepermint
Copy link
Author

Hum, good! Let’s see if the PR resolves it. Please test it against the same URL.

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

No branches or pull requests

2 participants