459231 - Memory leak in cert fetching - AIA extension.

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Slavomir Katuscak]

Found in PKIX certiticate chain test.

Steps to reproduce:

1. Generate root CA cert.
certutil -N -d RootDB -f RootDB/dbpasswd
certutil -s "CN=Root ROOT CA, O=Root, C=US" -S -n Root -t CTu,CTu,CTu -v 600 -x -d RootDB -1 -2 -5 -f RootDB/dbpasswd -z /export/tinderlight/data/chains/mozilla/tests_results/security/ciotat.4/tests_noise.25494

2. Generate intermediate CA signed by root CA.
certutil -N -d CADB -f CADB/dbpasswd
certutil -s "CN=CA Intermediate, O=CA, C=US" -R -2 -d CADB -f CADB/dbpasswd -z /export/tinderlight/data/chains/mozilla/tests_results/security/ciotat.4/tests_noise.25494 -o CAReq.der
certutil -C -c Root -v 60 -d RootDB -i CAReq.der -o CARoot.der -f RootDB/dbpasswd
certutil -A -n CA -t u,u,u -d CADB -f CADB/dbpasswd -i CARoot.der

3. Generate user certificate signed by intermediate CA, with AIA extension.
certutil -N -d UserDB -f UserDB/dbpasswd
certutil -s "CN=User EE, O=User, C=US" -R  -d UserDB -f UserDB/dbpasswd -z /export/tinderlight/data/chains/mozilla/tests_results/security/ciotat.4/tests_noise.25494 -o UserReq.der
certutil -C -c CA -v 60 -d CADB -i UserReq.der -o UserCA.der -f CADB/dbpasswd   --extAIA
certutil -A -n User -t u,u,u -d UserDB -f UserDB/dbpasswd -i UserCA.der

When asked for AIA details, set CA issuers (1), then uniformResourceidentifier (7) and then http path to intermediate CA cert. 

4. Verify user cert trusting Root CA with fetch flag set (intermediate CA not set on command line, should be fetched).
vfychain -d UserDB -pp -vv -f   UserCA.der  -t Root.der

Memory Leak (mel):
Found leaked block of size 60 bytes at address 0x727a0
At time of allocation, the call stack was:
    [1] PR_Malloc() at line 467 in "prmem.c"
    [2] PORT_Alloc_Util() at line 113 in "secport.c"
    [3] ocsp_ParseURL() at line 2801 in "ocsp.c"
    [4] CERT_ParseURL() at line 3242 in "ocsp.c"
    [5] pkix_pl_AIAMgr_GetHTTPCerts() at line 305 in "pkix_pl_aiamgr.c"
    [6] PKIX_PL_AIAMgr_GetAIACerts() at line 667 in "pkix_pl_aiamgr.c"
    [7] pkix_BuildForwardDepthFirstSearch() at line 2519 in "pkix_build.c"
    [8] pkix_Build_InitiateBuildChain() at line 4262 in "pkix_build.c"
    [9] PKIX_BuildChain() at line 4435 in "pkix_build.c"
    [10] CERT_PKIXVerifyCert() at line 2157 in "certvfypkix.c"
    [11] main() at line 506 in "vfychain.c"

Memory Leak (mel):
Found leaked block of size 27 bytes at address 0x72be8
At time of allocation, the call stack was:
    [1] PR_Malloc() at line 467 in "prmem.c"
    [2] PORT_Alloc_Util() at line 113 in "secport.c"
    [3] ocsp_ParseURL() at line 2768 in "ocsp.c"
    [4] CERT_ParseURL() at line 3242 in "ocsp.c"
    [5] pkix_pl_AIAMgr_GetHTTPCerts() at line 305 in "pkix_pl_aiamgr.c"
    [6] PKIX_PL_AIAMgr_GetAIACerts() at line 667 in "pkix_pl_aiamgr.c"
    [7] pkix_BuildForwardDepthFirstSearch() at line 2519 in "pkix_build.c"
    [8] pkix_Build_InitiateBuildChain() at line 4262 in "pkix_build.c"
    [9] PKIX_BuildChain() at line 4435 in "pkix_build.c"
    [10] CERT_PKIXVerifyCert() at line 2157 in "certvfypkix.c"
    [11] main() at line 506 in "vfychain.c"

Seems that there is some memory allocated when processing http path of cert to fetch, and this memory is not released later.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

This appears to be a leak in pkix_pl_AIAMgr_GetHTTPCerts.
It calls CERT_ParseURL, which outputs an allocated host name string.
There appear to be at least two error paths in that function that return
without freeing that string.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Alexei, I would propose to fix this as follows:

In the non-error case, after CERT_ParseURL returns the newly allocated copy
of hostname, pkix_pl_AIAMgr_GetHTTPCerts passes that copy to 
pkix_pl_HttpDefaultClient_CreateSessionFcn which passes it to
pkix_pl_HttpDefaultClient_CreateSession    which passes it to
pkix_pl_HttpDefaultClient_Create           which stores the address of
that copy in client->host.  See 
http://mxr.mozilla.org/security/source/security/nss/lib/libpkix/pkix_pl_nss/module/pkix_pl_httpdefaultclient.c#442

