* [Qemu-devel] [PATCH 1/3] libcacard: introduce new vcard_emul_logout
2014-10-19 2:12 [Qemu-devel] [PATCH 0/3] A few smartcard patches Ray Strode
@ 2014-10-19 2:12 ` Ray Strode
2014-10-19 2:12 ` [Qemu-devel] [PATCH 2/3] libcacard: Lock NSS cert db when selecting an applet on an emulated card Ray Strode
2014-10-19 2:12 ` [Qemu-devel] [PATCH 3/3] libcacard: don't free sign buffer while sign op is pending Ray Strode
2 siblings, 0 replies; 6+ messages in thread
From: Ray Strode @ 2014-10-19 2:12 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, Ray Strode, Michael Tokarev, Alon Levy,
Robert Relyea
From: Ray Strode <rstrode@redhat.com>
vcard_emul_reset currently only logs NSS out, but there is a TODO
for potentially sending insertion/removal events when powering down
or powering up.
For clarity, this commit moves the current guts of vcard_emul_reset to
a new vcard_emul_logout function which will never send insertion/removal
events. The vcard_emul_reset function now just calls vcard_emul_logout,
but also retains its TODO for watching power state transitions and sending
insertion/removal events.
Signed-off-by: Ray Strode <rstrode@redhat.com>
Reviewed-By: Robert Relyea <rrelyea@redhat.com>
Reviewed-By: Alon Levy <alevy@redhat.com>
---
libcacard/vcard_emul.h | 1 +
libcacard/vcard_emul_nss.c | 16 ++++++++++++----
2 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/libcacard/vcard_emul.h b/libcacard/vcard_emul.h
index 963563f..f09ee98 100644
--- a/libcacard/vcard_emul.h
+++ b/libcacard/vcard_emul.h
@@ -13,53 +13,54 @@
#ifndef VCARD_EMUL_H
#define VCARD_EMUL_H 1
#include "card_7816t.h"
#include "vcard.h"
#include "vcard_emul_type.h"
/*
* types
*/
typedef enum {
VCARD_EMUL_OK = 0,
VCARD_EMUL_FAIL,
/* return values by vcard_emul_init */
VCARD_EMUL_INIT_ALREADY_INITED,
} VCardEmulError;
/* options are emul specific. call card_emul_parse_args to change a string
* To an options struct */
typedef struct VCardEmulOptionsStruct VCardEmulOptions;
/*
* Login functions
*/
/* return the number of login attempts still possible on the card. if unknown,
* return -1 */
int vcard_emul_get_login_count(VCard *card);
/* login into the card, return the 7816 status word (sw2 || sw1) */
vcard_7816_status_t vcard_emul_login(VCard *card, unsigned char *pin,
int pin_len);
+void vcard_emul_logout(VCard *card);
/*
* key functions
*/
/* delete a key */
void vcard_emul_delete_key(VCardKey *key);
/* RSA sign/decrypt with the key, signature happens 'in place' */
vcard_7816_status_t vcard_emul_rsa_op(VCard *card, VCardKey *key,
unsigned char *buffer, int buffer_size);
void vcard_emul_reset(VCard *card, VCardPower power);
void vcard_emul_get_atr(VCard *card, unsigned char *atr, int *atr_len);
/* Re-insert of a card that has been removed by force removal */
VCardEmulError vcard_emul_force_card_insert(VReader *vreader);
/* Force a card removal even if the card is not physically removed */
VCardEmulError vcard_emul_force_card_remove(VReader *vreader);
VCardEmulOptions *vcard_emul_options(const char *args);
VCardEmulError vcard_emul_init(const VCardEmulOptions *options);
void vcard_emul_replay_insertion_events(void);
void vcard_emul_usage(void);
#endif
diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c
index 07b4464..53252a8 100644
--- a/libcacard/vcard_emul_nss.c
+++ b/libcacard/vcard_emul_nss.c
@@ -374,78 +374,86 @@ vcard_emul_login(VCard *card, unsigned char *pin, int pin_len)
if (!nss_emul_init) {
return VCARD7816_STATUS_ERROR_CONDITION_NOT_SATISFIED;
}
slot = vcard_emul_card_get_slot(card);
/* We depend on the PKCS #11 module internal login state here because we
* create a separate process to handle each guest instance. If we needed
* to handle multiple guests from one process, then we would need to keep
* a lot of extra state in our card structure
* */
pin_string = g_malloc(pin_len+1);
memcpy(pin_string, pin, pin_len);
pin_string[pin_len] = 0;
/* handle CAC expanded pins correctly */
for (i = pin_len-1; i >= 0 && (pin_string[i] == 0xff); i--) {
pin_string[i] = 0;
}
rv = PK11_Authenticate(slot, PR_FALSE, pin_string);
memset(pin_string, 0, pin_len); /* don't let the pin hang around in memory
to be snooped */
g_free(pin_string);
if (rv == SECSuccess) {
return VCARD7816_STATUS_SUCCESS;
}
/* map the error from port get error */
return VCARD7816_STATUS_ERROR_CONDITION_NOT_SATISFIED;
}
void
-vcard_emul_reset(VCard *card, VCardPower power)
+vcard_emul_logout(VCard *card)
{
PK11SlotInfo *slot;
if (!nss_emul_init) {
return;
}
+ slot = vcard_emul_card_get_slot(card);
+ if (PK11_IsLoggedIn(slot,NULL)) {
+ PK11_Logout(slot); /* NOTE: ignoring SECStatus return value */
+ }
+}
+
+void
+vcard_emul_reset(VCard *card, VCardPower power)
+{
/*
* if we reset the card (either power on or power off), we lose our login
* state
*/
+ vcard_emul_logout(card);
+
/* TODO: we may also need to send insertion/removal events? */
- slot = vcard_emul_card_get_slot(card);
- PK11_Logout(slot); /* NOTE: ignoring SECStatus return value */
}
-
static VReader *
vcard_emul_find_vreader_from_slot(PK11SlotInfo *slot)
{
VReaderList *reader_list = vreader_get_reader_list();
VReaderListEntry *current_entry;
if (reader_list == NULL) {
return NULL;
}
for (current_entry = vreader_list_get_first(reader_list); current_entry;
current_entry = vreader_list_get_next(current_entry)) {
VReader *reader = vreader_list_get_reader(current_entry);
VReaderEmul *reader_emul = vreader_get_private(reader);
if (reader_emul->slot == slot) {
vreader_list_delete(reader_list);
return reader;
}
vreader_free(reader);
}
vreader_list_delete(reader_list);
return NULL;
}
/*
* create a new reader emul
*/
static VReaderEmul *
vreader_emul_new(PK11SlotInfo *slot, VCardEmulType type, const char *params)
{
--
2.1.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [Qemu-devel] [PATCH 2/3] libcacard: Lock NSS cert db when selecting an applet on an emulated card
2014-10-19 2:12 [Qemu-devel] [PATCH 0/3] A few smartcard patches Ray Strode
2014-10-19 2:12 ` [Qemu-devel] [PATCH 1/3] libcacard: introduce new vcard_emul_logout Ray Strode
@ 2014-10-19 2:12 ` Ray Strode
2014-10-19 2:12 ` [Qemu-devel] [PATCH 3/3] libcacard: don't free sign buffer while sign op is pending Ray Strode
2 siblings, 0 replies; 6+ messages in thread
From: Ray Strode @ 2014-10-19 2:12 UTC (permalink / raw)
To: qemu-devel
Cc: Marc-André Lureau, Ray Strode, Michael Tokarev, Alon Levy,
Robert Relyea
From: Ray Strode <rstrode@redhat.com>
When a process in a guest uses an emulated smartcard, libcacard running
on the host passes the PIN from the guest to the PK11_Authenticate NSS
function. The first time PK11_Authenticate is called the passed in PIN
is used to unlock the certificate database. Subsequent calls to
PK11_Authenticate will transparently succeed, regardless of the passed in
PIN. This is a convenience for applications provided by NSS.
Of course, the guest may have many applications using the one emulated
smart card all driven from the same host QEMU process. That means if a
user enters the right PIN in one program in the guest, and then enters the
wrong PIN in another program in the guest, the wrong PIN will still
successfully unlock the virtual smartcard.
This commit forces the NSS certificate database to be locked anytime an
applet is selected on an emulated smartcard by calling vcard_emul_logout.
Signed-off-by: Ray Strode <rstrode@redhat.com>
Reviewed-By: Robert Relyea <rrelyea@redhat.com>
Reviewed-By: Alon Levy <alevy@redhat.com>
---
libcacard/vcard.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/libcacard/vcard.c b/libcacard/vcard.c
index 87ad516..d140a8e 100644
--- a/libcacard/vcard.c
+++ b/libcacard/vcard.c
@@ -223,60 +223,65 @@ vcard_find_applet(VCard *card, unsigned char *aid, int aid_len)
{
VCardApplet *current_applet;
for (current_applet = card->applet_list; current_applet;
current_applet = current_applet->next) {
if (current_applet->aid_len != aid_len) {
continue;
}
if (memcmp(current_applet->aid, aid, aid_len) == 0) {
break;
}
}
return current_applet;
}
unsigned char *
vcard_applet_get_aid(VCardApplet *applet, int *aid_len)
{
if (applet == NULL) {
return NULL;
}
*aid_len = applet->aid_len;
return applet->aid;
}
void
vcard_select_applet(VCard *card, int channel, VCardApplet *applet)
{
assert(channel < MAX_CHANNEL);
+
+ /* If using an emulated card, make sure to log out of any already logged in
+ * session. */
+ vcard_emul_logout(card);
+
card->current_applet[channel] = applet;
/* reset the applet */
if (applet && applet->reset_applet) {
applet->reset_applet(card, channel);
}
}
VCardAppletPrivate *
vcard_get_current_applet_private(VCard *card, int channel)
{
VCardApplet *applet = card->current_applet[channel];
if (applet == NULL) {
return NULL;
}
return applet->applet_private;
}
VCardStatus
vcard_process_applet_apdu(VCard *card, VCardAPDU *apdu,
VCardResponse **response)
{
if (card->current_applet[apdu->a_channel]) {
return card->current_applet[apdu->a_channel]->process_apdu(
card, apdu, response);
}
return VCARD_NEXT;
}
/*
--
2.1.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [Qemu-devel] [PATCH 3/3] libcacard: don't free sign buffer while sign op is pending
2014-10-19 2:12 [Qemu-devel] [PATCH 0/3] A few smartcard patches Ray Strode
2014-10-19 2:12 ` [Qemu-devel] [PATCH 1/3] libcacard: introduce new vcard_emul_logout Ray Strode
2014-10-19 2:12 ` [Qemu-devel] [PATCH 2/3] libcacard: Lock NSS cert db when selecting an applet on an emulated card Ray Strode
@ 2014-10-19 2:12 ` Ray Strode
2014-10-19 6:23 ` Alon Levy
2 siblings, 1 reply; 6+ messages in thread
From: Ray Strode @ 2014-10-19 2:12 UTC (permalink / raw)
To: qemu-devel
Cc: Ray Strode, Robert Relyea, Michael Tokarev, Alon Levy,
Marc-André Lureau, Nalin Dahyabhai
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.
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:
--
2.1.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH 3/3] libcacard: don't free sign buffer while sign op is pending
2014-10-19 2:12 ` [Qemu-devel] [PATCH 3/3] libcacard: don't free sign buffer while sign op is pending Ray Strode
@ 2014-10-19 6:23 ` Alon Levy
2014-10-20 18:40 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: Alon Levy @ 2014-10-19 6:23 UTC (permalink / raw)
To: Ray Strode, qemu-devel
Cc: Marc-André Lureau, Ray Strode, Michael Tokarev,
Robert Relyea, Nalin Dahyabhai
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:
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH 3/3] libcacard: don't free sign buffer while sign op is pending
2014-10-19 6:23 ` Alon Levy
@ 2014-10-20 18:40 ` Paolo Bonzini
0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2014-10-20 18:40 UTC (permalink / raw)
To: Alon Levy, Ray Strode, qemu-devel
Cc: Ray Strode, Michael Tokarev, qemu-stable, Nalin Dahyabhai,
Marc-André Lureau, Robert Relyea
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:
>>
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread