From b7b501d0a51dfa91598c6ed3180b909cfde6ff32 Mon Sep 17 00:00:00 2001 From: Frank Morgner Date: Fri, 10 May 2019 18:18:36 +0200 Subject: [PATCH] fixed issues reported by clang-analyzer --- src/common/compat_getopt.c | 5 +++-- src/common/simclist.c | 4 ++-- src/libopensc/card-atrust-acos.c | 3 +-- src/libopensc/card-entersafe.c | 3 +-- src/libopensc/card-epass2003.c | 4 +--- src/libopensc/card-iasecc.c | 3 ++- src/libopensc/card-starcos.c | 3 +-- src/libopensc/pkcs15-piv.c | 6 ++++-- src/libopensc/pkcs15-pubkey.c | 7 ++++--- src/pkcs11/framework-pkcs15.c | 5 ++++- src/sm/sm-eac.c | 31 ++++++++++++++++++------------- src/sm/sm-iso.c | 16 +++++++--------- src/tools/pkcs15-crypt.c | 4 ++-- src/tools/pkcs15-init.c | 23 +++++++++++++++++++---- 14 files changed, 69 insertions(+), 48 deletions(-) diff --git a/src/common/compat_getopt.c b/src/common/compat_getopt.c index 951e3554..bc701135 100644 --- a/src/common/compat_getopt.c +++ b/src/common/compat_getopt.c @@ -143,7 +143,7 @@ int _my_getopt_internal(int argc, char * argv[], const char *shortopts, const struct option *longopts, int *longind, int long_only) { - char mode, colon_mode = *shortopts; + char mode, colon_mode; int shortoff = 0, opt = -1; if(getenv("POSIXLY_CORRECT")) colon_mode = mode = '+'; @@ -230,7 +230,8 @@ int _my_getopt_internal(int argc, char * argv[], const char *shortopts, argv[0], longopts[found].name); } else { my_optarg = argv[my_optind] + ++charind; - charind = 0; + /* charind not read again + * charind = 0;*/ } } else if(longopts[found].has_arg == 1) { if(++my_optind >= argc) { diff --git a/src/common/simclist.c b/src/common/simclist.c index 03ed79bd..58be5e3e 100644 --- a/src/common/simclist.c +++ b/src/common/simclist.c @@ -764,10 +764,10 @@ int list_concat(const list_t *l1, const list_t *l2, list_t *simclist_restrict de /* fix mid pointer */ err = l2->numels - l1->numels; - if ((err+1)/2 > 0) { /* correct pos RIGHT (err-1)/2 moves */ + if (dest->mid && (err+1)/2 > 0) { /* correct pos RIGHT (err-1)/2 moves */ err = (err+1)/2; for (cnt = 0; cnt < (unsigned int)err; cnt++) dest->mid = dest->mid->next; - } else if (err/2 < 0) { /* correct pos LEFT (err/2)-1 moves */ + } else if (dest->mid && err/2 < 0) { /* correct pos LEFT (err/2)-1 moves */ err = -err/2; for (cnt = 0; cnt < (unsigned int)err; cnt++) dest->mid = dest->mid->prev; } diff --git a/src/libopensc/card-atrust-acos.c b/src/libopensc/card-atrust-acos.c index fb3bd70d..83da3923 100644 --- a/src/libopensc/card-atrust-acos.c +++ b/src/libopensc/card-atrust-acos.c @@ -445,8 +445,7 @@ static int atrust_acos_select_file(struct sc_card *card, { n_pathbuf[0] = 0x3f; n_pathbuf[1] = 0x00; - for (i=0; i< pathlen; i++) - n_pathbuf[i+2] = pathbuf[i]; + memcpy(n_pathbuf+2, path, pathlen); path = n_pathbuf; pathlen += 2; } diff --git a/src/libopensc/card-entersafe.c b/src/libopensc/card-entersafe.c index aa74c619..cb6fa15b 100644 --- a/src/libopensc/card-entersafe.c +++ b/src/libopensc/card-entersafe.c @@ -596,8 +596,7 @@ static int entersafe_select_path(sc_card_t *card, { n_pathbuf[0] = 0x3f; n_pathbuf[1] = 0x00; - for (i=0; i< pathlen; i++) - n_pathbuf[i+2] = pathbuf[i]; + memcpy(n_pathbuf+2, path, pathlen); path = n_pathbuf; pathlen += 2; } diff --git a/src/libopensc/card-epass2003.c b/src/libopensc/card-epass2003.c index 299520d6..26b9d79f 100644 --- a/src/libopensc/card-epass2003.c +++ b/src/libopensc/card-epass2003.c @@ -1463,9 +1463,7 @@ epass2003_select_path(struct sc_card *card, const u8 pathbuf[16], const size_t l if (path[0] != 0x3f || path[1] != 0x00) { n_pathbuf[0] = 0x3f; n_pathbuf[1] = 0x00; - - for (i = 0; i < pathlen; i++) - n_pathbuf[i + 2] = pathbuf[i]; + memcpy(n_pathbuf+2, path, pathlen); path = n_pathbuf; pathlen += 2; } diff --git a/src/libopensc/card-iasecc.c b/src/libopensc/card-iasecc.c index 8f5be901..9a4c3ce1 100644 --- a/src/libopensc/card-iasecc.c +++ b/src/libopensc/card-iasecc.c @@ -623,8 +623,9 @@ iasecc_init(struct sc_card *card) card->sm_ctx.ops.update_binary = _iasecc_sm_update_binary; #endif - if (!rv) + if (!rv && card->ef_atr && card->ef_atr->aid.len) { sc_log(ctx, "EF.ATR(aid:'%s')", sc_dump_hex(card->ef_atr->aid.value, card->ef_atr->aid.len)); + } LOG_FUNC_RETURN(ctx, rv); } diff --git a/src/libopensc/card-starcos.c b/src/libopensc/card-starcos.c index c022b04d..a518eab9 100644 --- a/src/libopensc/card-starcos.c +++ b/src/libopensc/card-starcos.c @@ -704,8 +704,7 @@ static int starcos_select_file(sc_card_t *card, { n_pathbuf[0] = 0x3f; n_pathbuf[1] = 0x00; - for (i=0; i< pathlen; i++) - n_pathbuf[i+2] = pathbuf[i]; + memcpy(n_pathbuf+2, path, pathlen); path = n_pathbuf; pathlen += 2; } diff --git a/src/libopensc/pkcs15-piv.c b/src/libopensc/pkcs15-piv.c index 5f615cc7..4f069930 100644 --- a/src/libopensc/pkcs15-piv.c +++ b/src/libopensc/pkcs15-piv.c @@ -981,7 +981,7 @@ sc_log(card->ctx, "DEE Adding pin %d label=%s",i, label); for (i = 0; i < PIV_NUM_CERTS_AND_KEYS; i++) { struct sc_pkcs15_pubkey_info pubkey_info; struct sc_pkcs15_object pubkey_obj; - struct sc_pkcs15_pubkey *p15_key; + struct sc_pkcs15_pubkey *p15_key = NULL; memset(&pubkey_info, 0, sizeof(pubkey_info)); memset(&pubkey_obj, 0, sizeof(pubkey_obj)); @@ -1035,8 +1035,10 @@ sc_log(card->ctx, "DEE Adding pin %d label=%s",i, label); sc_log(card->ctx, "Adding pubkey from file %s",filename); r = sc_pkcs15_pubkey_from_spki_file(card->ctx, filename, &p15_key); - if (r < 0) + if (r < 0) { + free(p15_key); continue; + } /* Lets also try another method. */ r = sc_pkcs15_encode_pubkey_as_spki(card->ctx, p15_key, &pubkey_info.direct.spki.value, &pubkey_info.direct.spki.len); diff --git a/src/libopensc/pkcs15-pubkey.c b/src/libopensc/pkcs15-pubkey.c index b094832c..84827c4a 100644 --- a/src/libopensc/pkcs15-pubkey.c +++ b/src/libopensc/pkcs15-pubkey.c @@ -200,8 +200,8 @@ sc_pkcs15_decode_pubkey_direct_value(struct sc_pkcs15_card *p15card, struct sc_p LOG_TEST_RET(ctx, rv, "Failed to decode 'SPKI' direct value"); rv = sc_pkcs15_encode_pubkey(ctx, pubkey, &info->direct.raw.value, &info->direct.raw.len); - LOG_TEST_RET(ctx, rv, "Failed to encode 'RAW' direct value"); sc_pkcs15_free_pubkey(pubkey); + LOG_TEST_RET(ctx, rv, "Failed to encode 'RAW' direct value"); } LOG_FUNC_RETURN(ctx, SC_SUCCESS); @@ -1428,9 +1428,10 @@ sc_pkcs15_pubkey_from_spki_sequence(struct sc_context *ctx, const unsigned char r = sc_asn1_decode(ctx, asn1_spki, buf, buflen, NULL, NULL); LOG_TEST_RET(ctx, r, "ASN.1 cannot parse subjectPublicKeyInfo"); - if(outpubkey) + if(outpubkey) { + free(*outpubkey); *outpubkey = pubkey; - else + } else free(pubkey); LOG_FUNC_RETURN(ctx, r); diff --git a/src/pkcs11/framework-pkcs15.c b/src/pkcs11/framework-pkcs15.c index 4b1f1790..57ea18a5 100644 --- a/src/pkcs11/framework-pkcs15.c +++ b/src/pkcs11/framework-pkcs15.c @@ -4085,7 +4085,7 @@ pkcs15_prkey_unwrap(struct sc_pkcs11_session *session, void *obj, struct pkcs15_fw_data *fw_data = NULL; struct pkcs15_prkey_object *prkey = (struct pkcs15_prkey_object *) obj; struct pkcs15_any_object *targetKeyObj = (struct pkcs15_any_object *) targetKey; - int rv, flags = 0; + int rv; sc_log(context, "Initiating unwrapping with private key."); @@ -4109,6 +4109,8 @@ pkcs15_prkey_unwrap(struct sc_pkcs11_session *session, void *obj, sc_log(context, "Using mechanism %lx.", pMechanism->mechanism); +#if 0 + /* FIXME https://github.com/OpenSC/OpenSC/issues/1595 */ /* Select the proper padding mechanism */ switch (pMechanism->mechanism) { case CKM_RSA_PKCS: @@ -4120,6 +4122,7 @@ pkcs15_prkey_unwrap(struct sc_pkcs11_session *session, void *obj, default: return CKR_MECHANISM_INVALID; } +#endif rv = sc_lock(p11card->card); diff --git a/src/sm/sm-eac.c b/src/sm/sm-eac.c index 88f670a1..53380194 100644 --- a/src/sm/sm-eac.c +++ b/src/sm/sm-eac.c @@ -1486,6 +1486,8 @@ int perform_terminal_authentication(sc_card_t *card, struct eac_sm_ctx *eacsmctx = NULL; unsigned char *ef_cardaccess = NULL; EAC_CTX *eac_ctx = NULL; + const unsigned char *chr = NULL; + size_t chr_len = 0; if (!card || !certs_lens || !certs) { r = SC_ERROR_INVALID_ARGUMENTS; @@ -1566,6 +1568,9 @@ int perform_terminal_authentication(sc_card_t *card, if (r < 0) goto err; + chr = cvc_cert->body->certificate_holder_reference->data; + chr_len = cvc_cert->body->certificate_holder_reference->length; + certs++; certs_lens++; } @@ -1590,9 +1595,7 @@ int perform_terminal_authentication(sc_card_t *card, } - r = eac_mse_set_at_ta(card, eacsmctx->ctx->ta_ctx->protocol, - cvc_cert->body->certificate_holder_reference->data, - cvc_cert->body->certificate_holder_reference->length, + r = eac_mse_set_at_ta(card, eacsmctx->ctx->ta_ctx->protocol, chr, chr_len, (unsigned char *) eacsmctx->eph_pub_key->data, eacsmctx->eph_pub_key->length, auxiliary_data, auxiliary_data_len); if (r < 0) { @@ -2345,16 +2348,18 @@ eac_sm_clear_free(const struct iso_sm_ctx *ctx) { if (ctx) { struct eac_sm_ctx *eacsmctx = ctx->priv_data; - EAC_CTX_clear_free(eacsmctx->ctx); - if (eacsmctx->certificate_description) - BUF_MEM_free(eacsmctx->certificate_description); - if (eacsmctx->id_icc) - BUF_MEM_free(eacsmctx->id_icc); - if (eacsmctx->eph_pub_key) - BUF_MEM_free(eacsmctx->eph_pub_key); - if (eacsmctx->auxiliary_data) - BUF_MEM_free(eacsmctx->auxiliary_data); - free(eacsmctx); + if (eacsmctx) { + EAC_CTX_clear_free(eacsmctx->ctx); + if (eacsmctx->certificate_description) + BUF_MEM_free(eacsmctx->certificate_description); + if (eacsmctx->id_icc) + BUF_MEM_free(eacsmctx->id_icc); + if (eacsmctx->eph_pub_key) + BUF_MEM_free(eacsmctx->eph_pub_key); + if (eacsmctx->auxiliary_data) + BUF_MEM_free(eacsmctx->auxiliary_data); + free(eacsmctx); + } } } diff --git a/src/sm/sm-iso.c b/src/sm/sm-iso.c index 41b32a6b..9dbda86e 100644 --- a/src/sm/sm-iso.c +++ b/src/sm/sm-iso.c @@ -92,17 +92,15 @@ add_padding(const struct iso_sm_ctx *ctx, const u8 *data, size_t datalen, switch (ctx->padding_indicator) { case SM_NO_PADDING: if (*padded != data) { - if (datalen == 0) { - free(*padded); - p = malloc(datalen); - } else { + if (datalen != 0) { p = realloc(*padded, datalen); + if (!p) + return SC_ERROR_OUT_OF_MEMORY; + *padded = p; + memcpy(*padded, data, datalen); + } else { + *padded = NULL; } - if (!p) - return SC_ERROR_OUT_OF_MEMORY; - *padded = p; - /* Flawfinder: ignore */ - memcpy(*padded, data, datalen); } return datalen; case SM_ISO_PADDING: diff --git a/src/tools/pkcs15-crypt.c b/src/tools/pkcs15-crypt.c index 6d064ec3..763510ed 100644 --- a/src/tools/pkcs15-crypt.c +++ b/src/tools/pkcs15-crypt.c @@ -292,7 +292,7 @@ static int decipher(struct sc_pkcs15_object *obj) static int get_key(unsigned int usage, sc_pkcs15_object_t **result) { - sc_pkcs15_object_t *key, *pin; + sc_pkcs15_object_t *key, *pin = NULL; const char *usage_name; sc_pkcs15_id_t id; int r; @@ -346,7 +346,7 @@ static int get_key(unsigned int usage, sc_pkcs15_object_t **result) * a crypto operation. Card drivers can test for SC_AC_CONTEXT_SPECIFIC * to do any special handling. */ - if (key->user_consent) { + if (key->user_consent && pin) { int auth_meth_saved; struct sc_pkcs15_auth_info *pinfo = (struct sc_pkcs15_auth_info *) pin->data; diff --git a/src/tools/pkcs15-init.c b/src/tools/pkcs15-init.c index f02692d1..f45f4cb6 100644 --- a/src/tools/pkcs15-init.c +++ b/src/tools/pkcs15-init.c @@ -2097,6 +2097,10 @@ get_pin_callback(struct sc_profile *profile, hints.p15card = g_p15card; if ((r = get_pin(&hints, &secret)) < 0) { + if (secret) { + sc_mem_clear(secret, strlen(secret)); + free(secret); + } fprintf(stderr, "Failed to read PIN from user: %s\n", sc_strerror(r)); @@ -3171,7 +3175,7 @@ static int verify_pin(struct sc_pkcs15_card *p15card, char *auth_id_str) } if (opt_pins[0] != NULL) { - pin = (char *) opt_pins[0]; + pin = strdup(opt_pins[0]); } else { sc_ui_hints_t hints; @@ -3192,15 +3196,26 @@ static int verify_pin(struct sc_pkcs15_card *p15card, char *auth_id_str) hints.card = g_card; hints.p15card = p15card; - get_pin(&hints, &pin); + if ((r = get_pin(&hints, &pin)) < 0) { + if (pin) { + sc_mem_clear(pin, strlen(pin)); + free(pin); + } + fprintf(stderr, + "Failed to read PIN from user: %s\n", + sc_strerror(r)); + return r; + } } - r = sc_pkcs15_verify_pin(p15card, pin_obj, (unsigned char *)pin, pin ? strlen((char *) pin) : 0); + r = sc_pkcs15_verify_pin(p15card, pin_obj, (unsigned char *)pin, pin ? strlen(pin) : 0); if (r < 0) fprintf(stderr, "Operation failed: %s\n", sc_strerror(r)); - if (NULL == opt_pins[0]) + if (pin) { + sc_mem_clear(pin, strlen(pin)); free(pin); + } return r; }