Re: [Qemu-devel] [PATCH 3/3] libcacard: don't free sign buffer while sign op is pending

[Paolo Bonzini <pbonzini@redhat.com>]

On 10/19/2014 08:23 AM, Alon Levy wrote:
> On 10/19/2014 05:12 AM, Ray Strode wrote:
>> From: Ray Strode <rstrode@redhat.com>
>>
>> commit 57f97834efe0c208ffadc9d2959f3d3d55580e52 cleaned up
>> the cac_applet_pki_process_apdu function to have a single
>> exit point. Unfortunately, that commit introduced a bug
>> where the sign buffer can get free'd and nullified while
>> it's still being used.
>>
>> This commit corrects the bug by introducing a boolean to
>> track whether or not the sign buffer should be freed in
>> the function exit path.
>
> My bad, thanks for catching this.
>
> Reviewed-by: Alon Levy <alon@pobox.com>

Please Cc qemu-stable@nongnu.org if you send a pull request.

Paolo

>
>>
>> Signed-off-by: Ray Strode <rstrode@redhat.com>
>> ---
>>   libcacard/cac.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/libcacard/cac.c b/libcacard/cac.c
>> index ae8c378..f38fdce 100644
>> --- a/libcacard/cac.c
>> +++ b/libcacard/cac.c
>> @@ -88,60 +88,61 @@ cac_common_process_apdu(VCard *card, VCardAPDU *apdu, VCardResponse **response)
>>   }
>>
>>   /*
>>    *  reset the inter call state between applet selects
>>    */
>>   static VCardStatus
>>   cac_applet_pki_reset(VCard *card, int channel)
>>   {
>>       VCardAppletPrivate *applet_private;
>>       CACPKIAppletData *pki_applet;
>>       applet_private = vcard_get_current_applet_private(card, channel);
>>       assert(applet_private);
>>       pki_applet = &(applet_private->u.pki_data);
>>
>>       pki_applet->cert_buffer = NULL;
>>       g_free(pki_applet->sign_buffer);
>>       pki_applet->sign_buffer = NULL;
>>       pki_applet->cert_buffer_len = 0;
>>       pki_applet->sign_buffer_len = 0;
>>       return VCARD_DONE;
>>   }
>>
>>   static VCardStatus
>>   cac_applet_pki_process_apdu(VCard *card, VCardAPDU *apdu,
>>                               VCardResponse **response)
>>   {
>>       CACPKIAppletData *pki_applet;
>>       VCardAppletPrivate *applet_private;
>>       int size, next;
>>       unsigned char *sign_buffer;
>> +    bool retain_sign_buffer = FALSE;
>>       vcard_7816_status_t status;
>>       VCardStatus ret = VCARD_FAIL;
>>
>>       applet_private = vcard_get_current_applet_private(card, apdu->a_channel);
>>       assert(applet_private);
>>       pki_applet = &(applet_private->u.pki_data);
>>
>>       switch (apdu->a_ins) {
>>       case CAC_UPDATE_BUFFER:
>>           *response = vcard_make_response(
>>               VCARD7816_STATUS_ERROR_CONDITION_NOT_SATISFIED);
>>           ret = VCARD_DONE;
>>           break;
>>       case CAC_GET_CERTIFICATE:
>>           if ((apdu->a_p2 != 0) || (apdu->a_p1 != 0)) {
>>               *response = vcard_make_response(
>>                                VCARD7816_STATUS_ERROR_P1_P2_INCORRECT);
>>               break;
>>           }
>>           assert(pki_applet->cert != NULL);
>>           size = apdu->a_Le;
>>           if (pki_applet->cert_buffer == NULL) {
>>               pki_applet->cert_buffer = pki_applet->cert;
>>               pki_applet->cert_buffer_len = pki_applet->cert_len;
>>           }
>>           size = MIN(size, pki_applet->cert_buffer_len);
>>           next = MIN(255, pki_applet->cert_buffer_len - size);
>>           *response = vcard_response_new_bytes(
>>                           card, pki_applet->cert_buffer, size,
>>                           apdu->a_Le, next ?
>> @@ -151,85 +152,88 @@ cac_applet_pki_process_apdu(VCard *card, VCardAPDU *apdu,
>>           pki_applet->cert_buffer += size;
>>           pki_applet->cert_buffer_len -= size;
>>           if ((*response == NULL) || (next == 0)) {
>>               pki_applet->cert_buffer = NULL;
>>           }
>>           if (*response == NULL) {
>>               *response = vcard_make_response(
>>                               VCARD7816_STATUS_EXC_ERROR_MEMORY_FAILURE);
>>           }
>>           ret = VCARD_DONE;
>>           break;
>>       case CAC_SIGN_DECRYPT:
>>           if (apdu->a_p2 != 0) {
>>               *response = vcard_make_response(
>>                                VCARD7816_STATUS_ERROR_P1_P2_INCORRECT);
>>               break;
>>           }
>>           size = apdu->a_Lc;
>>
>>           sign_buffer = g_realloc(pki_applet->sign_buffer,
>>                                   pki_applet->sign_buffer_len + size);
>>           memcpy(sign_buffer+pki_applet->sign_buffer_len, apdu->a_body, size);
>>           size += pki_applet->sign_buffer_len;
>>           switch (apdu->a_p1) {
>>           case  0x80:
>>               /* p1 == 0x80 means we haven't yet sent the whole buffer, wait for
>>                * the rest */
>>               pki_applet->sign_buffer = sign_buffer;
>>               pki_applet->sign_buffer_len = size;
>>               *response = vcard_make_response(VCARD7816_STATUS_SUCCESS);
>> +            retain_sign_buffer = TRUE;
>>               break;
>>           case 0x00:
>>               /* we now have the whole buffer, do the operation, result will be
>>                * in the sign_buffer */
>>               status = vcard_emul_rsa_op(card, pki_applet->key,
>>                                          sign_buffer, size);
>>               if (status != VCARD7816_STATUS_SUCCESS) {
>>                   *response = vcard_make_response(status);
>>                   break;
>>               }
>>               *response = vcard_response_new(card, sign_buffer, size, apdu->a_Le,
>>                                                        VCARD7816_STATUS_SUCCESS);
>>               if (*response == NULL) {
>>                   *response = vcard_make_response(
>>                                   VCARD7816_STATUS_EXC_ERROR_MEMORY_FAILURE);
>>               }
>>               break;
>>           default:
>>              *response = vcard_make_response(
>>                                   VCARD7816_STATUS_ERROR_P1_P2_INCORRECT);
>>               break;
>>           }
>> -        g_free(sign_buffer);
>> -        pki_applet->sign_buffer = NULL;
>> -        pki_applet->sign_buffer_len = 0;
>> +        if (!retain_sign_buffer) {
>> +            g_free(sign_buffer);
>> +            pki_applet->sign_buffer = NULL;
>> +            pki_applet->sign_buffer_len = 0;
>> +        }
>>           ret = VCARD_DONE;
>>           break;
>>       case CAC_READ_BUFFER:
>>           /* new CAC call, go ahead and use the old version for now */
>>           /* TODO: implement */
>>           *response = vcard_make_response(
>>                                   VCARD7816_STATUS_ERROR_COMMAND_NOT_SUPPORTED);
>>           ret = VCARD_DONE;
>>           break;
>>       default:
>>           ret = cac_common_process_apdu(card, apdu, response);
>>           break;
>>       }
>>       return ret;
>>   }
>>
>>
>>   static VCardStatus
>>   cac_applet_id_process_apdu(VCard *card, VCardAPDU *apdu,
>>                              VCardResponse **response)
>>   {
>>       VCardStatus ret = VCARD_FAIL;
>>
>>       switch (apdu->a_ins) {
>>       case CAC_UPDATE_BUFFER:
>>           *response = vcard_make_response(
>>                           VCARD7816_STATUS_ERROR_CONDITION_NOT_SATISFIED);
>>           ret = VCARD_DONE;
>>           break;
>>       case CAC_READ_BUFFER:
>>
>
>
>