randombit / botan

Cryptography Toolkit
https://botan.randombit.net
BSD 2-Clause "Simplified" License
2.52k stars 558 forks source link

Instead of sending Server Hello, it sends Server Hello Done #2583

Closed jeffRTC closed 3 years ago

jeffRTC commented 3 years ago

@randombit

Wired but this is what happen when I inspect traffic on Wireshark.

Please see my code,

namespace TLS
{
    typedef std::chrono::duration<int, std::ratio<31556926>> years;

    class callbacks : public Botan::TLS::Callbacks
    {
    public:
        std::string decodedMessage; // Store Decoded (In)
        std::string encodedMessage; // Store Encoded (Out)

    public:
        void tls_emit_data(const uint8_t data[], size_t size) override
        {
            encodedMessage = std::string(data, data + size);
        }

        void tls_record_received(uint64_t seq_no, const uint8_t data[], size_t size) override
        {

            decodedMessage += std::string(data, data + size);
        }

        void tls_alert(Botan::TLS::Alert alert) override
        {
            // handle a tls alert received from the tls server
            std::cout << alert.type_string() << "\n";
        }

        bool tls_session_established(const Botan::TLS::Session &session) override
        {
            // the session with the tls client was established
            // return false to prevent the session from being cached, true to
            // cache the session in the configured session manager
            return false;
        }
    };

    class credentials : public Botan::Credentials_Manager
    {
    private:
        struct certificate
        {
            std::vector<Botan::X509_Certificate> certs;
            std::shared_ptr<Botan::Private_Key> key;
        };

        std::vector<certificate> creds;
        std::vector<std::shared_ptr<Botan::Certificate_Store>> store;

    public:
        Botan::Private_Key *private_key_for(const Botan::X509_Certificate &cert,
                                            const std::string & /*type*/,
                                            const std::string & /*context*/) override
        {
            for (auto &&i : creds)
            {
                if (cert == i.certs[0])
                {
                    return i.key.get();
                }
            }

            return nullptr;
        }

        std::vector<Botan::X509_Certificate> cert_chain(const std::vector<std::string> &algos,
                                                        const std::string &type,
                                                        const std::string &hostname) override
        {
            BOTAN_UNUSED(type);

            for (auto &&i : creds)
            {
                if (std::find(algos.begin(), algos.end(), i.key->algo_name()) == algos.end())
                {
                    continue;
                }

                if (hostname != "" && !i.certs[0].matches_dns_name(hostname))
                {
                    continue;
                }

                return i.certs;
            }

            return std::vector<Botan::X509_Certificate>();
        }

        std::vector<Botan::Certificate_Store *> trusted_certificate_authorities(const std::string &type,
                                                                                const std::string & /*hostname*/) override
        {
            std::vector<Botan::Certificate_Store *> v;

            // don't ask for client certs
            if (type == "tls-server")
            {
                return v;
            }
            else
            {
                std::cout << "DEBUG: Request for Client Cert"
                          << "\n";
            }

            for (auto &&cs : store)
            {
                v.push_back(cs.get());
            }

            return v;
        }

        void createCert(std::string hostname)
        {
            /**
             * Check for existing Certs 
            **/

            for (auto &&i : creds)
            {

                if (!i.certs[0].matches_dns_name(hostname))
                {
                    continue;
                }

                // Found a match for hostname
                return;
            }

            /**
             * Initialize Root CA
            **/

            Botan::AutoSeeded_RNG rng;

            const Botan::X509_Certificate rootCert("myCA.pem");

            std::ifstream rootCertPrivateKeyFile("myCAKey.pkcs8.pem");

            Botan::DataSource_Stream rootCertPrivateKeyStream(rootCertPrivateKeyFile);

            std::unique_ptr<Botan::Private_Key> rootCertPrivateKey = Botan::PKCS8::load_key(rootCertPrivateKeyStream, "pass123");

            Botan::X509_CA rootCA(rootCert, *rootCertPrivateKey, "SHA-256", rng);

            /**
            * Generate a Cert & Sign with Root CA
            **/

            Botan::X509_Cert_Options opts;
            std::shared_ptr<Botan::Private_Key> serverPrivateKeyShared(new Botan::RSA_PrivateKey(rng, 4096));
            Botan::RSA_PrivateKey* serverPrivateKey = (Botan::RSA_PrivateKey*)serverPrivateKeyShared.get();

            opts.common_name = hostname;
            opts.country = "US";

            auto now = std::chrono::system_clock::now();

            Botan::X509_Time todayDate(now);
            Botan::X509_Time expireDate(now + years(1));

            Botan::PKCS10_Request req = Botan::X509::create_cert_req(opts, *serverPrivateKey, "SHA-256", rng);

            auto serverCert = rootCA.sign_request(req, rng, todayDate, expireDate);

            /**
             * Load Cert to In-Memory Database
            **/

            certificate cert;

            cert.certs.push_back(serverCert);
            cert.key = serverPrivateKeyShared;

            creds.push_back(cert);
        }
    };
}; // namespace TLS

