[kmf-discuss] bug in OpenSSL_FindCert() ?

[Wyllys Ingersoll]

Jürgen Keil wrote:
> While debugging the libelfsign.so / SUNWObjectCA / MD5 checksum issue
> I found this piece of code in OpenSSL_FindCert(), which seems to be buggy:
> 
> http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/lib/libkmf/plugins/kmf_openssl/common/openssl_spi.c#992
> 
>    1051 			/* If load succeeds, add certdata to the list */
>    1052 			if (kmf_cert != NULL) {
>    1053 				for (i = 0; i < loaded_certs &&
>    1054 				    i < maxcerts; i++) {
>    1055 					kmf_cert[n].certificate.Data =
>    1056 						certlist[i].Data;
>    1057 					kmf_cert[n].certificate.Length =
>    1058 						certlist[i].Length;
>    1059 
>    1060 					kmf_cert[n].kmf_private.keystore_type =
>    1061 						KMF_KEYSTORE_OPENSSL;
>    1062 					kmf_cert[n].kmf_private.flags =
>    1063 						KMF_FLAG_CERT_VALID;
>    1064 					kmf_cert[n].kmf_private.label =
>    1065 						strdup(fname);
>    1066 					n++;
>    1067 				}
>    1068 				/* If maxcerts < loaded_certs, clean up */
>    1069 				for (; i < loaded_certs; i++)
>    1070 					KMF_FreeData(&certlist[i]);
>    1071 			} else {
> 
> 
> Shouldn't the for loop test ``n < maxcerts'' at line 1054 ?
> With the current code we could overflow the kmf_cert array... 
> 
> And the comment ``If maxcerts < loaded_certs, clean up'' doesn't make 
> much sense because the code always cleans up all loaded certs...

The point of that comment and the code that follows is to clean up the
memory associated with certs that were not added to the kmf_cert list.
I'll try to clarify the comment when I fix the other bug you noticed.

Bug 6549186 has been filed, it will appear on opensolaris in the next 24 hrs.

-Wyllys