Duplicated code src/crypto/tls and src/tls

[mkalex777]

Started to use this library in my hobby project, added some fixes to support ClientHello customization and then found that there are at least two folders with duplicated classes.

For example:

• https://github.com/bcgit/bc-csharp/blob/master/crypto/src/crypto/tls/AbstractTlsClient.cs
• https://github.com/bcgit/bc-csharp/blob/master/crypto/src/tls/AbstractTlsClient.cs

I was worked with src/crypto/tls/AbstractTlsClient.cs, but now it looks that this is a very outdated version and I'm confused.

Could you please explain what is the idea to keep two duplicated folders within the same project?

[peterdettman]

Org.BouncyCastle.Tls is a new replacement API, recently ported from bc-java, with support for TLS 1.3 amongst other features and improvements. .Crypto.Tls will be deprecated in a future release once people have had a chance to move to .Tls and work out any kinks (which won't happen until it's at least appeared in a release). So the duplication is temporary while users migrate.

[mkalex777]

okay. Is it worth to pull request with some minor fixes for old src/crypto/tls version?

Does current src/tls implementation supports TLS 1.3? Is it works? Or still under construction?

I need TLS 1.3 and wanted to add it. If it's already done, then this is a good news :)

[peterdettman]

New _.Tls already has working 1.3, yes (have to specify it in your TlsClient subclass though). I'm happy to apply useful PR to old code (and check if it is relevant to new code).

[mkalex777]

just tried a new code from src/tls folder. At first attempt it works with a simple request with default ClientHello config, but when I replaced default ClientHello config with a little customized, it fails to verify finished message from www.google.com.

Server negotiate cipher suite 0xCCA8 TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256. Old code works ok with exactly the same ClientHello config.

Here is log file (all values are hex):

send client_hello
       random:       [32 bytes]
       sessionId:
       cipherSuites: cca8,cc13,c02f,c014,c013,009c,0035,002f,000a
         renegotiation_info
         server_name: 0:"www.google.com"
         extended_master_secret
         session_ticket
         signature_algorithms: 0601,0603,0501,0503,0401,0403,0301,0303,0201,0203
         status_request
         next_protocol_negotiation
         signed_certificate_timestamp
         application_layer_protocol_negotiation: "h2","spdy/3.1","http/1.1"
         ec_point_formats: 00
         supported_groups: 0017,0018
         padding
       fingerprint:  771,52392-52243-49199-49172-49171-156-53-47-10,65281-0-23-35-13-5-13172-18-16-11-10-21,23-24,0
       ja3_hash:     3d2a747f5bd0fe4b756a0b19d87e3daf
recv server_hello
       random:       [32 bytes]
       sessionId:
       cipherSuite:  cca8 TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
         extended_master_secret
         renegotiation_info
         ec_point_formats: 00
         session_ticket
         application_layer_protocol_negotiation: "h2"
TLS client negotiated TLS 1.2
recv certificate
recv server_key_exchange
recv server_hello_done
send client_key_exchange
send change_cipher_spec
send finished
recv session_ticket
recv change_cipher_spec
recv finished
send alert: fatal, decrypt_error
TlsFatalAlert: decrypt_error(51)

Tested with Wireshark, ClientHello messages are identical for both cases (byte to byte comparison, the difference is just ClientRandom). For both cases the server https://www.google.com selects cipherSuite cca8 TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256, but a new code fails at finished message verification.

Any idea why it may happens?

[peterdettman]

Could you perhaps summarize the changes you're making to the default ClientHello? I see some new extensions, of which the server appears to accept session_ticket and then actually sends a session_ticket handhsake message.

I note in particular that HandshakeType.new_session_ticket is explicitly excluded from the handshake transcript in current code (see TlsProtocol.ProcessHandshakeQueue). Probably that should NOT be excluded from the transcript pre-TLS 1.3, so I would give that a try.

[mkalex777]

I needs full control on extensions which are used in the ClientHello, adding or removing something or change sequence is not an option for me. The extension list should comply with predefined sequence list.

Here is my GetClientExtension method for my TlsClient customization:

