Skip to content

(xmlsec-gcrypt) Fixed PKCS#8 key rejection, native EC key adoption, and input-size validations #1376

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

Merged
lsh123 merged 1 commit into from
Oct 5, 2026
Merged

(xmlsec-gcrypt) Fixed PKCS#8 key rejection, native EC key adoption, and input-size validations #1376

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
22 changes: 18 additions & 4 deletions src/gcrypt/app.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,11 @@
#include "asn1.h"
#include "../cast_helpers.h"

/* the flag that indicates whether the secure memory was initialized
(GCRYCTL_INIT_SECMEM can fail; the error is ignored because of
a known libgcrypt bug, see https://github.com/lsh123/xmlsec/issues/415) */
static int g_xmlSecGCryptSecureMemoryInitialized = 0;

/**
* @brief Initializes the GCrypt crypto engine.
* @details General crypto engine initialization. This function is used
Expand Down Expand Up @@ -95,6 +100,9 @@ Noteworthy changes in version 1.4.3 (2008-09-18)
xmlSecGCryptError("gcry_control(GCRYCTL_INIT_SECMEM)", err, NULL);
/* ignore this error because of libgcrypt bug in allocating memory,
see https://github.com/lsh123/xmlsec/issues/415 for more details */
g_xmlSecGCryptSecureMemoryInitialized = 0;
} else {
g_xmlSecGCryptSecureMemoryInitialized = 1;
}

/* It is now okay to let Libgcrypt complain when there was/is
Expand Down Expand Up @@ -130,10 +138,16 @@ int
xmlSecGCryptAppShutdown(void) {
gcry_error_t err;

err = gcry_control(GCRYCTL_TERM_SECMEM);
if(err != GPG_ERR_NO_ERROR) {
xmlSecGCryptError("gcry_control(GCRYCTL_TERM_SECMEM)", err, NULL);
return(-1);
/* only terminate the secure memory if it was actually initialized;
GCRYCTL_TERM_SECMEM can fail (e.g. GPG_ERR_CONFLICT) when the
secure memory was never allocated (e.g. because the ignored
GCRYCTL_INIT_SECMEM failure happened during initialization) */
if(g_xmlSecGCryptSecureMemoryInitialized) {
err = gcry_control(GCRYCTL_TERM_SECMEM);
if(err != GPG_ERR_NO_ERROR) {
xmlSecGCryptError("gcry_control(GCRYCTL_TERM_SECMEM)", err, NULL);
return(-1);
}
}

