From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49801) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xfjtt-0003dZ-82 for qemu-devel@nongnu.org; Sun, 19 Oct 2014 02:23:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xfjtn-0006zH-NX for qemu-devel@nongnu.org; Sun, 19 Oct 2014 02:23:33 -0400 Received: from pb-smtp1.int.icgroup.com ([208.72.237.35]:52878 helo=sasl.smtp.pobox.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xfjtn-0006yj-HV for qemu-devel@nongnu.org; Sun, 19 Oct 2014 02:23:27 -0400 Message-ID: <544358DA.6040000@pobox.com> Date: Sun, 19 Oct 2014 09:23:22 +0300 From: Alon Levy MIME-Version: 1.0 References: In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/3] libcacard: don't free sign buffer while sign op is pending List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ray Strode , qemu-devel@nongnu.org Cc: =?UTF-8?B?TWFyYy1BbmRyw6kgTHVyZWF1?= , Ray Strode , Michael Tokarev , Robert Relyea , Nalin Dahyabhai On 10/19/2014 05:12 AM, Ray Strode wrote: > From: Ray Strode > > 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 > > Signed-off-by: Ray Strode > --- > 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: >