Missing resourse release of function cert_new_load()

[lc3412]

Hi,

When I read the code, I notice that function cert_new_load() is missing the corresonding resource release.

See the following code, function cert_new_load() allocates the resource (Line 64), however, when the target function X509_check_private_key() fails (Line 70) and then does some handling actions (Line 71 - Line 74). There doesnot exist an action to release the resource allocated by cert_new_load(), resulting in a resource release bug.

Ref: https://github.com/droe/sslsplit/blob/develop/opts.c#L64-L75

Line 64:    cert = cert_new_load(filename);
Line 65:    if (!cert) {
Line 66:        log_err_printf("Failed to load cert and key from PEM file "
Line 67:                        "'%s'\n", filename);
Line 68:        return NULL;
Line 69:    }
Line 70:    if (X509_check_private_key(cert->crt, cert->key) != 1) {
Line 71:        log_err_printf("Cert does not match key in PEM file "
Line 72:                        "'%s':\n", filename);
Line 73:        ERR_print_errors_fp(stderr);
Line 74:        return NULL;
Line 75:    }

Furthermore, I find the other call sites of function cert_new_load() release the allocated resources, see the following References:

https://github.com/droe/sslsplit/blob/develop/cert.t.c#L43-L49 https://github.com/droe/sslsplit/blob/develop/cert.t.c#L57-L64 https://github.com/droe/sslsplit/blob/develop/cachetgcrt.t.c#L58-L65 https://github.com/droe/sslsplit/blob/develop/cachetgcrt.t.c#L82-L88 https://github.com/droe/sslsplit/blob/develop/cachetgcrt.t.c#L96-L113

Also, in this ref (https://github.com/droe/sslsplit/blob/develop/cert.t.c#L57-L64), there exists a double free bug?? As shown in Line 62, the allocated resource is already freed, and then it is freed again (Line 64).

Line 57:    c = cert_new_load(TESTCERT);
Line 58:    fail_unless(!!c, "loading PEM failed");
Line 59:    fail_unless(c->references == 1, "refcount mismatch");
Line 60:    cert_refcount_inc(c);
Line 61:    fail_unless(c->references == 2, "refcount mismatch");
Line 62:    cert_free(c);
Line 63:    fail_unless(c->references == 1, "refcount mismatch");
Line 64:    cert_free(c);

[sonertari]

In the first case, if opts_load_cert_chain_key() returns a NULL cert, sslsplit exits with failure, almost immediately, after all calls to that function. So there is actually no memory leak in the normal operation of sslsplit there.

In the second case, we are testing the handling of the references field in the cert struct. The calls to cert_new_load() and cert_refcount_inc() functions increment the references field to 2. So we decrement it by calling cert_free() twice. Since cert_free() does not free the cert param unless the references field is 0, there is no illegal memory access there (the deliberate access is below the last call there, but we have disabled it already).

You should read all related code, especially the code before and after the lines or functions you are inspecting. Keep looking, and report back if you can find an actual issue in our code.

[lc3412]

Thanks for your suggestion and explanation!!