diff --git a/src/exchange.c b/src/exchange.c index b6652bda..b7c4d9cd 100644 --- a/src/exchange.c +++ b/src/exchange.c @@ -212,7 +212,7 @@ static int p11prov_ecdh_derive(void *ctx, unsigned char *secret, CK_KEY_TYPE key_type = CKK_GENERIC_SECRET; CK_BBOOL val_true = CK_TRUE; CK_BBOOL val_false = CK_FALSE; - CK_ULONG key_size = 0; + CK_ULONG key_size = outlen; CK_ATTRIBUTE key_template[5] = { { CKA_CLASS, &key_class, sizeof(key_class) }, { CKA_KEY_TYPE, &key_type, sizeof(key_type) }, @@ -225,7 +225,6 @@ static int p11prov_ecdh_derive(void *ctx, unsigned char *secret, CK_OBJECT_HANDLE handle; CK_OBJECT_HANDLE secret_handle; CK_SLOT_ID slotid; - unsigned long secret_len; struct fetch_attrs attrs[1]; int num = 0; CK_RV ret; @@ -240,11 +239,6 @@ static int p11prov_ecdh_derive(void *ctx, unsigned char *secret, return RET_OSSL_OK; } - if (ecdhctx->kdf_outlen > outlen) { - ERR_raise(ERR_LIB_PROV, PROV_R_OUTPUT_BUFFER_TOO_SMALL); - return 0; - } - /* set up mechanism */ if (ecdhctx->ecdh_params.kdf == CKF_DIGEST) { ecdhctx->ecdh_params.kdf = p11prov_ecdh_digest_to_kdf(ecdhctx->digest); @@ -266,9 +260,9 @@ static int p11prov_ecdh_derive(void *ctx, unsigned char *secret, /* complete key template */ if (ecdhctx->kdf_outlen) { - key_size = ecdhctx->kdf_outlen; - } else { - key_size = p11prov_obj_get_key_size(ecdhctx->key); + if (ecdhctx->kdf_outlen < outlen) { + key_size = ecdhctx->kdf_outlen; + } } handle = p11prov_obj_get_handle(ecdhctx->key); @@ -292,7 +286,7 @@ static int p11prov_ecdh_derive(void *ctx, unsigned char *secret, } P11PROV_debug("ECDH derived hey handle: %lu", secret_handle); - FA_SET_BUF_VAL(attrs, num, CKA_VALUE, secret, secret_len, false, true); + FA_SET_BUF_VAL(attrs, num, CKA_VALUE, secret, key_size, true); ret = p11prov_fetch_attributes(ecdhctx->provctx, session, secret_handle, attrs, num); p11prov_return_session(session); @@ -300,7 +294,7 @@ static int p11prov_ecdh_derive(void *ctx, unsigned char *secret, P11PROV_debug("ecdh failed to retrieve secret %lu", ret); return RET_OSSL_ERR; } - *psecretlen = secret_len; + FA_GET_LEN(attrs, 0, *psecretlen); return RET_OSSL_OK; } diff --git a/src/kdf.c b/src/kdf.c index 1e835a64..979e06d4 100644 --- a/src/kdf.c +++ b/src/kdf.c @@ -107,7 +107,6 @@ static int p11prov_hkdf_derive(void *ctx, unsigned char *key, size_t keylen, CK_OBJECT_HANDLE pkey_handle; CK_OBJECT_HANDLE dkey_handle; CK_SLOT_ID slotid; - unsigned long dkey_len; struct fetch_attrs attrs[1]; int num = 0; CK_RV ret; @@ -155,11 +154,19 @@ static int p11prov_hkdf_derive(void *ctx, unsigned char *key, size_t keylen, } P11PROV_debug("HKDF derived hey handle: %lu", dkey_handle); - FA_SET_BUF_VAL(attrs, num, CKA_VALUE, key, dkey_len, false, true); + FA_SET_BUF_VAL(attrs, num, CKA_VALUE, key, keylen, true); ret = p11prov_fetch_attributes(hkdfctx->provctx, hkdfctx->session, dkey_handle, attrs, num); if (ret != CKR_OK) { - P11PROV_debug("hkdf failed to retrieve secret %lu", ret); + P11PROV_raise(hkdfctx->provctx, ret, "Failed to retrieve derived key"); + return RET_OSSL_ERR; + } + FA_GET_LEN(attrs, 0, key_size); + if (key_size != keylen) { + ret = CKR_GENERAL_ERROR; + P11PROV_raise(hkdfctx->provctx, ret, + "Expected derived key of len %lz, but got %lu", keylen, + key_size); return RET_OSSL_ERR; } diff --git a/src/objects.c b/src/objects.c index d552ed15..78959697 100644 --- a/src/objects.c +++ b/src/objects.c @@ -615,11 +615,12 @@ CK_RV p11prov_obj_from_handle(P11PROV_CTX *ctx, P11PROV_SESSION *session, if (ret != CKR_OK) { P11PROV_raise(ctx, ret, "Failed to probe quirk"); } else if (token_supports_allowed_mechs == CK_TRUE) { - num = 0; - FA_SET_BUF_ALLOC(attrs, num, CKA_ALLOWED_MECHANISMS, false); - ret = p11prov_fetch_attributes(ctx, session, handle, attrs, num); + struct fetch_attrs a[1]; + CK_ULONG an = 0; + FA_SET_BUF_ALLOC(a, an, CKA_ALLOWED_MECHANISMS, false); + ret = p11prov_fetch_attributes(ctx, session, handle, a, 1); if (ret == CKR_OK) { - CKATTR_MOVE(obj->attrs[obj->numattrs], attrs[0]); + obj->attrs[obj->numattrs] = a[0].attr; obj->numattrs++; } else if (ret == CKR_ATTRIBUTE_TYPE_INVALID) { token_supports_allowed_mechs = CK_FALSE; diff --git a/src/util.c b/src/util.c index f443d15f..84eac250 100644 --- a/src/util.c +++ b/src/util.c @@ -9,14 +9,6 @@ #include #include -#define FA_RETURN_VAL(x, _a, _b) \ - do { \ - *x.value_ptr = _a; \ - *x.value_len_ptr = _b; \ - } while (0) - -#define FA_RETURN_LEN(x, _a) *x.value_len_ptr = _a - CK_RV p11prov_fetch_attributes(P11PROV_CTX *ctx, P11PROV_SESSION *session, CK_OBJECT_HANDLE object, struct fetch_attrs *attrs, @@ -28,11 +20,7 @@ CK_RV p11prov_fetch_attributes(P11PROV_CTX *ctx, P11PROV_SESSION *session, CK_RV ret; for (size_t i = 0; i < attrnums; i++) { - if (attrs[i].allocate) { - CKATTR_ASSIGN(q[i], attrs[i].type, NULL, 0); - } else { - CKATTR_SET(q[i], attrs[i]); - } + q[i] = attrs[i].attr; } /* try one shot, then fallback to individual calls if that fails */ @@ -41,26 +29,24 @@ CK_RV p11prov_fetch_attributes(P11PROV_CTX *ctx, P11PROV_SESSION *session, unsigned long retrnums = 0; for (size_t i = 0; i < attrnums; i++) { if (q[i].ulValueLen == CK_UNAVAILABLE_INFORMATION) { - if (attrs[i].required) { - return CKR_HOST_MEMORY; - } - FA_RETURN_LEN(attrs[i], 0); - continue; + /* This can't happen according to the algorithm described + * in the spec when the call returns CKR_OK. */ + return CKR_GENERAL_ERROR; } if (attrs[i].allocate) { - /* always allocate and zero one more, so that - * zero terminated strings work automatically */ - uint8_t *a = OPENSSL_zalloc(q[i].ulValueLen + 1); - if (a == NULL) { + /* always allocate one more, so that zero terminated strings + * work automatically */ + q[i].pValue = OPENSSL_zalloc(q[i].ulValueLen + 1); + if (!q[i].pValue) { return CKR_HOST_MEMORY; } - FA_RETURN_VAL(attrs[i], a, q[i].ulValueLen); - - CKATTR_SET(r[retrnums], attrs[i]); + /* add to re-request list */ + r[retrnums] = q[i]; retrnums++; - } else { - FA_RETURN_LEN(attrs[i], q[i].ulValueLen); } + /* always return data to caller so memory can be properly freed if + * necessary */ + attrs[i].attr = q[i]; } if (retrnums > 0) { ret = p11prov_GetAttributeValue(ctx, sess, object, r, retrnums); @@ -73,33 +59,36 @@ CK_RV p11prov_fetch_attributes(P11PROV_CTX *ctx, P11PROV_SESSION *session, * and does not handle it gracefully */ for (size_t i = 0; i < attrnums; i++) { if (attrs[i].allocate) { - CKATTR_ASSIGN(q[0], attrs[i].type, NULL, 0); - ret = p11prov_GetAttributeValue(ctx, sess, object, q, 1); + ret = p11prov_GetAttributeValue(ctx, sess, object, + &attrs[i].attr, 1); if (ret != CKR_OK) { if (attrs[i].required) { return ret; } } else { - uint8_t *a = OPENSSL_zalloc(q[0].ulValueLen + 1); - if (a == NULL) { + attrs[i].attr.pValue = + OPENSSL_zalloc(attrs[i].attr.ulValueLen + 1); + if (!attrs[i].attr.pValue) { return CKR_HOST_MEMORY; } - FA_RETURN_VAL(attrs[i], a, q[0].ulValueLen); } } - CKATTR_SET(r[0], attrs[i]); - ret = p11prov_GetAttributeValue(ctx, sess, object, r, 1); + ret = + p11prov_GetAttributeValue(ctx, sess, object, &attrs[i].attr, 1); if (ret != CKR_OK) { - if (r[0].ulValueLen == CK_UNAVAILABLE_INFORMATION) { - FA_RETURN_LEN(attrs[i], 0); - } if (attrs[i].required) { return ret; + } else { + if (attrs[i].allocate && attrs[i].attr.pValue) { + OPENSSL_free(attrs[i].attr.pValue); + attrs[i].attr.pValue = NULL; + attrs[i].attr.ulValueLen = CK_UNAVAILABLE_INFORMATION; + } } } P11PROV_debug("Attribute| type:0x%08lX value:%p, len:%lu", - attrs[i].type, *attrs[i].value_ptr, - *attrs[i].value_len_ptr); + attrs[i].attr.type, attrs[i].attr.pValue, + attrs[i].attr.ulValueLen); } ret = CKR_OK; } @@ -111,17 +100,12 @@ void p11prov_move_alloc_attrs(struct fetch_attrs *attrs, int num, { int c = *ck_num; for (int i = 0; i < num; i++) { - if (!attrs[i].allocate) { - continue; - } - if (*attrs[i].value_len_ptr > 0) { - ck_attrs[c].type = attrs[i].type; - ck_attrs[c].pValue = *attrs[i].value_ptr; - ck_attrs[c].ulValueLen = *attrs[i].value_len_ptr; + if (attrs[i].allocate && attrs[i].attr.pValue) { + ck_attrs[c] = attrs[i].attr; c++; - /* clear moved values */ - *attrs[i].value_ptr = NULL; - *attrs[i].value_len_ptr = 0; + /* clear moved values for good measure */ + attrs[i].attr.pValue = NULL; + attrs[i].attr.ulValueLen = CK_UNAVAILABLE_INFORMATION; } } *ck_num = c; @@ -131,7 +115,7 @@ void p11prov_fetch_attrs_free(struct fetch_attrs *attrs, int num) { for (int i = 0; i < num; i++) { if (attrs[i].allocate) { - OPENSSL_free(*attrs[i].value_ptr); + OPENSSL_free(attrs[i].attr.pValue); } } } diff --git a/src/util.h b/src/util.h index fbae3b90..044d075d 100644 --- a/src/util.h +++ b/src/util.h @@ -4,37 +4,30 @@ #ifndef _UTIL_H #define _UTIL_H +#define CKATTR_ASSIGN(x, _a, _b, _c) \ + do { \ + x.type = (_a); \ + x.pValue = (void *)(_b); \ + x.ulValueLen = (_c); \ + } while (0) + /* Utilities to fetch objects from tokens */ struct fetch_attrs { - CK_ATTRIBUTE_TYPE type; - CK_BYTE **value_ptr; - CK_ULONG *value_len_ptr; + CK_ATTRIBUTE attr; bool allocate; bool required; - - /* auxiliary members to make life easier */ - CK_BYTE *value; - CK_ULONG value_len; }; -#define FA_SET_BUF_VAL(x, n, _t, _v, _l, _a, _r) \ +#define FA_SET_BUF_VAL(x, n, _t, _v, _l, _r) \ do { \ - x[n].type = _t; \ - x[n].value_ptr = (CK_BYTE_PTR *)&_v; \ - x[n].value_len_ptr = &_l; \ - x[n].allocate = _a; \ + CKATTR_ASSIGN(x[n].attr, _t, _v, _l); \ + x[n].allocate = false; \ x[n].required = _r; \ - x[n].value = NULL; \ - x[n].value_len = 0; \ n++; \ } while (0) #define FA_SET_BUF_ALLOC(x, n, _t, _r) \ do { \ - x[n].type = _t; \ - x[n].value = NULL; \ - x[n].value_len = 0; \ - x[n].value_ptr = &x[n].value; \ - x[n].value_len_ptr = &x[n].value_len; \ + CKATTR_ASSIGN(x[n].attr, _t, NULL, 0); \ x[n].allocate = true; \ x[n].required = _r; \ n++; \ @@ -42,41 +35,13 @@ struct fetch_attrs { #define FA_SET_VAR_VAL(x, n, _t, _v, _r) \ do { \ - x[n].type = _t; \ - x[n].value = (CK_BYTE *)&_v; \ - x[n].value_len = sizeof(_v); \ - x[n].value_ptr = &x[n].value; \ - x[n].value_len_ptr = &x[n].value_len; \ + CKATTR_ASSIGN(x[n].attr, _t, (CK_BYTE *)&(_v), sizeof(_v)); \ x[n].allocate = false; \ x[n].required = _r; \ n++; \ } while (0) -#define FA_GET_VAL(x, n) *x[n].value_ptr -#define FA_GET_LEN(x, n) *x[n].value_len_ptr - -#define CKATTR_ASSIGN(x, _a, _b, _c) \ - do { \ - x.type = (_a); \ - x.pValue = (void *)(_b); \ - x.ulValueLen = (_c); \ - } while (0) - -#define CKATTR_SET(x, y) \ - do { \ - x.type = y.type; \ - x.pValue = *y.value_ptr; \ - x.ulValueLen = *y.value_len_ptr; \ - } while (0) - -#define CKATTR_MOVE(x, y) \ - do { \ - x.type = y.type; \ - x.pValue = *y.value_ptr; \ - x.ulValueLen = *y.value_len_ptr; \ - *y.value_ptr = NULL; \ - *y.value_len_ptr = 0; \ - } while (0) +#define FA_GET_LEN(x, n, _l) (_l) = x[n].attr.ulValueLen CK_RV p11prov_fetch_attributes(P11PROV_CTX *ctx, P11PROV_SESSION *session, CK_OBJECT_HANDLE object,