abstract class CustomTlsClient : DefaultTlsClient
{
        public override IDictionary GetClientExtensions()
        {
            IDictionary clientExtensions = TlsExtensionsUtilities.EnsureExtensionsInitialised(base.GetClientExtensions());
            {
                TlsExtensionsUtilities.AddMaxFragmentLengthExtension(clientExtensions, MaxFragmentLength.pow2_9);
                TlsExtensionsUtilities.AddPaddingExtension(clientExtensions, m_context.Crypto.SecureRandom.Next(16));
                TlsExtensionsUtilities.AddTruncatedHmacExtension(clientExtensions);

                TlsExtensionsUtilities.AddRecordSizeLimitExtension(clientExtensions, 16385);
                TLS.TlsHelper.Add_renegotiation_info(clientExtensions);
                TlsExtensionsUtilities.AddPaddingExtension(clientExtensions, 0);
            }
            var sequence = GetClientExtensionSequence();
            var remove = clientExtensions.Keys
                .Cast<int>()
                .Except(sequence)
                .ToArray();
            foreach (var key in remove)
            {
                clientExtensions.Remove(key);
            }
            var missing = sequence
                .Except(clientExtensions.Keys.Cast<int>())
                .ToArray();
#if true
            foreach (var m in missing)
            {
                clientExtensions[m] = new byte[0];
            }
#else
            if (missing.Length > 0)
                throw new NotImplementedException("missing extensions: "+string.Join(", ", missing.Select(arg=>arg.ToString()).ToArray()));
#endif
            clientExtensions = TLS.TlsHelper.MakeKeyOrderDictionary<int, byte[]>(clientExtensions, pair => GetExtensionOrder(pair.Key, sequence));
            return clientExtensions;
        }

        private int GetExtensionOrder(int type, int[] sequence)
        {
            for (var i = 0; i < sequence.Length; i++)
                if (sequence[i] == type)
                    return i;
            return -1;
        }
//...
}

This code gets extension list from base.GetClientExtensions() and remove all unwanted extensions (which is missing in GetClientExtensionSequence() list) and add extensions which are missing but required. Then it sorts extensions order to comply with required extension sequence and returns IDictionary wrapper which allows to keep Keys list order unchanged to keep extension sequence when it will be written to the stream.

GetClientExtensionSequence is abstract and implemented in the child class which returns required extension sequence in the ClientHello:

        public override int[] GetClientExtensionSequence()
        {
            return new int[] {
                ExtensionType.renegotiation_info,       //(65281)
                ExtensionType.server_name,              //(0)
                ExtensionType.extended_master_secret,   //(23)
                ExtensionType.session_ticket,           //(35)
                ExtensionType.signature_algorithms,     //(13)
                ExtensionType.status_request,           //(5)
                ExtensionType.next_protocol_negotiation,//(13172)
                ExtensionType.signed_certificate_timestamp,             //(18)
                ExtensionType.application_layer_protocol_negotiation,   //(16)
                ExtensionType.ec_point_formats,         //(11)
                ExtensionType.supported_groups,         //(10)
                ExtensionType.padding,                  //(21) [align 512]
            };
        }

Also I removed kludge in TlsProtocol.WriteExtensionsData to write extensions with requested sequence (original code write it in two steps with break requested extension sequence):

        internal static void WriteExtensionsData(IDictionary extensions, MemoryStream buf, int bindersSize)
        {
            WriteSelectedExtensions(buf, extensions);
            WritePreSharedKeyExtension(buf, extensions, bindersSize);
        }
        internal static void WriteSelectedExtensions(Stream output, IDictionary extensions)
        {
            foreach (Int32 key in extensions.Keys)
            {
                int extension_type = key;
                byte[] extension_data = (byte[])extensions[key];

                TlsUtilities.CheckUint16(extension_type);
                TlsUtilities.WriteUint16(extension_type, output);
                TlsUtilities.WriteOpaque16(extension_data, output);
            }
        }

Also I moved adding extended_master_secret from TlsClientProtocol.SendClientHello() and from DtlsClientProtocol.GenerateClientHello(ClientHandshakeState state) to AbstractTlsClient.GetClientExtensions(). Because original code breaks requested extension list. With this change it will be processed before my code to filter and order required extension sequence.

These changes affects ClientHello message generation and the result ClientHello message is correct (tested with Wireshark). And should not affect data flow logic which can cause verify data change.

The same changes I made in old src/crypto/tls code and all worked great with exactly the same ClientHello message and the same cipherSuite.

I tested it on the following url: "https://www.google.com"

Here is the log file for old code src/tls:

send client_hello
       random:       [32 bytes]
       sessionId:
       cipherSuites: cca8,cc13,c02f,c014,c013,009c,0035,002f,000a
         renegotiation_info
         server_name: 0:"www.google.com"
         extended_master_secret
         session_ticket
         signature_algorithms: 0601,0603,0501,0503,0401,0403,0301,0303,0201,0203
         status_request
         next_protocol_negotiation
         signed_certificate_timestamp
         application_layer_protocol_negotiation: "h2","spdy/3.1","http/1.1"
         ec_point_formats: 00
         supported_groups: 0017,0018
         padding
       fingerprint:  771,52392-52243-49199-49172-49171-156-53-47-10,65281-0-23-35-13-5-13172-18-16-11-10-21,23-24,0
       ja3_hash:     3d2a747f5bd0fe4b756a0b19d87e3daf