Any failure in that call path means that the copy wasn't saved. So one way
to fix it is to change this code in pkix_pl_AIAMgr_GetHTTPCerts as follows:

313                 PKIX_PL_NSSCALLRV
314                         (AIAMGR, rv, hcv1->createSessionFcn,
315                         (hostname, port, &serverSession));
316 
317                 if (rv != SECSuccess) {
   +                        PORT_Free(hostname); 
318                         PKIX_ERROR(PKIX_HTTPCLIENTCREATESESSIONFAILED);
319                 }

Another way to fix it would be to change pkix_pl_HttpDefaultClient_Create 
to always makes its own copy of hostname (e.g. strdup), and then change 
pkix_pl_AIAMgr_GetHTTPCerts to always free its copy before it returns.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

As an aside, I observe that all but one of the functions in the stack I 
described above follows the libPKIX calling conventions, returns a PKIX_ERROR 
*.  The one that does not is pkix_pl_HttpDefaultClient_CreateSessionFcn. 
It's quite unclear to me why we have one function in the middle of an 
otherwise all PKIX stack that does not follow the PKIX conventions.  
It can be argued that this leak was due to that inconsistency.  

Maybe hostname should be turned into a PKIX object of some kind, and should
be incref'ed in pkix_pl_HttpDefaultClient_Create and should be decref'ed at 
the end of pkix_pl_AIAMgr_GetHTTPCerts.

Comment 4

[Alexei Volkov]

Thank you Nelson for initial assessment. We have to similar leaks here: hostname and path. There are multiple paths where the code leaks these two allocations. In fact, there is only one path, that cleans the allocation of "path" and that is when hcv1->createFcn function invocation returns failure:

324                         PKIX_PL_NSSCALLRV
325                                 (AIAMGR, rv, hcv1->createFcn,
326                                 (serverSession,
327                                 "http",
328                                 path,
329                                 "GET",
330                                 PR_TicksPerSecond() * 60,
331                                 &requestSession));
332 
333                         if (rv != SECSuccess) {
334                                 if (path != NULL) {
335                                         PORT_Free(path);
336                                 }
337                                 PKIX_ERROR(PKIX_HTTPSERVERERROR);
338                         }

Again, all others paths when code finishes with success, failure or returns nbio leaks hostname and path. I believe that the reason we have the leak is due to an incorrect code adjustment to handle nbio. These two values are potentially needed when we return from a blocked call and trying again to call sendAndReceive function. 

In my opinion there is only one way to fix the problem and that is to copy both values to become properties of httpDefaultClient object and free them, when we done with the object. 

Patch is coming...

Comment 5

[Alexei Volkov]

Attachment: Patch v1. Leak fix.

patch makes http default client to create own copies of hostname and path and free them at the end of operations.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 342976 [details] [diff] [review]
Patch v1. Leak fix. 

This patch adds some strdup calls, but doesn't check the pointers returned
by strdup for NULL.

>+        /* "host" is a parsing result by CERT_GetURL function that adds
>+         * "end of line" to the value. OK to dup the string. */
>+        client->host = PORT_Strdup(host);
>         client->path = NULL;

>-        client->path = path_and_query_string;
>+        if (path_and_query_string) {
>+            /* "host" is a parsing result by CERT_GetURL function that adds
>+             * "end of line" to the value. OK to dup the string. */
>+            client->path = PORT_Strdup(path_and_query_string);
>+        }

I wonder, does libPKIX have string objects?  Should we be using string 
objects instead of mixing libpkix objects with c strings?
(The answer may well be: no.  I just don't know)

Comment 7

[Alexei Volkov]

(In reply to comment #3)
> As an aside, I observe that all but one of the functions in the stack I 
> described above follows the libPKIX calling conventions, returns a PKIX_ERROR 
> *.  The one that does not is pkix_pl_HttpDefaultClient_CreateSessionFcn. 
> It's quite unclear to me why we have one function in the middle of an 
> otherwise all PKIX stack that does not follow the PKIX conventions.  
> It can be argued that this leak was due to that inconsistency.  

pkix_pl_HttpDefaultClient_CreateSessionFcn is the libpkix implementation of the nss default http client defined in ocspt.h. It can not return PKIX_Error object. But the memory leak is not associated with this inconsistency.

Comment 8

[Alexei Volkov]

Attachment: Patch v2 Leak fix.

Adding check for dup memory. Return error in case of a failure.

There is string objects in libpkix. I don't think it is necessary to use them here, since it will bring only overhead of back and forth conversions. The only benefit on the string object is to handle encoding, which we don't have in our case.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 343433 [details] [diff] [review]
Patch v2 Leak fix.

r=nelson

Comment 10

[Alexei Volkov]

patch v2 is integrated.

Comment 11

[Alexei Volkov]

patch v2 is integrated.