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

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

Closed
Mekk opened this issue Aug 1, 2022 · 2 comments

Comments

@Mekk
Copy link

Mekk commented Aug 1, 2022

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).

@mcauto
Copy link

mcauto commented Sep 28, 2022

Why don't you put up pull request?

@edenhill
Copy link
Contributor

Thank you @Mekk , this fix will be included in the next release.

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

3 participants