Skip to content

Commit

Permalink
Simplify utility to fetch attributes
Browse files Browse the repository at this point in the history 
Reduce the size and reuse the CK_ATTRIBUTE structure within fetch_attrs
to further simplify copying data between attribute storages when needed.

Also fix some checks in the derive key functions that I found wrong
while refactoring.

Signed-off-by: Simo Sorce <simo@redhat.com>
  • Loading branch information
@simo5
simo5 committed Feb 7, 2023
1 parent f8076a6 commit 1cd2be8
Show file tree
Hide file tree
Showing 5 changed files with 69 additions and 118 deletions.
18 changes: 6 additions & 12 deletions src/exchange.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) },
Expand All @@ -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;
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -292,15 +286,15 @@ 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);
if (ret != CKR_OK) {
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;
}

Expand Down
13 changes: 10 additions & 3 deletions src/kdf.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}

Expand Down
9 changes: 5 additions & 4 deletions src/objects.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
84 changes: 34 additions & 50 deletions src/util.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,6 @@
#include <openssl/bn.h>
#include <openssl/x509.h>

#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,
Expand All @@ -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 */
Expand All @@ -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);
Expand All @@ -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;
}
Expand All @@ -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;
Expand All @@ -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);
}
}
}
Expand Down
63 changes: 14 additions & 49 deletions src/util.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,79 +4,44 @@
#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++; \
} while (0)

#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,
Expand Down

0 comments on commit 1cd2be8

Please sign in to comment.