[Pki-devel] Review Request: TMS ECC Key Recovery

Christina Fu cfu at redhat.com
Thu Sep 27 17:56:18 UTC 2012


Hi Jack,

Thanks for the review.  Please find my response in-line below.

thanks,
Christina

On 09/26/2012 06:39 PM, John Magne wrote:
> Couple Questions:
>
> In the following piece of the patch:
>
> +            SECITEM_FreeItem(&der, PR_FALSE);
> 81	                        SECKEY_DestroySubjectPublicKeyInfo(spki);
> 82	
>
> It looks like both are executed , even when the call to
>     spki = SECKEY_DecodeDERSubjectPublicKeyInfo(&der);
>
> Fails and spki is NULL. Does the DestroySubjectPublicKeyInfo handle this? Also Will "der" be in a state to harm the call to SECITEM_FreeItem?
>

DestroySubjectPublicKeyInfo checks before it frees so it is okay if spki happens to be null:

void
SECKEY_DestroySubjectPublicKeyInfo(CERTSubjectPublicKeyInfo *spki)
{
     if (spki && spki->arena) {
     PORT_FreeArena(spki->arena, PR_FALSE);
     }
}

Also, the content of der is copied over in 
SECKEY_DecodeDERSubjectPublicKeyInfo(), so once the call is done, the 
code has no use for der any more, hence the SECITEM_FreeItem after.
Also, SECITEM_FreeItem checks for null before it frees as well, so it's 
safe to call.
>
> Index: tps/src/engine/RA.cpp
> 187	===================================================================
> 188	--- tps/src/engine/RA.cpp       (revision 2497)
> 189	+++ tps/src/engine/RA.cpp       (working copy)
> 190	@@ -1238,7 +1238,10 @@
> 191	        goto loser;
> 192	       } else {
> 193	        RA::Debug(LL_PER_PDU, "RecoverKey", "got public key =%s", tmp);
> 194	-       *publicKey_s  = PL_strdup(tmp);
> 195	+          char *tmp_publicKey_s  = PL_strdup(tmp);
> 196	+          Buffer *decodePubKey = Util::URLDecode(tmp_publicKey_s);
> 197	+          *publicKey_s =
> 198	+              BTOA_DataToAscii(decodePubKey->getBuf(), decodePubKey->getLen());
> 199	       }
> 200	
> 201	       tmp = NULL;
> 202	@@ -1256,7 +1259,7 @@
> 203	           RA::Error(LL_PER_PDU, "RecoverKey",
> 204	               "did not get iv_param for recovered  key in DRM response");
> 205	       } else {
> 206	-          RA::Debug(LL_PER_PDU, "ServerSideKeyGen", "got iv_param for recovered key =%s", tmp);
> 207	+          RA::Debug(LL_PER_PDU, "RecoverKey", "got iv_param for recovered key =%s", tmp);
> 208	           *ivParam_s  = PL_strdup(tmp);
> 209	       }
>
> In the above code we are doing a strdup giving publicKey_s.
>
> Are we freeing that string anywhere?
>
I introduced two variables there.  Yes, I should free them.
>
>
>
>
> ----- Original Message -----
> From: "Christina Fu"<cfu at redhat.com>
> To: "pki-devel"<pki-devel at redhat.com>
> Sent: Monday, September 24, 2012 4:06:32 PM
> Subject: [Pki-devel] Review Request: TMS ECC Key Recovery
>
> https://fedorahosted.org/pki/ticket/252  - TMS - ECC Key Recovery
>
> patch for review:
> https://fedorahosted.org/pki/attachment/ticket/252/TMS-ECC-Recovery.patchForReview
>
> This patch provides code to allow ECC key recovery in the TMS environment.
> It was tested to work with tpsclient.  The key injection part of
> implementation for the actual smart card tokens is scheduled to be done
> in  #235 at a later time.
>
> I can do a demo tomorrow in office.
>
> thanks,
> Christina
>
> _______________________________________________
> Pki-devel mailing list
> Pki-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel




More information about the Pki-devel mailing list