From 8ea328ff7fb311e582f312fe4f145c1af0d52390 Mon Sep 17 00:00:00 2001 From: vletoux Date: Sun, 29 Mar 2015 15:55:10 +0200 Subject: [PATCH] Minor code quality improvements. Basically checks that the memory allocation succeed. The ctbcs.c change improve the readability because count = 0 and len > 254 does not add any value. VTA: added few coding style changes --- src/libopensc/card-openpgp.c | 12 +++++- src/libopensc/ctbcs.c | 4 +- src/libopensc/muscle-filesystem.c | 2 + src/libopensc/pkcs15-gemsafeV1.c | 65 ++++++++++++++++--------------- src/libopensc/pkcs15-pubkey.c | 4 ++ src/pkcs11/mechanism.c | 3 ++ 6 files changed, 54 insertions(+), 36 deletions(-) diff --git a/src/libopensc/card-openpgp.c b/src/libopensc/card-openpgp.c index 5ae66f67..6c63a5e8 100644 --- a/src/libopensc/card-openpgp.c +++ b/src/libopensc/card-openpgp.c @@ -305,7 +305,7 @@ pgp_init(sc_card_t *card) sc_file_t *file = NULL; struct do_info *info; int r; - struct blob *child = NULL; + struct blob *child = NULL; priv = calloc (1, sizeof *priv); if (!priv) @@ -330,8 +330,14 @@ pgp_init(sc_card_t *card) return r; } + /* defensive programming check */ + if (!file) { + pgp_finish(card); + return SC_ERROR_OBJECT_NOT_FOUND; + } + /* read information from AID */ - if (file && file->namelen == 16) { + if (file->namelen == 16) { /* OpenPGP card spec 1.1 & 2.0, section 4.2.1 & 4.1.2.1 */ priv->bcd_version = bebytes2ushort(file->name + 6); /* kludge: get card's serial number from manufacturer ID + serial number */ @@ -2166,7 +2172,9 @@ out: /* ABI: card ctl: perform special card-specific operations */ static int pgp_card_ctl(sc_card_t *card, unsigned long cmd, void *ptr) { +#ifdef ENABLE_OPENSSL int r; +#endif /* ENABLE_OPENSSL */ LOG_FUNC_CALLED(card->ctx); diff --git a/src/libopensc/ctbcs.c b/src/libopensc/ctbcs.c index 16c43369..ad51cac8 100644 --- a/src/libopensc/ctbcs.c +++ b/src/libopensc/ctbcs.c @@ -58,7 +58,7 @@ ctbcs_build_perform_verification_apdu(sc_apdu_t *apdu, struct sc_pin_cmd_data *d prompt = data->pin1.prompt; if (prompt && *prompt) { len = strlen(prompt); - if (count + len + 2 > buflen || len > 254) + if (len + 2 > buflen) return SC_ERROR_BUFFER_TOO_SMALL; buf[count++] = CTBCS_TAG_PROMPT; buf[count++] = len; @@ -126,7 +126,7 @@ ctbcs_build_modify_verification_apdu(sc_apdu_t *apdu, struct sc_pin_cmd_data *da prompt = data->pin1.prompt; if (prompt && *prompt) { len = strlen(prompt); - if (count + len + 2 > buflen || len > 254) + if (len + 2 > buflen) return SC_ERROR_BUFFER_TOO_SMALL; buf[count++] = CTBCS_TAG_PROMPT; buf[count++] = len; diff --git a/src/libopensc/muscle-filesystem.c b/src/libopensc/muscle-filesystem.c index f54d263f..dc618a67 100644 --- a/src/libopensc/muscle-filesystem.c +++ b/src/libopensc/muscle-filesystem.c @@ -42,6 +42,8 @@ static const u8* ignoredFiles[] = { mscfs_t *mscfs_new(void) { mscfs_t *fs = malloc(sizeof(mscfs_t)); + if (!fs) + return NULL; memset(fs, 0, sizeof(mscfs_t)); memcpy(fs->currentPath, "\x3F\x00", 2); return fs; diff --git a/src/libopensc/pkcs15-gemsafeV1.c b/src/libopensc/pkcs15-gemsafeV1.c index aa928421..c1191eef 100644 --- a/src/libopensc/pkcs15-gemsafeV1.c +++ b/src/libopensc/pkcs15-gemsafeV1.c @@ -183,11 +183,9 @@ static int gemsafe_get_cert_len(sc_card_t *card) * (allocated EF space is much greater!) */ objlen = (((size_t) ibuf[0]) << 8) | ibuf[1]; - sc_debug(card->ctx, SC_LOG_DEBUG_NORMAL, - "Stored object is of size: %d\n", objlen); + sc_log(card->ctx, "Stored object is of size: %d", objlen); if (objlen < 1 || objlen > GEMSAFE_MAX_OBJLEN) { - sc_debug(card->ctx, SC_LOG_DEBUG_NORMAL, - "Invalid object size: %d\n", objlen); + sc_log(card->ctx, "Invalid object size: %d", objlen); return SC_ERROR_INTERNAL; } @@ -209,15 +207,14 @@ static int gemsafe_get_cert_len(sc_card_t *card) while (ibuf[ind] == 0x01) { if (ibuf[ind+1] == 0xFE) { gemsafe_prkeys[i].ref = ibuf[ind+4]; - sc_debug(card->ctx, SC_LOG_DEBUG_NORMAL, - "Key container %d is allocated and uses key_ref %d\n", i+1, - gemsafe_prkeys[i].ref); + sc_log(card->ctx, "Key container %d is allocated and uses key_ref %d", + i+1, gemsafe_prkeys[i].ref); ind += 9; - } else { + } + else { gemsafe_prkeys[i].label = NULL; gemsafe_cert[i].label = NULL; - sc_debug(card->ctx, SC_LOG_DEBUG_NORMAL, - "Key container %d is unallocated\n", i+1); + sc_log(card->ctx, "Key container %d is unallocated", i+1); ind += 8; } i++; @@ -239,8 +236,7 @@ static int gemsafe_get_cert_len(sc_card_t *card) r = sc_read_binary(card, iptr - ibuf, iptr, MIN(GEMSAFE_READ_QUANTUM, objlen - (iptr - ibuf)), 0); if (r < 0) { - sc_debug(card->ctx, SC_LOG_DEBUG_NORMAL, - "Could not read cert object\n"); + sc_log(card->ctx, "Could not read cert object"); return SC_ERROR_INTERNAL; } iptr += GEMSAFE_READ_QUANTUM; @@ -254,15 +250,12 @@ static int gemsafe_get_cert_len(sc_card_t *card) while (i < gemsafe_cert_max && gemsafe_cert[i].label == NULL) i++; if (i == gemsafe_cert_max) { - sc_debug(card->ctx, SC_LOG_DEBUG_NORMAL, - "Warning: Found orphaned certificate at offset %d\n", ind); + sc_log(card->ctx, "Warning: Found orphaned certificate at offset %d", ind); return SC_SUCCESS; } /* DER cert len is encoded this way */ certlen = ((((size_t) ibuf[ind+2]) << 8) | ibuf[ind+3]) + 4; - sc_debug(card->ctx, SC_LOG_DEBUG_NORMAL, - "Found certificate of key container %d at offset %d, len %d\n", - i+1, ind, certlen); + sc_log(card->ctx, "Found certificate of key container %d at offset %d, len %d", i+1, ind, certlen); gemsafe_cert[i].index = ind; gemsafe_cert[i].count = certlen; ind += certlen; @@ -276,8 +269,7 @@ static int gemsafe_get_cert_len(sc_card_t *card) */ for (; i < gemsafe_cert_max; i++) { if (gemsafe_cert[i].label) { - sc_debug(card->ctx, SC_LOG_DEBUG_NORMAL, - "Warning: Certificate of key container %d is missing\n", i+1); + sc_log(card->ctx, "Warning: Certificate of key container %d is missing", i+1); gemsafe_prkeys[i].label = NULL; gemsafe_cert[i].label = NULL; } @@ -304,7 +296,7 @@ static int sc_pkcs15emu_gemsafeV1_init( sc_pkcs15_card_t *p15card) struct sc_apdu apdu; u8 rbuf[SC_MAX_APDU_BUFFER_SIZE]; - sc_debug(p15card->card->ctx, SC_LOG_DEBUG_NORMAL, "Setting pkcs15 parameters\n"); + sc_log(p15card->card->ctx, "Setting pkcs15 parameters"); if (p15card->tokeninfo->label) free(p15card->tokeninfo->label); @@ -330,7 +322,8 @@ static int sc_pkcs15emu_gemsafeV1_init( sc_pkcs15_card_t *p15card) apdu.lc = 0; apdu.datalen = 0; r = sc_transmit_apdu(card, &apdu); - SC_TEST_RET(card->ctx, SC_LOG_DEBUG_NORMAL, r, "APDU transmit failed"); + LOG_TEST_RET(card->ctx, r, "APDU transmit failed"); + if (apdu.sw1 != 0x90 || apdu.sw2 != 0x00) return SC_ERROR_INTERNAL; if (r != SC_SUCCESS) @@ -350,7 +343,7 @@ static int sc_pkcs15emu_gemsafeV1_init( sc_pkcs15_card_t *p15card) return SC_ERROR_INTERNAL; /* set certs */ - sc_debug(p15card->card->ctx, SC_LOG_DEBUG_NORMAL, "Setting certificates\n"); + sc_log(p15card->card->ctx, "Setting certificates"); for (i = 0; i < gemsafe_cert_max; i++) { struct sc_pkcs15_id p15Id; struct sc_path path; @@ -367,7 +360,7 @@ static int sc_pkcs15emu_gemsafeV1_init( sc_pkcs15_card_t *p15card) } /* set gemsafe_pin */ - sc_debug(p15card->card->ctx, SC_LOG_DEBUG_NORMAL, "Setting PIN\n"); + sc_log(p15card->card->ctx, "Setting PIN"); for (i=0; i < gemsafe_pin_max; i++) { struct sc_pkcs15_id p15Id; struct sc_path path; @@ -388,11 +381,11 @@ static int sc_pkcs15emu_gemsafeV1_init( sc_pkcs15_card_t *p15card) }; /* set private keys */ - sc_debug(p15card->card->ctx, SC_LOG_DEBUG_NORMAL, "Setting private keys\n"); + sc_log(p15card->card->ctx, "Setting private keys"); for (i = 0; i < gemsafe_cert_max; i++) { struct sc_pkcs15_id p15Id, authId, *pauthId; struct sc_path path; - int key_ref = 0x03; + int key_ref = 0x03; if (gemsafe_prkeys[i].label == NULL) continue; @@ -403,7 +396,7 @@ static int sc_pkcs15emu_gemsafeV1_init( sc_pkcs15_card_t *p15card) } else pauthId = NULL; sc_format_path(gemsafe_prkeys[i].path, &path); - /* + /* * The key ref may be different for different sites; * by adding flags=n where the low order 4 bits can be * the key ref we can force it. @@ -423,7 +416,7 @@ static int sc_pkcs15emu_gemsafeV1_init( sc_pkcs15_card_t *p15card) } /* select the application DF */ - sc_debug(p15card->card->ctx, SC_LOG_DEBUG_NORMAL,"Selecting application DF\n"); + sc_log(p15card->card->ctx, "Selecting application DF"); sc_format_path(GEMSAFE_APP_PATH, &path); r = sc_select_file(card, &path, &file); if (r != SC_SUCCESS || !file) @@ -511,8 +504,7 @@ sc_pkcs15emu_add_object(sc_pkcs15_card_t *p15card, int type, df_type = SC_PKCS15_CDF; break; default: - sc_debug(p15card->card->ctx, SC_LOG_DEBUG_NORMAL, - "Unknown PKCS15 object type %d\n", type); + sc_log(p15card->card->ctx, "Unknown PKCS15 object type %d", type); free(obj); return SC_ERROR_INVALID_ARGUMENTS; } @@ -534,6 +526,9 @@ sc_pkcs15emu_add_pin(sc_pkcs15_card_t *p15card, sc_pkcs15_auth_info_t *info; info = calloc(1, sizeof(*info)); + if (!info) + LOG_FUNC_RETURN(p15card->card->ctx, SC_ERROR_OUT_OF_MEMORY); + info->auth_type = SC_PKCS15_PIN_AUTH_TYPE_PIN; info->auth_method = SC_AC_CHV; info->auth_id = *id; @@ -549,9 +544,7 @@ sc_pkcs15emu_add_pin(sc_pkcs15_card_t *p15card, if (path) info->path = *path; - return sc_pkcs15emu_add_object(p15card, - SC_PKCS15_TYPE_AUTH_PIN, - label, info, NULL, obj_flags); + return sc_pkcs15emu_add_object(p15card, SC_PKCS15_TYPE_AUTH_PIN, label, info, NULL, obj_flags); } static int @@ -563,6 +556,10 @@ sc_pkcs15emu_add_cert(sc_pkcs15_card_t *p15card, { sc_pkcs15_cert_info_t *info; info = calloc(1, sizeof(*info)); + if (!info) + { + LOG_FUNC_RETURN(p15card->card->ctx, SC_ERROR_OUT_OF_MEMORY); + } info->id = *id; info->authority = authority; if (path) @@ -582,6 +579,10 @@ sc_pkcs15emu_add_prkey(sc_pkcs15_card_t *p15card, sc_pkcs15_prkey_info_t *info; info = calloc(1, sizeof(*info)); + if (!info) + { + LOG_FUNC_RETURN(p15card->card->ctx, SC_ERROR_OUT_OF_MEMORY); + } info->id = *id; info->modulus_length = modulus_length; info->usage = usage; diff --git a/src/libopensc/pkcs15-pubkey.c b/src/libopensc/pkcs15-pubkey.c index 189e413e..9cf75e35 100644 --- a/src/libopensc/pkcs15-pubkey.c +++ b/src/libopensc/pkcs15-pubkey.c @@ -1019,6 +1019,8 @@ sc_pkcs15_pubkey_from_prvkey(struct sc_context *ctx, struct sc_pkcs15_prkey *prv break; case SC_ALGORITHM_EC: pubkey->u.ec.ecpointQ.value = malloc(prvkey->u.ec.ecpointQ.len); + if (!pubkey->u.ec.ecpointQ.value) + LOG_FUNC_RETURN(ctx, SC_ERROR_OUT_OF_MEMORY); memcpy(pubkey->u.ec.ecpointQ.value, prvkey->u.ec.ecpointQ.value, prvkey->u.ec.ecpointQ.len); pubkey->u.ec.ecpointQ.len = prvkey->u.ec.ecpointQ.len; break; @@ -1583,6 +1585,8 @@ sc_pkcs15_convert_pubkey(struct sc_pkcs15_pubkey *pkcs15_key, void *evp_key) /* copy the public key */ if (buflen > 0) { dst->ecpointQ.value = malloc(buflen); + if (!dst->ecpointQ.value) + return SC_ERROR_OUT_OF_MEMORY; memcpy(dst->ecpointQ.value, buf, buflen); dst->ecpointQ.len = buflen; /* calculate the field length */ diff --git a/src/pkcs11/mechanism.c b/src/pkcs11/mechanism.c index 63506382..fec1123a 100644 --- a/src/pkcs11/mechanism.c +++ b/src/pkcs11/mechanism.c @@ -990,6 +990,9 @@ sc_pkcs11_register_sign_and_hash_mechanism(struct sc_pkcs11_card *p11card, mech_info.flags &= (CKF_SIGN | CKF_SIGN_RECOVER | CKF_VERIFY | CKF_VERIFY_RECOVER); info = calloc(1, sizeof(*info)); + if (!info) + LOG_FUNC_RETURN(p11card->card->ctx, SC_ERROR_OUT_OF_MEMORY); + info->mech = mech; info->sign_type = sign_type; info->hash_type = hash_type;