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

[Paolo Bonzini]

On 10/19/2014 08:23 AM, Alon Levy wrote:
> On 10/19/2014 05:12 AM, Ray Strode wrote:
> > From: Ray Strode <address@hidden>
> >
> > 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 <address@hidden>

Please Cc address@hidden if you send a pull request.

Paolo

> > Signed-off-by: Ray Strode <address@hidden>
> > ---
> >   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: