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

[Alon Levy <alon@pobox.com>]

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>

> 
> 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:
>