Skip to content

(xmlsec-nss) Hardened key transport / key agreement size checks, improved KW/KDF performance via context caching and in-place scratch staging, and cleaned up dead code #1386

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
lsh123 wants to merge 1 commit into from
Closed

(xmlsec-nss) Hardened key transport / key agreement size checks, improved KW/KDF performance via context caching and in-place scratch staging, and cleaned up dead code #1386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
193 changes: 193 additions & 0 deletions REVIEW-REPORT.md
View file
Open in desktop

Large diffs are not rendered by default.

90 changes: 55 additions & 35 deletions src/nss/crypto.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,16 @@

static xmlSecCryptoDLFunctionsPtr gXmlSecNssFunctions = NULL;

/*
* NSS 3.103 and later have dedicated OIDs (SEC_OID_ECDH_KEA and
* SEC_OID_X25519) that allow the ECDH and X25519 key agreement algorithms
* to be checked against the NSS security policy; older NSS does not, so the
* closest available proxy, the CKM_ECDH1_DERIVE mechanism, is used for the
* policy check there.
*/
#if (NSS_VMAJOR > 3) || ((NSS_VMAJOR == 3) && (NSS_VMINOR >= 103))
# define XMLSEC_NSS_HAS_KEY_AGREEMENT_OIDS 1
#endif

/*
* Checks if a given algorithm is enabled in NSS.
Expand Down Expand Up @@ -507,24 +517,22 @@ xmlSecNssUpdateAvailableCryptoTransforms(xmlSecCryptoDLFunctionsPtr functions) {

/****** CHACHA20 ******/
#ifndef XMLSEC_NO_CHACHA20
/*
* NSS has no OID for ChaCha20-Poly1305, so its availability cannot be
* checked against the NSS security policy; the transform is left
* registered and will fail at runtime if the mechanism is unsupported.
*/
if (xmlSecNssCryptoCheckAlgorithm(SEC_OID_CHACHA20_POLY1305) == 0) {
functions->transformChaCha20Poly1305GetKlass = NULL;
}
#endif /* XMLSEC_NO_CHACHA20 */

/****** DSA ******/
#ifndef XMLSEC_NO_DSA

