Hi there,
sorry for coming back again with the same topic, but one of the fixes
from last email was not ideal solution (was leaving locked mutex) and
after cleaning the others I managed to isolate in this second round (all
summed up in commit message):
* soft_stdll/soft_specific.c:
* Missing fallthrough comment from last report
* common/trace.c:
* Ignore return value of read (make compiler satisfied)
* icsf_stdll/new_host.c:
* Amend last commit - we can't return before releasing lock
* common/object.c:
* Fix refactorization of object_free() to do the actual free()
* broken by commit 06fbff5de88ec44c14eabc8df71f25336633420c
The other patch (3) is from different run, where I searched also for
bugs from earlier versions, not just the ones introduced in latest
versions. I searched for the ones that make sense, but please review
them. I am running another scan if the fixes really fix what I though,
but I will not make it to review them completely today.
Kind regards,
--
Jakub Jelen
Security Technologies
Red Hat
commit 0f844b6bee71ed93a7e772e718230535ac942a6d
Author: Jakub Jelen <jje...@redhat.com>
Date: Thu Mar 24 13:40:23 2016 +0100
Few more issues reported by Coverity
* soft_stdll/soft_specific.c:
* Missing fallthrough comment from last report
* common/trace.c:
* Ignore return value of read
* icsf_stdll/new_host.c:
* Amend last commit - we can't return before releasing lock
* common/object.c:
* Fix refcatorization of object_free() to do the actual free()
diff --git a/usr/lib/pkcs11/common/object.c b/usr/lib/pkcs11/common/object.c
index f085b95..5955cc0 100755
--- a/usr/lib/pkcs11/common/object.c
+++ b/usr/lib/pkcs11/common/object.c
@@ -632,8 +632,9 @@ object_flatten( OBJECT * obj,
//
void object_free(OBJECT *obj)
{
- if (obj && obj->template) {
- template_free(obj->template);
+ if (obj) {
+ if (obj->template)
+ template_free(obj->template);
free(obj);
}
}
diff --git a/usr/lib/pkcs11/common/trace.c b/usr/lib/pkcs11/common/trace.c
index da294f8..ecffe3d 100644
--- a/usr/lib/pkcs11/common/trace.c
+++ b/usr/lib/pkcs11/common/trace.c
@@ -517,7 +517,7 @@ void ock_traceit(trace_level_t level, const char *fmt, ...)
/* serialize appends to the file */
pthread_mutex_lock(&tlmtx);
- write(trace.fd, buf, strlen(buf));
+ (void) write(trace.fd, buf, strlen(buf));
pthread_mutex_unlock(&tlmtx);
}
}
diff --git a/usr/lib/pkcs11/icsf_stdll/new_host.c b/usr/lib/pkcs11/icsf_stdll/new_host.c
index 57d561e..12bb3e7 100644
--- a/usr/lib/pkcs11/icsf_stdll/new_host.c
+++ b/usr/lib/pkcs11/icsf_stdll/new_host.c
@@ -998,7 +998,7 @@ CK_RV SC_Login(ST_SESSION_HANDLE *sSession, CK_USER_TYPE userType,
if (!sess) {
TRACE_ERROR("%s\n", ock_err(ERR_SESSION_HANDLE_INVALID));
rc = CKR_SESSION_HANDLE_INVALID;
- return rc; /* can't goto done; We need sess to log */
+ goto done;
}
flags = &nv_token_data->token_info.flags;
@@ -1087,7 +1087,8 @@ done:
}
TRACE_INFO("C_Login: rc = 0x%08lx\n", rc);
- save_token_data(sess->session_info.slotID);
+ if (sess)
+ save_token_data(sess->session_info.slotID);
MY_UnlockMutex(&login_mutex);
return rc;
}
diff --git a/usr/lib/pkcs11/soft_stdll/soft_specific.c b/usr/lib/pkcs11/soft_stdll/soft_specific.c
index ce53439..a201ec2 100644
--- a/usr/lib/pkcs11/soft_stdll/soft_specific.c
+++ b/usr/lib/pkcs11/soft_stdll/soft_specific.c
@@ -2626,6 +2626,7 @@ static CK_RV softtok_hmac_final(SIGN_VERIFY_CONTEXT *ctx, CK_BYTE *signature,
break;
case CKM_SHA384_HMAC_GENERAL:
general = TRUE;
+ /* fallthrough */
case CKM_SHA384_HMAC:
mac_len = SHA3_HASH_SIZE;
break;
commit cf0d31fd06520f7d7bbe4385992b8f76df597ea7
Author: Jakub Jelen <jje...@redhat.com>
Date: Thu Mar 24 16:27:51 2016 +0100
Coverity: Full scan
* common/asn1.c
* Potential memory leak
* Sanitize repetitive usage of buffer
* common/cert.c
* Dead code removed
* common/mech_dh.c
* Potential memory leak
* common/mech_rsa.c
* Potential memory leak
* common/new_host.c
* Sanitize cleanup sess pointer
* common/object.c
* Unused variable
* Potential use of unitialized memory
* common/sess_mgr.c
* Potential memory leak
* common/template.c
* Uninitialized variable
* icsf_stdll/icsf_specific.c
* Unused variable
* Memset '0' -- not sure if it was intended
* Add switch fallthrough note
* Sanitize arguments
* soft_stdll/soft_specific.c
* Potential memory leak
* tpm_stdll/tpm_specific.c
* Double free
diff --git a/usr/lib/pkcs11/common/asn1.c b/usr/lib/pkcs11/common/asn1.c
index 5c63861..8d0e33a 100755
--- a/usr/lib/pkcs11/common/asn1.c
+++ b/usr/lib/pkcs11/common/asn1.c
@@ -733,6 +733,7 @@ ber_encode_SEQUENCE( CK_BBOOL length_only,
return CKR_OK;
}
+ free( buf );
TRACE_ERROR("%s\n", ock_err(ERR_FUNCTION_FAILED));
return CKR_FUNCTION_FAILED;
}
@@ -1045,6 +1046,7 @@ ber_encode_RSAPrivateKey( CK_BBOOL length_only,
memcpy( buf+offset, buf2, len );
offset += len;
free( buf2 );
+ buf2 = NULL;
rc = ber_encode_INTEGER( FALSE, &buf2, &len, (CK_BYTE *)modulus + sizeof(CK_ATTRIBUTE), modulus->ulValueLen );
if (rc != CKR_OK){
@@ -1054,6 +1056,7 @@ ber_encode_RSAPrivateKey( CK_BBOOL length_only,
memcpy( buf+offset, buf2, len );
offset += len;
free( buf2 );
+ buf2 = NULL;
rc = ber_encode_INTEGER( FALSE, &buf2, &len, (CK_BYTE *)publ_exp + sizeof(CK_ATTRIBUTE), publ_exp->ulValueLen );
if (rc != CKR_OK){
@@ -1063,6 +1066,7 @@ ber_encode_RSAPrivateKey( CK_BBOOL length_only,
memcpy( buf+offset, buf2, len );
offset += len;
free( buf2 );
+ buf2 = NULL;
if (opaque != NULL) {
// the CKA_IBM_OPAQUE attrib
@@ -1074,6 +1078,7 @@ ber_encode_RSAPrivateKey( CK_BBOOL length_only,
memcpy( buf+offset, buf2, len );
offset += len;
free( buf2 );
+ buf2 = NULL;
} else {
rc = ber_encode_INTEGER( FALSE, &buf2, &len, (CK_BYTE *)priv_exp + sizeof(CK_ATTRIBUTE), priv_exp->ulValueLen );
if (rc != CKR_OK){
@@ -1083,6 +1088,7 @@ ber_encode_RSAPrivateKey( CK_BBOOL length_only,
memcpy( buf+offset, buf2, len );
offset += len;
free( buf2 );
+ buf2 = NULL;
rc = ber_encode_INTEGER( FALSE, &buf2, &len, (CK_BYTE *)prime1 + sizeof(CK_ATTRIBUTE), prime1->ulValueLen );
if (rc != CKR_OK){
@@ -1092,6 +1098,7 @@ ber_encode_RSAPrivateKey( CK_BBOOL length_only,
memcpy( buf+offset, buf2, len );
offset += len;
free( buf2 );
+ buf2 = NULL;
rc = ber_encode_INTEGER( FALSE, &buf2, &len, (CK_BYTE *)prime2 + sizeof(CK_ATTRIBUTE), prime2->ulValueLen );
if (rc != CKR_OK){
@@ -1101,6 +1108,7 @@ ber_encode_RSAPrivateKey( CK_BBOOL length_only,
memcpy( buf+offset, buf2, len );
offset += len;
free( buf2 );
+ buf2 = NULL;
rc = ber_encode_INTEGER( FALSE, &buf2, &len, (CK_BYTE *)exponent1 + sizeof(CK_ATTRIBUTE), exponent1->ulValueLen );
if (rc != CKR_OK){
@@ -1110,6 +1118,7 @@ ber_encode_RSAPrivateKey( CK_BBOOL length_only,
memcpy( buf+offset, buf2, len );
offset += len;
free( buf2 );
+ buf2 = NULL;
rc = ber_encode_INTEGER( FALSE, &buf2, &len, (CK_BYTE *)exponent2 + sizeof(CK_ATTRIBUTE), exponent2->ulValueLen );
if (rc != CKR_OK){
@@ -1119,6 +1128,7 @@ ber_encode_RSAPrivateKey( CK_BBOOL length_only,
memcpy( buf+offset, buf2, len );
offset += len;
free( buf2 );
+ buf2 = NULL;
rc = ber_encode_INTEGER( FALSE, &buf2, &len, (CK_BYTE *)coeff + sizeof(CK_ATTRIBUTE), coeff->ulValueLen );
if (rc != CKR_OK){
@@ -1128,6 +1138,7 @@ ber_encode_RSAPrivateKey( CK_BBOOL length_only,
memcpy( buf+offset, buf2, len );
offset += len;
free( buf2 );
+ buf2 = NULL;
}
rc = ber_encode_SEQUENCE( FALSE, &buf2, &len, buf, offset );
diff --git a/usr/lib/pkcs11/common/cert.c b/usr/lib/pkcs11/common/cert.c
index f85f308..b478aab 100755
--- a/usr/lib/pkcs11/common/cert.c
+++ b/usr/lib/pkcs11/common/cert.c
@@ -370,8 +370,6 @@ cert_validate_attribute( TEMPLATE *tmpl, CK_ATTRIBUTE *attr, CK_ULONG mode )
default:
return template_validate_base_attribute( tmpl, attr, mode );
}
-
- return template_validate_base_attribute( tmpl, attr, mode );
}
diff --git a/usr/lib/pkcs11/common/mech_dh.c b/usr/lib/pkcs11/common/mech_dh.c
index 0d67a05..467c708 100644
--- a/usr/lib/pkcs11/common/mech_dh.c
+++ b/usr/lib/pkcs11/common/mech_dh.c
@@ -398,6 +398,7 @@ dh_pkcs_derive( SESSION * sess,
&temp_obj );
if (rc != CKR_OK){
TRACE_DEVEL("Object Mgr create skeleton failed.\n");
+ free(new_attr);
return rc;
}
diff --git a/usr/lib/pkcs11/common/mech_rsa.c b/usr/lib/pkcs11/common/mech_rsa.c
index 0430863..d4aba92 100755
--- a/usr/lib/pkcs11/common/mech_rsa.c
+++ b/usr/lib/pkcs11/common/mech_rsa.c
@@ -2303,6 +2303,7 @@ rsa_hash_pkcs_sign_final( SESSION * sess,
tmp = (CK_BYTE *)buf1;
memcpy( tmp, oid, oid_len );
memcpy( tmp + oid_len, octet_str, octet_str_len );
+ free( octet_str );
rc = ber_encode_SEQUENCE( FALSE, &ber_data, &ber_data_len, tmp, (oid_len + octet_str_len) );
if (rc != CKR_OK){
@@ -2331,7 +2332,6 @@ rsa_hash_pkcs_sign_final( SESSION * sess,
}
done:
- if (octet_str) free( octet_str );
if (ber_data) free( ber_data );
sign_mgr_cleanup( &sign_ctx );
return rc;
diff --git a/usr/lib/pkcs11/common/new_host.c b/usr/lib/pkcs11/common/new_host.c
index 71e6515..dd4cb0a 100755
--- a/usr/lib/pkcs11/common/new_host.c
+++ b/usr/lib/pkcs11/common/new_host.c
@@ -1355,7 +1355,8 @@ done:
}
TRACE_INFO("C_Login: rc = 0x%08lx\n", rc);
- save_token_data(sess->session_info.slotID);
+ if (sess)
+ save_token_data(sess->session_info.slotID);
MY_UnlockMutex(&login_mutex);
return rc;
}
diff --git a/usr/lib/pkcs11/common/object.c b/usr/lib/pkcs11/common/object.c
index 5955cc0..d6cbea7 100755
--- a/usr/lib/pkcs11/common/object.c
+++ b/usr/lib/pkcs11/common/object.c
@@ -335,7 +335,6 @@ object_create( CK_ATTRIBUTE * pTemplate,
CK_ATTRIBUTE * attr = NULL;
CK_ATTRIBUTE * sensitive = NULL;
CK_ATTRIBUTE * extractable = NULL;
- CK_ATTRIBUTE * local = NULL;
CK_BBOOL class_given = FALSE;
CK_BBOOL subclass_given = FALSE;
CK_BBOOL flag;
@@ -447,7 +446,6 @@ object_create( CK_ATTRIBUTE * pTemplate,
error:
if (sensitive) free( sensitive );
if (extractable) free( extractable );
- if (local) free( local );
object_free( o );
return rc;
@@ -493,7 +491,10 @@ object_copy( CK_ATTRIBUTE * pTemplate,
if (!o || !tmpl || !new_tmpl) {
rc = CKR_HOST_MEMORY;
TRACE_ERROR("%s\n", ock_err(ERR_HOST_MEMORY));
- goto error;
+ if (o) free(o);
+ if (tmpl) free(tmpl);
+ if (new_tmpl) free(new_tmpl);
+ return rc; // do not goto done -- memory might not be initialized
}
memset( o, 0x0, sizeof(OBJECT) );
diff --git a/usr/lib/pkcs11/common/sess_mgr.c b/usr/lib/pkcs11/common/sess_mgr.c
index 32d1081..367e9c5 100755
--- a/usr/lib/pkcs11/common/sess_mgr.c
+++ b/usr/lib/pkcs11/common/sess_mgr.c
@@ -373,6 +373,7 @@ session_mgr_new( CK_ULONG flags, CK_SLOT_ID slot_id, CK_SESSION_HANDLE_PTR phSes
rc = MY_LockMutex( &pkcs_mutex ); // this protects next_session_handle
if (rc != CKR_OK){
TRACE_ERROR("Mutex lock failed.\n");
+ free( new_session );
return rc;
}
pkcs_locked = TRUE;
@@ -395,6 +396,7 @@ session_mgr_new( CK_ULONG flags, CK_SLOT_ID slot_id, CK_SESSION_HANDLE_PTR phSes
rc = MY_LockMutex( &sess_list_mutex );
if (rc != CKR_OK){
TRACE_ERROR("Mutex lock failed.\n");
+ free( new_session );
return rc;
}
sess_locked = TRUE;
diff --git a/usr/lib/pkcs11/common/template.c b/usr/lib/pkcs11/common/template.c
index 9173c4a..c1a12e9 100755
--- a/usr/lib/pkcs11/common/template.c
+++ b/usr/lib/pkcs11/common/template.c
@@ -1075,7 +1075,7 @@ CK_RV template_free(TEMPLATE *tmpl)
CK_BBOOL template_get_class(TEMPLATE *tmpl, CK_ULONG *class, CK_ULONG *subclass)
{
DL_NODE *node;
- CK_BBOOL found;
+ CK_BBOOL found = FALSE;
if (!tmpl || !class || !subclass)
return FALSE;
diff --git a/usr/lib/pkcs11/icsf_stdll/icsf_specific.c b/usr/lib/pkcs11/icsf_stdll/icsf_specific.c
index 996d62d..a633a22 100644
--- a/usr/lib/pkcs11/icsf_stdll/icsf_specific.c
+++ b/usr/lib/pkcs11/icsf_stdll/icsf_specific.c
@@ -478,7 +478,6 @@ CK_RV login(LDAP **ld, CK_SLOT_ID slot_id, CK_BYTE *pin, CK_ULONG pin_len,
CK_RV rc = CKR_OK;
struct slot_data data;
LDAP *ldapd = NULL;
- char *fname = NULL;
int ret;
/* Check Slot ID */
@@ -543,9 +542,6 @@ done:
if (rc == CKR_OK && ld)
*ld = ldapd;
- if (fname)
- free(fname);
-
return rc;
}
@@ -602,7 +598,7 @@ CK_RV reset_token_data(CK_SLOT_ID slot_id, CK_CHAR_PTR pin, CK_ULONG pin_len)
TRACE_ERROR("Failed to reset so pin.\n");
return CKR_FUNCTION_FAILED;
}
- memset(nv_token_data->user_pin_sha, '0',
+ memset(nv_token_data->user_pin_sha, 0,
sizeof(nv_token_data->user_pin_sha));
if (slot_data[slot_id]->mech == ICSF_CFG_MECH_SIMPLE) {
@@ -2609,6 +2605,7 @@ CK_RV icsftok_decrypt_update(SESSION *session, CK_BYTE_PTR input_part,
case CKM_DES_CBC_PAD:
case CKM_DES3_CBC_PAD:
padding = 1;
+ /* fallthrough */
default:
if (multi_part_ctx->initiated) {
chaining = ICSF_CHAINING_CONTINUE;
@@ -3651,6 +3648,10 @@ CK_RV icsftok_sign_update(SESSION *session, CK_BYTE *in_data,
if (multi_part_ctx->initiated)
memcpy(chain_data, multi_part_ctx->chain_data,
chain_data_len);
+ } else {
+ TRACE_ERROR("%s\n", ock_err(ERR_ARGUMENTS_BAD));
+ rc = ERR_ARGUMENTS_BAD;
+ goto done;
}
switch (ctx->mech.mechanism) {
@@ -3805,6 +3806,10 @@ CK_RV icsftok_sign_final(SESSION *session, CK_BYTE *signature,
if (ctx->context) {
multi_part_ctx = (struct icsf_multi_part_context *)ctx->context;
memcpy(chain_data, multi_part_ctx->chain_data, chain_data_len);
+ } else {
+ TRACE_ERROR("%s\n", ock_err(ERR_ARGUMENTS_BAD));
+ rc = ERR_ARGUMENTS_BAD;
+ goto done;
}
switch (ctx->mech.mechanism) {
diff --git a/usr/lib/pkcs11/soft_stdll/soft_specific.c b/usr/lib/pkcs11/soft_stdll/soft_specific.c
index a201ec2..66e3a2c 100644
--- a/usr/lib/pkcs11/soft_stdll/soft_specific.c
+++ b/usr/lib/pkcs11/soft_stdll/soft_specific.c
@@ -1752,7 +1752,7 @@ CK_RV token_specific_rsa_oaep_decrypt(ENCR_DECR_CONTEXT *ctx, CK_BYTE *in_data,
rc = os_specific_rsa_decrypt(in_data, in_data_len, decr_data, key_obj);
if (rc != CKR_OK)
- return rc;
+ goto error;
/* pkcs1v2.2, section 7.1.2 Step 2:
* EME-OAEP decoding.
@@ -1760,6 +1760,7 @@ CK_RV token_specific_rsa_oaep_decrypt(ENCR_DECR_CONTEXT *ctx, CK_BYTE *in_data,
rc = decode_eme_oaep(decr_data, in_data_len, out_data, out_data_len,
oaepParms->mgf, hash, hlen);
+error:
if (decr_data)
free(decr_data);
return rc;
diff --git a/usr/lib/pkcs11/tpm_stdll/tpm_specific.c b/usr/lib/pkcs11/tpm_stdll/tpm_specific.c
index e588f03..e7978d3 100644
--- a/usr/lib/pkcs11/tpm_stdll/tpm_specific.c
+++ b/usr/lib/pkcs11/tpm_stdll/tpm_specific.c
@@ -966,7 +966,6 @@ token_store_priv_key(TSS_HKEY hKey, int key_type, CK_OBJECT_HANDLE *ckKey)
flag = TRUE;
if ((rc = build_attribute(CKA_HIDDEN, &flag, sizeof(CK_BBOOL), &new_attr))) {
TRACE_DEVEL("build_attribute failed\n");
- free(key_id);
return rc;
}
template_update_attribute( priv_key_obj->template, new_attr );
------------------------------------------------------------------------------
Transform Data into Opportunity.
Accelerate data analysis in your applications with
Intel Data Analytics Acceleration Library.
Click to learn more.
http://pubads.g.doubleclick.net/gampad/clk?id=278785351&iu=/4140
_______________________________________________
Opencryptoki-tech mailing list
Opencryptoki-tech@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opencryptoki-tech