Socket part,

creds.createCert("www.google.com");

existingConnection->second->server.received_data(reinterpret_cast<const uint8_t*>(res.c_str()), res.size());

if (!existingConnection->second->tlsCallbacks.decodedMessage.empty())
{
    std::cout << "We are with a decoded message"
                << existingConnection->second->tlsCallbacks.decodedMessage
                << "\n";
}

if (!existingConnection->second->tlsCallbacks.encodedMessage.empty())
{
    std::cout << "We have to send a message"
                << existingConnection->second->tlsCallbacks.encodedMessage
                << "\n";

    // Send Response
    socket.send(frames[0], zmq::send_flags::sndmore);
    socket.send(zmq::buffer(existingConnection->second->tlsCallbacks.encodedMessage), zmq::send_flags::sndmore);
}

I know Botan has it own send method but I'm not sure what its. I use Botan's received_data method to pass the data the socket received

randombit commented 3 years ago

Can you attach the pcap? Sending server hello done before server hello makes no sense

nametoolong commented 3 years ago

The previous messages (Server Hello, KEX, etc.) of server handshake sequence might be overridden in tls_emit_data, only the last message (Server Hello Done) being sent. There is no support for coalescing records in Botan yet so you should ensure consecutive calls to tls_emit_data preserve their original order.

jeffRTC commented 3 years ago

@nametoolong

I think what you described is exactly the issue. I've modified the code and push messages into a vector on tls_emit_data event. It seems it sends Server Hello but nowhere to find Sever Hello Done.

What could be causing that?

The vector only has 1 message when I inspect it which is Server Hello.

@randombit Please see the pcap file.

ssl_packets.zip

@randombit Things are different from original issue now. Now it sends Server Hello but never Sever Hello Done.

nametoolong commented 3 years ago

Is the buffer fully copied into the vector? Only saving the pointer is pointless per the doc. Concatenating multiple messages is fine though.

After this function returns, the buffer containing data will be overwritten, so a copy of the input must be made if the callback cannot send the data immediately.

If you still cannot locate the issue, try counting how many times tls_emit_data is called. The difference between invocation count and on-wire message count indicates an issue.

jeffRTC commented 3 years ago

@nametoolong

I convert buffer to std::string then emplace back to vector.

encodedMessages.emplace_back(std::string(data, data + size));

The reason I do so is because ZeroMQ Stream Socket type doesn't accept raw buffer so I have to pass a String.

nametoolong commented 3 years ago

Passing an already-constructed std::string to emplace_back kind of defeats the whole point.

I'm unsure why other messages disappear but counting tls_emit_data invocations might help. Botan 2 always sends one record per invocation. If there is only one call to tls_emit_data on the server side then it is a Botan bug.

jeffRTC commented 3 years ago

@nametoolong

I did count.

The callbacks get invoked 5 times and the vector only has two messages...

Not sure what you mean by defats whole point. I need to access messages outside of callback so I use emplace back

I am going to try with push back now and see if anything change

jeffRTC commented 3 years ago

It's crazy, even with push_back, the result is same

jeffRTC commented 3 years ago

@nametoolong

Strange....

It appear to be bug within C++ Ranged Loops

                        int counter = 0;

                        std::cout << existingConnection->second->tlsCallbacks.encodedMessages.size() << "\n";

                         // Logs 4 

                        for(const auto &i : existingConnection->second->tlsCallbacks.encodedMessages) {
                            std::cout << "MESSAGE :" <<  counter << "\n";
                            // Logs 1,2 then stop

                            // Send Response
                            socket.send(frames[0], zmq::send_flags::sndmore);
                            socket.send(zmq::buffer(i), zmq::send_flags::sndmore);

                            counter++;
                        }
jeffRTC commented 3 years ago

@nametoolong

Posted this one Stackoverflow. They suggested this behavior could happen if callbacks are from another thread or whatever..

They closed the question because they can't reproduce due to not giving them full code...

jeffRTC commented 3 years ago

@nametoolong

I tried with raw loop, same behavior, it doesn't loop over all elements.

Strange ..

nametoolong commented 3 years ago

This is weird... Not a Botan bug. I have no idea either. :shrug:

What I mean by defeats the whole point is that emplace_back(std::string(data, data + size)) does not emplace. It actually constructs a temporary string first and then copies it into the end of the array. You should doemplace_back(data, data + size) to emplace directly.

jeffRTC commented 3 years ago

@nametoolong

I tried but nothing has changed. It only loop over two elements

@randombit Do you have any idea either? Is this happen because some sort of threading in your library?