recv server_hello
       random:       [32 bytes]
       sessionId:
       cipherSuite:  cca8 TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
         extended_master_secret
         renegotiation_info
         ec_point_formats: 00
         session_ticket
         application_layer_protocol_negotiation: "h2"
TLS client negotiated TLS 1.2
recv certificate
recv server_key_exchange
recv server_hello_done
send client_key_exchange
send change_cipher_spec
send finished
recv session_ticket
recv change_cipher_spec
recv finished
send application_data
recv application_data
send application_data
send application_data
send application_data
send application_data
recv application_data
recv application_data
:status: 200
recv application_data
recv application_data
...

as you can see all exactly the same, but old code pass verify message with no issue and process application_data.

[peterdettman]

Find the following block in TlsProtocol.ProcessHandshakeQueue:

    /*
     * These message types aren't included in the transcript.
     */
    case HandshakeType.hello_request:
    case HandshakeType.key_update:
    case HandshakeType.new_session_ticket:
        break;

and replace it with this:

    /*
     * These message types aren't included in the transcript.
     */
    case HandshakeType.hello_request:
    case HandshakeType.key_update:
        break;

    /*
     * Not included in the transcript for TLS 1.3+
     */
    case HandshakeType.new_session_ticket:
    {
        ProtocolVersion negotiatedVersion = Context.ServerVersion;
        if (null != negotiatedVersion && !TlsUtilities.IsTlsV13(negotiatedVersion))
        {
            buf.UpdateHash(m_handshakeHash);
        }

        break;
    }

Let me know the results.

[mkalex777]

Tried your change, now hash verification has passed. But now it fails at TlsProtocol.CompleteHandshake() here:

                        .SetServerExtensions(m_serverExtensions)
                        .Build();

// fails at the following line, because m_tlsSession==null
                    this.m_tlsSession = TlsUtilities.ImportSession(m_tlsSession.SessionID, m_sessionParameters);
                }
                else
                {
                    securityParameters.m_localCertificate = m_sessionParameters.LocalCertificate;

added the following change:

                    if (m_tlsSession == null)
                        m_tlsSession = TlsUtilities.ImportSession(TlsUtilities.EmptyBytes, null);
                    this.m_tlsSession = TlsUtilities.ImportSession(m_tlsSession.SessionID, m_sessionParameters);
                }
                else

now it works :)

send client_hello
       random:       [32 bytes]
       sessionId:
       cipherSuites: cca8,cc13,c02f,c014,c013,009c,0035,002f,000a
         renegotiation_info
         server_name: 0:"www.google.com"
         extended_master_secret
         session_ticket
         signature_algorithms: 0601,0603,0501,0503,0401,0403,0301,0303,0201,0203
         status_request
         next_protocol_negotiation
         signed_certificate_timestamp
         application_layer_protocol_negotiation: "h2","spdy/3.1","http/1.1"
         ec_point_formats: 00
         supported_groups: 0017,0018
         padding
       fingerprint:  771,52392-52243-49199-49172-49171-156-53-47-10,65281-0-23-35-13-5-13172-18-16-11-10-21,23-24,0
       ja3_hash:     3d2a747f5bd0fe4b756a0b19d87e3daf
recv server_hello
       random:       [32 bytes]
       sessionId:
       cipherSuite:  cca8 TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
         extended_master_secret
         renegotiation_info
         ec_point_formats: 00
         session_ticket
         application_layer_protocol_negotiation: "h2"
TLS client negotiated TLS 1.2
recv certificate
recv server_key_exchange
recv server_hello_done
send client_key_exchange
send change_cipher_spec
send finished
recv session_ticket
recv change_cipher_spec
recv finished
send application_data
recv application_data
send application_data
send application_data
send application_data
send application_data
recv application_data
recv application_data
:status: 200
recv application_data
recv application_data
recv application_data

But I'm not sure is it correct way to fix it? Why m_tlsSession is not initialized and where it is expected to be initialized?

According to the log, the server respond with ServerHello with empty sessionId

[mkalex777]

Just tried with TLS 1.3, also works :)

So the last question is where and how to initialize m_tlsSession properly? Am I doing it in the right way?

[peterdettman]

I will look into what codepath could leave it uninitialized, but it sounds like it's an OK workaround for now.

Edit: Thanks for confirming the fix for new_session_ticket.

[peterdettman]

The issue with m_tlsSession is caused around line 711 of TlsClientProtocol when the new_session_ticket is received. It clears any existing session, but should be resetting the sessionID and restoring m_tlsSession accordingly. Both this fix and the handshake hash update fix are now pushed to our git repo and should mirror here soon (also fixed in bc-java).