librdkafka icon indicating copy to clipboard operation
librdkafka copied to clipboard

Memory leak in rd_kafka_cert_new (with trivial fix) - in loading PEM CA certificate

Open Mekk opened this issue 3 years ago • 0 comments

Problem

rd_kafka_cert_new(encoding=RD_KAFKA_CERT_ENC_PEM, type=RD_KAFKA_CERT_CA, …) leaks allocated X509 buffer (result of PEM_read_bio_X509).

This is the problematic code: https://github.com/edenhill/librdkafka/blob/9b72ca3aa6c49f8f57eea02f70aadb1453d3ba1f/src/rdkafka_cert.c#L264

                             while ((x509 =
                                        PEM_read_bio_X509(
                                                bio, NULL,
                                                rd_kafka_conf_ssl_passwd_cb,
                                                (void *)conf))) {

                                        if (!X509_STORE_add_cert(cert->store,
                                                                 x509)) {
                                                action = "add certificate to "
                                                        "X.509 store";
                                                X509_free(x509);
                                                goto fail;
                                        }

                                        cnt++;
                                }

Solution

Adding simple

X509_free(x509);

above cnt ++ resolves the issue:

                             while ((x509 =
                                        PEM_read_bio_X509(
                                                bio, NULL,
                                                rd_kafka_conf_ssl_passwd_cb,
                                                (void *)conf))) {

                                        if (!X509_STORE_add_cert(cert->store,
                                                                 x509)) {
                                                action = "add certificate to "
                                                        "X.509 store";
                                                X509_free(x509);
                                                goto fail;
                                        }

                                        X509_free(x509);   // <---- This fixes the problem
                                        cnt++;
                                }

Notes

I verified in OpenSSL sources, that similar construct is used there (= that X509_STORE_add_cert copies data and doesn't consume the pointer). For example https://github.com/openssl/openssl/blob/master/crypto/x509/by_file.c#L117 (note that there is X509_free(x) there in correct path in spite of earlier call to X59-_store_add_cert on this var).

Mekk avatar Aug 01 '22 17:08 Mekk