#ifndef XMLSEC_NO_SHA1
if (xmlSecNssCryptoCheckAlgorithm(SEC_OID_ANSIX9_DSA_SIGNATURE_WITH_SHA1_DIGEST) == 0) {
if ((xmlSecNssCryptoCheckAlgorithm(SEC_OID_ANSIX9_DSA_SIGNATURE_WITH_SHA1_DIGEST) == 0) || (xmlSecNssCryptoCheckAlgorithm(SEC_OID_SHA1) == 0)) {
functions->transformDsaSha1GetKlass = NULL;
}
#endif /* XMLSEC_NO_SHA1 */

#ifndef XMLSEC_NO_SHA256
if (xmlSecNssCryptoCheckAlgorithm(SEC_OID_NIST_DSA_SIGNATURE_WITH_SHA256_DIGEST) == 0) {
if ((xmlSecNssCryptoCheckAlgorithm(SEC_OID_NIST_DSA_SIGNATURE_WITH_SHA256_DIGEST) == 0) || (xmlSecNssCryptoCheckAlgorithm(SEC_OID_SHA256) == 0)) {
functions->transformDsaSha256GetKlass = NULL;
}
#endif /* XMLSEC_NO_SHA256 */
Expand All @@ -533,50 +541,70 @@ xmlSecNssUpdateAvailableCryptoTransforms(xmlSecCryptoDLFunctionsPtr functions) {

/****** XDH ******/
#ifndef XMLSEC_NO_XDH
/*
* NSS has no X25519-specific OID, so the ECDH-derive mechanism (which
#ifdef XMLSEC_NSS_HAS_KEY_AGREEMENT_OIDS
if (xmlSecNssCryptoCheckAlgorithm(SEC_OID_X25519) == 0) {
functions->transformX25519GetKlass = NULL;
}
#else
/* NSS has no X25519-specific OID, so the ECDH-derive mechanism (which
* maps to the ECDSA OID) is used as the closest available proxy for the
* NSS security policy check.
*/
* NSS security policy check */
if (xmlSecNssCryptoCheckMechanism(CKM_ECDH1_DERIVE) == 0) {
functions->transformX25519GetKlass = NULL;
}
#endif /* XMLSEC_NSS_HAS_KEY_AGREEMENT_OIDS */
#endif /* XMLSEC_NO_XDH */

/****** ECDSA ******/
#ifndef XMLSEC_NO_EC

/* key agreement (ECDH-ES): uses the same derive mechanism as X25519 */
#ifdef XMLSEC_NSS_HAS_KEY_AGREEMENT_OIDS
/*
* Key agreement (ECDH-ES) uses the ECDH-derive mechanism. Several OIDs
* share CKM_ECDH1_DERIVE and NSS's mechanism map keeps only the last one
* (SEC_OID_ECDH_KEA), so that tag is what the security policy check
* resolves to; check it explicitly.
*/
if (xmlSecNssCryptoCheckAlgorithm(SEC_OID_ECDH_KEA) == 0) {
functions->transformEcdhGetKlass = NULL;
}
#else
/*
* Key agreement (ECDH-ES) uses the ECDH-derive mechanism; older NSS has
* no dedicated OID for it, so the mechanism itself is the closest
* available proxy for the NSS security policy check
*/
if (xmlSecNssCryptoCheckMechanism(CKM_ECDH1_DERIVE) == 0) {
functions->transformEcdhGetKlass = NULL;
}
#endif /* XMLSEC_NSS_HAS_KEY_AGREEMENT_OIDS */

#ifndef XMLSEC_NO_SHA1
if (xmlSecNssCryptoCheckAlgorithm(SEC_OID_ANSIX962_ECDSA_SHA1_SIGNATURE) == 0) {
if ((xmlSecNssCryptoCheckAlgorithm(SEC_OID_ANSIX962_ECDSA_SHA1_SIGNATURE) == 0) || (xmlSecNssCryptoCheckAlgorithm(SEC_OID_SHA1) == 0)) {
functions->transformEcdsaSha1GetKlass = NULL;
}
#endif /* XMLSEC_NO_SHA1 */

#ifndef XMLSEC_NO_SHA224
if (xmlSecNssCryptoCheckAlgorithm(SEC_OID_ANSIX962_ECDSA_SHA224_SIGNATURE) == 0) {
if ((xmlSecNssCryptoCheckAlgorithm(SEC_OID_ANSIX962_ECDSA_SHA224_SIGNATURE) == 0) || (xmlSecNssCryptoCheckAlgorithm(SEC_OID_SHA224) == 0)) {
functions->transformEcdsaSha224GetKlass = NULL;
}
#endif /* XMLSEC_NO_SHA224 */

#ifndef XMLSEC_NO_SHA256
if (xmlSecNssCryptoCheckAlgorithm(SEC_OID_ANSIX962_ECDSA_SHA256_SIGNATURE) == 0) {
if ((xmlSecNssCryptoCheckAlgorithm(SEC_OID_ANSIX962_ECDSA_SHA256_SIGNATURE) == 0) || (xmlSecNssCryptoCheckAlgorithm(SEC_OID_SHA256) == 0)) {
functions->transformEcdsaSha256GetKlass = NULL;
}
#endif /* XMLSEC_NO_SHA256 */

#ifndef XMLSEC_NO_SHA384
if (xmlSecNssCryptoCheckAlgorithm(SEC_OID_ANSIX962_ECDSA_SHA384_SIGNATURE) == 0) {
if ((xmlSecNssCryptoCheckAlgorithm(SEC_OID_ANSIX962_ECDSA_SHA384_SIGNATURE) == 0) || (xmlSecNssCryptoCheckAlgorithm(SEC_OID_SHA384) == 0)) {
functions->transformEcdsaSha384GetKlass = NULL;
}
#endif /* XMLSEC_NO_SHA384 */

#ifndef XMLSEC_NO_SHA512
if (xmlSecNssCryptoCheckAlgorithm(SEC_OID_ANSIX962_ECDSA_SHA512_SIGNATURE) == 0) {
if ((xmlSecNssCryptoCheckAlgorithm(SEC_OID_ANSIX962_ECDSA_SHA512_SIGNATURE) == 0) || (xmlSecNssCryptoCheckAlgorithm(SEC_OID_SHA512) == 0)) {
functions->transformEcdsaSha512GetKlass = NULL;
}
#endif /* XMLSEC_NO_SHA512 */
Expand All @@ -594,14 +622,6 @@ xmlSecNssUpdateAvailableCryptoTransforms(xmlSecCryptoDLFunctionsPtr functions) {
/****** HMAC ******/
#ifndef XMLSEC_NO_HMAC

#ifndef XMLSEC_NO_RIPEMD160
/*
* The NSS softoken does not support RipeMD160 and there is no OID
* mapping for CKM_RIPEMD160_HMAC, so this transform is never available.
*/
functions->transformHmacRipemd160GetKlass = NULL;
#endif /* XMLSEC_NO_RIPEMD160 */

#ifndef XMLSEC_NO_SHA1
if (xmlSecNssCryptoCheckMechanism(CKM_SHA_1_HMAC) == 0) {
functions->transformHmacSha1GetKlass = NULL;
Expand Down Expand Up @@ -653,7 +673,7 @@ xmlSecNssUpdateAvailableCryptoTransforms(xmlSecCryptoDLFunctionsPtr functions) {
#ifndef XMLSEC_NO_RSA

#ifndef XMLSEC_NO_SHA1
if (xmlSecNssCryptoCheckAlgorithm(SEC_OID_PKCS1_SHA1_WITH_RSA_ENCRYPTION) == 0) {
if ((xmlSecNssCryptoCheckAlgorithm(SEC_OID_PKCS1_SHA1_WITH_RSA_ENCRYPTION) == 0) || (xmlSecNssCryptoCheckAlgorithm(SEC_OID_SHA1) == 0)) {
functions->transformRsaSha1GetKlass = NULL;
}

Expand All @@ -663,7 +683,7 @@ xmlSecNssUpdateAvailableCryptoTransforms(xmlSecCryptoDLFunctionsPtr functions) {
#endif /* XMLSEC_NO_SHA1 */

#ifndef XMLSEC_NO_SHA224
if (xmlSecNssCryptoCheckAlgorithm(SEC_OID_PKCS1_SHA224_WITH_RSA_ENCRYPTION) == 0) {
if ((xmlSecNssCryptoCheckAlgorithm(SEC_OID_PKCS1_SHA224_WITH_RSA_ENCRYPTION) == 0) || (xmlSecNssCryptoCheckAlgorithm(SEC_OID_SHA224) == 0)) {
functions->transformRsaSha224GetKlass = NULL;
}

Expand All @@ -673,7 +693,7 @@ xmlSecNssUpdateAvailableCryptoTransforms(xmlSecCryptoDLFunctionsPtr functions) {
#endif /* XMLSEC_NO_SHA224 */

#ifndef XMLSEC_NO_SHA256
if (xmlSecNssCryptoCheckAlgorithm(SEC_OID_PKCS1_SHA256_WITH_RSA_ENCRYPTION) == 0) {
if ((xmlSecNssCryptoCheckAlgorithm(SEC_OID_PKCS1_SHA256_WITH_RSA_ENCRYPTION) == 0) || (xmlSecNssCryptoCheckAlgorithm(SEC_OID_SHA256) == 0)) {
functions->transformRsaSha256GetKlass = NULL;
}

Expand All @@ -683,7 +703,7 @@ xmlSecNssUpdateAvailableCryptoTransforms(xmlSecCryptoDLFunctionsPtr functions) {
#endif /* XMLSEC_NO_SHA256 */

#ifndef XMLSEC_NO_SHA384
if (xmlSecNssCryptoCheckAlgorithm(SEC_OID_PKCS1_SHA384_WITH_RSA_ENCRYPTION) == 0) {
if ((xmlSecNssCryptoCheckAlgorithm(SEC_OID_PKCS1_SHA384_WITH_RSA_ENCRYPTION) == 0) || (xmlSecNssCryptoCheckAlgorithm(SEC_OID_SHA384) == 0)) {
functions->transformRsaSha384GetKlass = NULL;
}

Expand All @@ -693,7 +713,7 @@ xmlSecNssUpdateAvailableCryptoTransforms(xmlSecCryptoDLFunctionsPtr functions) {
#endif /* XMLSEC_NO_SHA384 */

#ifndef XMLSEC_NO_SHA512
if (xmlSecNssCryptoCheckAlgorithm(SEC_OID_PKCS1_SHA512_WITH_RSA_ENCRYPTION) == 0) {
if ((xmlSecNssCryptoCheckAlgorithm(SEC_OID_PKCS1_SHA512_WITH_RSA_ENCRYPTION) == 0) || (xmlSecNssCryptoCheckAlgorithm(SEC_OID_SHA512) == 0)) {
functions->transformRsaSha512GetKlass = NULL;
}

Expand Down Expand Up @@ -896,19 +916,19 @@ xmlSecNssGenerateRandom(xmlSecBufferPtr buffer, xmlSecSize size) {
xmlSecAssert2(buffer != NULL, -1);
xmlSecAssert2(size > 0, -1);

/* check the size fits into int before allocating */
XMLSEC_SAFE_CAST_SIZE_TO_INT(size, len, return(-1), NULL);

ret = xmlSecBufferSetSize(buffer, size);
if(ret < 0) {
xmlSecInternalError2("xmlSecBufferSetSize", NULL,
"size=" XMLSEC_SIZE_FMT, size);
xmlSecInternalError2("xmlSecBufferSetSize", NULL, "size=" XMLSEC_SIZE_FMT, size);
return(-1);
}

/* get random data */
XMLSEC_SAFE_CAST_SIZE_TO_INT(size, len, return(-1), NULL);
rv = PK11_GenerateRandom((xmlSecByte*)xmlSecBufferGetData(buffer), len);
if(rv != SECSuccess) {
xmlSecNssError2("PK11_GenerateRandom", NULL,
"size=" XMLSEC_SIZE_FMT, size);
xmlSecNssError2("PK11_GenerateRandom", NULL, "size=" XMLSEC_SIZE_FMT, size);
return(-1);
}
return(0);
Expand Down
48 changes: 22 additions & 26 deletions src/nss/kdf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,8 @@ xmlSecNssConcatKdfGenerateKey(xmlSecNssKdfCtxPtr ctx, xmlSecSize outLen, xmlSecB
xmlSecByte hashBuf[XMLSEC_NSS_KDF_MAX_HASH_SIZE];
xmlSecByte counter[4];
uint32_t counterVal;
PK11Context* hashCtx;
xmlSecSize toCopy, hashSize;
PK11Context* hashCtx = NULL;
SECStatus rv;
int ret;
int res = -1;
Expand Down Expand Up @@ -442,6 +443,15 @@ xmlSecNssConcatKdfGenerateKey(xmlSecNssKdfCtxPtr ctx, xmlSecSize outLen, xmlSecB
outData = xmlSecBufferGetData(out);
xmlSecAssert2(outData != NULL, -1);

/* one digest context is reused for all counter blocks: PK11_DigestBegin
* starts a fresh operation on each block, since PK11_DigestFinal clears
* the previous one (Begin after Final is an explicit NSS-supported pattern) */
hashCtx = PK11_CreateDigestContext(oidData->offset);
if(hashCtx == NULL) {
xmlSecNssError("PK11_CreateDigestContext", NULL);
goto done;
}

pos = 0;
counterVal = 1;
while(pos < outLen) {
Expand All @@ -456,74 +466,60 @@ xmlSecNssConcatKdfGenerateKey(xmlSecNssKdfCtxPtr ctx, xmlSecSize outLen, xmlSecB
counter[2] = (xmlSecByte)((counterVal >> 8) & 0xFF);
counter[3] = (xmlSecByte)(counterVal & 0xFF);

hashCtx = PK11_CreateDigestContext(oidData->offset);
if(hashCtx == NULL) {
xmlSecNssError("PK11_CreateDigestContext", NULL);
goto done;
}

rv = PK11_DigestBegin(hashCtx);
if(rv != SECSuccess) {
xmlSecNssError("PK11_DigestBegin", NULL);
PK11_DestroyContext(hashCtx, PR_TRUE);
goto done;
}

rv = PK11_DigestOp(hashCtx, counter, 4);
if(rv != SECSuccess) {
xmlSecNssError("PK11_DigestOp(counter)", NULL);
PK11_DestroyContext(hashCtx, PR_TRUE);
goto done;
}

XMLSEC_SAFE_CAST_SIZE_TO_UINT(keySize, keyLen,
PK11_DestroyContext(hashCtx, PR_TRUE); goto done, NULL);
XMLSEC_SAFE_CAST_SIZE_TO_UINT(keySize, keyLen, goto done, NULL);
rv = PK11_DigestOp(hashCtx, keyData, keyLen);
if(rv != SECSuccess) {
xmlSecNssError("PK11_DigestOp(Z)", NULL);
PK11_DestroyContext(hashCtx, PR_TRUE);
goto done;
}

if((fixedInfoData != NULL) && (fixedInfoSize > 0)) {
unsigned int fixedInfoLen;

XMLSEC_SAFE_CAST_SIZE_TO_UINT(fixedInfoSize, fixedInfoLen,
PK11_DestroyContext(hashCtx, PR_TRUE); goto done, NULL);
XMLSEC_SAFE_CAST_SIZE_TO_UINT(fixedInfoSize, fixedInfoLen, goto done, NULL);
rv = PK11_DigestOp(hashCtx, fixedInfoData, fixedInfoLen);
if(rv != SECSuccess) {
xmlSecNssError("PK11_DigestOp(OtherInfo)", NULL);
PK11_DestroyContext(hashCtx, PR_TRUE);
goto done;
}
}

hashLen = XMLSEC_NSS_KDF_MAX_HASH_SIZE;
rv = PK11_DigestFinal(hashCtx, hashBuf, &hashLen, XMLSEC_NSS_KDF_MAX_HASH_SIZE);
PK11_DestroyContext(hashCtx, PR_TRUE);
if(rv != SECSuccess) {
xmlSecNssError("PK11_DigestFinal", NULL);
goto done;
}
XMLSEC_SAFE_CAST_UINT_TO_SIZE(hashLen, hashSize, goto done, NULL);

{
xmlSecSize toCopy;

toCopy = outLen - pos;
if(toCopy > (xmlSecSize)hashLen) {
toCopy = (xmlSecSize)hashLen;
}
memcpy(outData + pos, hashBuf, toCopy);
pos += toCopy;
toCopy = outLen - pos;
if(toCopy > hashSize) {
toCopy = hashSize;
}

memcpy(outData + pos, hashBuf, toCopy);
pos += toCopy;
counterVal++;
}

/* success */
res = 0;

done:
if(hashCtx != NULL) {
PK11_DestroyContext(hashCtx, PR_TRUE);
}
xmlSecMemCleanse(hashBuf, sizeof(hashBuf));
return(res);
}
Expand Down
Loading
Loading