/* done */
Expand Down
46 changes: 33 additions & 13 deletions src/gcrypt/asn1.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,15 @@ xmlSecGCryptAsn1ParseIntegerSequence(int level, xmlSecByte const **buffer, xmlSe

/* expect an INTEGER tag */
if((remaining < 1) || (*p != TAG_INTEGER)) {
xmlSecInternalError2("xmlSecGCryptAsn1ParseIntegerSequence", NULL, "INTEGER expected inside BIT STRING, remaining=%lu", remaining);
if((remaining >= 1) && (*p == 0x30)) {
/* a SEQUENCE inside the BIT STRING indicates an
unsupported key format, e.g. an RSA key in
SPKI/SubjectPublicKeyInfo form; only the
traditional (PKCS#1) RSA formats are supported */
xmlSecInvalidDataError("unsupported key format: a SEQUENCE was found inside a BIT STRING (e.g. an RSA key in SPKI/SubjectPublicKeyInfo form); use the traditional PKCS#1 format instead", NULL);
} else {
xmlSecInternalError2("xmlSecGCryptAsn1ParseIntegerSequence", NULL, "INTEGER expected inside BIT STRING, remaining=%lu", remaining);
}
return(-1);
}
p++; remaining--;
Expand Down Expand Up @@ -436,21 +444,27 @@ xmlSecGCryptAsn1GuessKeyType(gcry_mpi_t * integers, xmlSecSize integers_num, xml
}

/* try other keys */
/* Note: this guessing is inherently heuristic. A malformed key (for example,
* an EC private key missing both its curve OID and its public key, which
* flattens to exactly two integers with no OIDs) can be misidentified as a
* different type (here, an RSA public key with a garbage modulus of 0 or 1).
* Such a misidentified key is invalid and is rejected by libgcrypt on use, so
* this cannot lead to a verification bypass; it only affects how malformed
* input is reported. This is therefore not a security defect. */
/* Note: this guessing is inherently heuristic. An RSA public key is only
* recognized when the first integer (the modulus) is at least 512 bits long;
* a 2-integer key with a small first integer (for example, an EC private key
* missing both its curve OID and its optional public point, which flattens to
* version + d) is not treated as an RSA key and is rejected with a
* "number of parameters" error below instead of a garbage key being built. */
switch(integers_num) {
case XMLSEC_GCRYPT_ASN1_DSA_PUB_NUM:
return(xmlSecGCryptDerKeyTypePublicDsa);
case XMLSEC_GCRYPT_ASN1_DSA_PRIV_NUM:
return(xmlSecGCryptDerKeyTypePrivateDsa);

case XMLSEC_GCRYPT_ASN1_RSA_PUB_NUM:
return(xmlSecGCryptDerKeyTypePublicRsa);
/* a real RSA modulus is at least 512 bits long; 512 is a heuristic
threshold (libgcrypt itself accepts smaller moduli), used to tell a
valid RSAPublicKey apart from other 2-integer shapes, e.g. an EC
private key (version + d) missing its curve OID */
if(gcry_mpi_get_nbits(integers[0]) >= 512) {
return(xmlSecGCryptDerKeyTypePublicRsa);
}
return(xmlSecGCryptDerKeyTypeAuto);
case XMLSEC_GCRYPT_ASN1_RSA_PRIV_NUM:
return(xmlSecGCryptDerKeyTypePrivateRsa);
default:
Expand Down Expand Up @@ -516,10 +530,16 @@ xmlSecGCryptParseDer(const xmlSecByte * der, xmlSecSize derlen,
}

/* PKCS#8-wrapped private keys (PrivateKeyInfo) are not supported and would be
* misparsed into a garbage key: they flatten to exactly two integers
* [version(0), <raw-key blob>] with an algorithm object id present. Detect this
* shape and fail instead of building a wrong key. */
if((integers_num == 2) && (objectids_num >= 1) && (gcry_mpi_get_nbits(integers[0]) == 0)) {
* misparsed into a garbage key: they always flatten to at least two integers
* starting with version(0) and carry an algorithm object id. For a PKCS#8 RSA
* key the shape is exactly two integers [version(0), <raw-key blob>], but a
* PKCS#8 DSA key flattens to more (the DSA parameters p, q, g are collected
* from the algorithm-params SEQUENCE), so detect any such shape (version(0) +
* >=1 more integer + an OID present) and fail instead of building a wrong key.
* No supported traditional format matches this: PKCS#1/traditional DSA/EC keys
* carry no OIDs, a SPKI DSA public key has integers[0]=p (non-zero), and a SPKI
* EC public key has exactly one integer. */
if((integers_num >= 2) && (objectids_num >= 1) && (gcry_mpi_get_nbits(integers[0]) == 0)) {
xmlSecInvalidDataError("PKCS#8 private keys are not supported; use a traditional format (PKCS#1 RSAPrivateKey, DSAPrivateKey, or ECPrivateKey)", NULL);
goto done;
}
Expand Down
87 changes: 75 additions & 12 deletions src/gcrypt/asymkeys.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1611,6 +1611,37 @@ xmlSecGCryptKeyDataEcGetKlass(void) {
return(&xmlSecGCryptKeyDataEcKlass);
}

/**
* @brief Checks that a GCrypt key S-expression is an EC key.
* @details Accepts both the "ecdsa" and the "ecc" algorithm token: "ecdsa" is
* the token used in the S-expressions constructed by this back-end, while "ecc"
* is the token that libgcrypt itself uses in the EC keys it creates
* (e.g. with gcry_pk_genkey).
* @param key the pointer to the GCrypt key S-expression (public key, private key, or key pair).
* @return 0 if the key is an EC key or -1 otherwise.
*/
static int
xmlSecGCryptKeyDataEcCheckKeyAlg(gcry_sexp_t key) {
gcry_sexp_t tok;

xmlSecAssert2(key != NULL, -1);

/* the algorithm token (ecdsa/ecc) is present in the public-key,
private-key and key-pair S-expressions alike, so its presence
unambiguously identifies the key type */
tok = gcry_sexp_find_token(key, "ecdsa", 0);
if(tok == NULL) {
tok = gcry_sexp_find_token(key, "ecc", 0);
}
if(tok == NULL) {
return(-1);
}
gcry_sexp_release(tok);

/* success */
return(0);
}

/**
* @brief Sets the value of EC key data.
* @details On success, @p ec_key will be owned by the @p data; on failure the
Expand All @@ -1624,7 +1655,7 @@ xmlSecGCryptKeyDataEcAdoptKey(xmlSecKeyDataPtr data, gcry_sexp_t ec_key) {
xmlSecAssert2(xmlSecKeyDataCheckId(data, xmlSecGCryptKeyDataEcId), -1);
xmlSecAssert2(ec_key != NULL, -1);

if(xmlSecGCryptAsymKeyDataCheckKeyAlg(ec_key, "ecdsa") < 0) {
if(xmlSecGCryptKeyDataEcCheckKeyAlg(ec_key) < 0) {
xmlSecInvalidDataError("the provided key is not an EC key", xmlSecKeyDataGetName(data));
return(-1);
}
Expand All @@ -1646,11 +1677,11 @@ xmlSecGCryptKeyDataEcAdoptKeyPair(xmlSecKeyDataPtr data, gcry_sexp_t pub_key, gc
xmlSecAssert2(xmlSecKeyDataCheckId(data, xmlSecGCryptKeyDataEcId), -1);
xmlSecAssert2(pub_key != NULL, -1);

if(xmlSecGCryptAsymKeyDataCheckKeyAlg(pub_key, "ecdsa") < 0) {
if(xmlSecGCryptKeyDataEcCheckKeyAlg(pub_key) < 0) {
xmlSecInvalidDataError("the provided key is not an EC key", xmlSecKeyDataGetName(data));
return(-1);
}
if((priv_key != NULL) && (xmlSecGCryptAsymKeyDataCheckKeyAlg(priv_key, "ecdsa") < 0)) {
if((priv_key != NULL) && (xmlSecGCryptKeyDataEcCheckKeyAlg(priv_key) < 0)) {
xmlSecInvalidDataError("the provided key is not an EC key", xmlSecKeyDataGetName(data));
return(-1);
}
Expand Down Expand Up @@ -1778,12 +1809,33 @@ typedef struct _xmlSecGCryptKeyDataEcCurveOidToName {
xmlChar curveOid[64];
} xmlSecGCryptKeyDataEcCurveOidToName;

/* The table contains both the curve names accepted by libgcrypt in the
S-expressions (secpNnnr1, primeNnnv1, secp256k1, brainpoolPNNNr1) and the
canonical names that libgcrypt itself stores in the S-expressions of the
keys it creates (e.g. "NIST P-256" for secp256r1/prime256v1), so that both
the keys constructed by this back-end and the libgcrypt-created keys can be
written to XML. */
static xmlSecGCryptKeyDataEcCurveOidToName g_xmlSecGCryptKeyDataEcCurveOidToName[] = {
{ "prime192v1", "1.2.840.10045.3.1.1" },
{ "prime256v1", "1.2.840.10045.3.1.7" },
{ "secp224r1", "1.3.132.0.33" },
{ "secp384r1", "1.3.132.0.34" },
{ "secp521r1", "1.3.132.0.35" }
{ "prime192v1", "1.2.840.10045.3.1.1" },
{ "prime256v1", "1.2.840.10045.3.1.7" },
{ "secp192r1", "1.3.132.0.32" },
{ "secp224r1", "1.3.132.0.33" },
{ "secp256r1", "1.2.840.10045.3.1.7" },
{ "secp384r1", "1.3.132.0.34" },
{ "secp521r1", "1.3.132.0.35" },
{ "secp256k1", "1.3.132.0.10" },
{ "brainpoolP160r1", "1.3.36.3.3.2.8.1.1.1" },
{ "brainpoolP192r1", "1.3.36.3.3.2.8.1.1.3" },
{ "brainpoolP224r1", "1.3.36.3.3.2.8.1.1.5" },
{ "brainpoolP256r1", "1.3.36.3.3.2.8.1.1.7" },
{ "brainpoolP320r1", "1.3.36.3.3.2.8.1.1.9" },
{ "brainpoolP384r1", "1.3.36.3.3.2.8.1.1.11" },
{ "brainpoolP512r1", "1.3.36.3.3.2.8.1.1.13" },
{ "NIST P-192", "1.2.840.10045.3.1.1" },
{ "NIST P-224", "1.3.132.0.33" },
{ "NIST P-256", "1.2.840.10045.3.1.7" },
{ "NIST P-384", "1.3.132.0.34" },
{ "NIST P-521", "1.3.132.0.35" }
};

static const char*
Expand Down Expand Up @@ -1834,11 +1886,17 @@ xmlSecGCryptKeyDataEcRead(xmlSecKeyDataId id, xmlSecKeyValueEcPtr ecValue) {
goto done;
}

/* pubkey */
if(xmlSecBufferGetSize(&(ecValue->pubkey)) == 0) {
xmlSecInvalidZeroKeyDataSizeError(xmlSecKeyDataKlassGetName(id));
/* check that the public key point is well-formed (odd size > 1 and the
leading uncompressed point magic byte 0x04); note that the point cannot
be validated to be on the curve, because libgcrypt does not expose the
curve parameters for that */
ret = xmlSecKeyDataEcPublicKeySplitComponents(ecValue);
if(ret < 0) {
xmlSecInternalError("xmlSecKeyDataEcPublicKeySplitComponents", xmlSecKeyDataKlassGetName(id));
goto done;
}

/* pubkey */
err = gcry_mpi_scan(&pubkey, GCRYMPI_FMT_USG,
xmlSecBufferGetData(&(ecValue->pubkey)), xmlSecBufferGetSize(&(ecValue->pubkey)),
NULL);
Expand Down Expand Up @@ -1946,9 +2004,14 @@ xmlSecGCryptKeyDataEcWrite(xmlSecKeyDataId id, xmlSecKeyDataPtr data, xmlSecKeyV
goto done;
}

/* the algorithm token is "ecdsa" for the S-expressions constructed by this
back-end and "ecc" for the keys created by libgcrypt itself */
s_ecdsa = gcry_sexp_find_token(s_pub_key, "ecdsa", 0);
if(s_ecdsa == NULL) {
xmlSecGCryptError("gcry_sexp_find_token(ecdsa)", (gcry_error_t)GPG_ERR_NO_ERROR,
s_ecdsa = gcry_sexp_find_token(s_pub_key, "ecc", 0);
}
if(s_ecdsa == NULL) {
xmlSecGCryptError("gcry_sexp_find_token(ecdsa/ecc)", (gcry_error_t)GPG_ERR_NO_ERROR,
xmlSecKeyDataKlassGetName(id));
goto done;
}
Expand Down
20 changes: 12 additions & 8 deletions src/gcrypt/ciphers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,11 +172,12 @@ xmlSecGCryptBlockCipherCtxUpdate(xmlSecGCryptBlockCipherCtxPtr ctx,
}
inSize = inBlocks * blockSize;

/* we write out the input size plus maybe one block.
*
* The size_t sum (outSize + inSize + blockSize) could in principle wrap on a
* 32-bit build, but only for multi-gigabyte buffers, which is not a realistic
* input size for XML Security processing; this matches the other backends. */
/* we write out the input size plus maybe one block */
if((inSize > XMLSEC_SIZE_MAX - blockSize) || (outSize > XMLSEC_SIZE_MAX - inSize - blockSize)) {
xmlSecInternalError4("xmlSecBufferSetMaxSize", cipherName,
"outSize=" XMLSEC_SIZE_FMT "; inSize=" XMLSEC_SIZE_FMT "; blockSize=" XMLSEC_SIZE_FMT, outSize, inSize, blockSize);
return(-1);
}
ret = xmlSecBufferSetMaxSize(out, outSize + inSize + blockSize);
if(ret < 0) {
xmlSecInternalError2("xmlSecBufferSetMaxSize", cipherName,
Expand Down Expand Up @@ -273,9 +274,12 @@ xmlSecGCryptBlockCipherCtxFinal(xmlSecGCryptBlockCipherCtxPtr ctx,
}
}

/* process last block. The size_t sum (outSize + 2 * blockSize) could in
* principle wrap on a 32-bit build, but only for multi-gigabyte buffers,
* which is not a realistic input size. */
/* process last block */
if((blockSize > (XMLSEC_SIZE_MAX / 2)) || (outSize > XMLSEC_SIZE_MAX - 2 * blockSize)) {
xmlSecInternalError3("xmlSecBufferSetMaxSize", cipherName,
"outSize=" XMLSEC_SIZE_FMT "; blockSize=" XMLSEC_SIZE_FMT, outSize, blockSize);
return(-1);
}
ret = xmlSecBufferSetMaxSize(out, outSize + 2 * blockSize);
if(ret < 0) {
xmlSecInternalError2("xmlSecBufferSetMaxSize", cipherName,
Expand Down
19 changes: 13 additions & 6 deletions src/gcrypt/crypto.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,12 +309,19 @@ xmlSecCryptoGetFunctions_gcrypt(void) {
*/
int
xmlSecGCryptInit (void) {
/* Note: this function does not perform explicit libgcrypt initialization
* (no gcry_check_version/gcry_control/gcry_init). The full libgcrypt
* initialization is performed by xmlSecGCryptAppInit (src/gcrypt/app.c),
* which the application invokes through xmlSecCryptoAppInit before calling
* xmlSecInit. Applications that skip the app-init step rely on libgcrypt's
* self-initialization, which was added in libgcrypt 1.4.3. */
/* Note: the full libgcrypt initialization (secure memory, etc.) is
* performed by xmlSecGCryptAppInit (src/gcrypt/app.c), which the
* application invokes through xmlSecCryptoAppInit before calling
* xmlSecInit. Applications that skip the app-init step rely on
* libgcrypt's self-initialization, which was added in libgcrypt 1.4.3;
* the version check below still makes sure that the linked libgcrypt is
* at least GCRYPT_MIN_VERSION (configure.ac defines GCRYPT_MIN_VERSION)
* and triggers the basic libgcrypt initialization. */
if(gcry_check_version(GCRYPT_MIN_VERSION) == NULL) {
xmlSecOtherError2(XMLSEC_ERRORS_R_CRYPTO_FAILED, NULL,
"gcry_check_version failed; min_version=%s", GCRYPT_MIN_VERSION);
return(-1);
}

/* Check loaded xmlsec library version */
if(xmlSecCheckVersionExact() != 1) {
Expand Down
5 changes: 2 additions & 3 deletions src/gcrypt/digests.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,9 +281,8 @@ xmlSecGCryptDigestExecute(xmlSecTransformPtr transform, int last, xmlSecTransfor

inSize = xmlSecBufferGetSize(in);
if(inSize > 0) {
/* The gcry_md_write() return value is not checked: given the validated
* context handle it effectively cannot fail. This is a codebase-wide
* pattern in the gcrypt backend. */
/* gcry_md_write() returns void (no error code), so there is
* nothing to check. */
gcry_md_write(ctx->digestCtx, xmlSecBufferGetData(in), inSize);

ret = xmlSecBufferRemoveHead(in, inSize);
Expand Down
14 changes: 10 additions & 4 deletions src/gcrypt/hmac.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,14 @@ xmlSecGCryptHmacVerify(xmlSecTransformPtr transform,
xmlSecAssert2(ctx->digestCtx != NULL, -1);
xmlSecAssert2(ctx->dgstSizeInBits > 0, -1);

/* the digest to verify must not be empty */
if(dataSize <= 0) {
xmlSecInvalidSizeError("HMAC digest", dataSize,
XMLSEC_BITS_TO_BYTES(ctx->dgstSizeInBits),
xmlSecTransformGetName(transform));
return(-1);
}

/* Returns 1 for match, 0 for no match, <0 for errors. */
ret = xmlSecTransformHmacVerify(data, dataSize, ctx->dgst, ctx->dgstSizeInBits, sizeof(ctx->dgst));
if(ret < 0) {
Expand Down Expand Up @@ -354,10 +362,8 @@ xmlSecGCryptHmacExecute(xmlSecTransformPtr transform, int last, xmlSecTransformC

inSize = xmlSecBufferGetSize(in);
if(inSize > 0) {
/* The gcry_md_write() return value is not checked: given the validated
* context handle it effectively cannot fail (if it ever did, the input
* chunk would be silently omitted from the HMAC). This is a codebase-wide
* pattern in the gcrypt backend. */
/* gcry_md_write() returns void (no error code), so there is
* nothing to check. */
gcry_md_write(ctx->digestCtx, xmlSecBufferGetData(in), inSize);

ret = xmlSecBufferRemoveHead(in, inSize);
Expand Down
Loading
Loading