From 26b613be5c4865079762771acbd5ba4efb515f66 Mon Sep 17 00:00:00 2001 From: Aleksey Sanin Date: Mon, 28 Nov 2022 13:40:25 -0500 Subject: [PATCH 1/2] fix missing return --- src/openssl/kt_rsa.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/openssl/kt_rsa.c b/src/openssl/kt_rsa.c index 4996720f5..894007eba 100644 --- a/src/openssl/kt_rsa.c +++ b/src/openssl/kt_rsa.c @@ -490,6 +490,7 @@ xmlSecOpenSSLRsaPkcs1Process(xmlSecTransformPtr transform) { if(ret < 0) { xmlSecInternalError("xmlSecOpenSSLRsaPkcs1ProcessImpl", xmlSecTransformGetName(transform)); + return(-1); } ret = xmlSecBufferSetSize(out, outSize); From 572edb4cabccfbf72a4ac0c06cf0ef5f0d718d2a Mon Sep 17 00:00:00 2001 From: Aleksey Sanin Date: Mon, 28 Nov 2022 15:32:47 -0500 Subject: [PATCH 2/2] audit all errors for returns --- scripts/check-return.pl | 23 +++++++++++++++++++++++ src/dl.c | 3 +++ src/gcrypt/signatures.c | 10 +++++----- src/gnutls/x509vfy.c | 6 +++--- src/keyinfo.c | 5 +++-- src/mscng/certkeys.c | 2 ++ src/mscng/x509.c | 4 ++++ src/mscng/x509vfy.c | 6 +++++- src/mscrypto/signatures.c | 2 +- src/nss/signatures.c | 2 +- src/openssl/evp_signatures.c | 2 +- src/openssl/kt_rsa.c | 1 + src/openssl/signatures.c | 2 +- src/openssl/x509vfy.c | 13 ++++++++----- src/xmldsig.c | 1 + src/xmlsec.c | 11 +++++++++-- src/xmltree.c | 1 + 17 files changed, 72 insertions(+), 22 deletions(-) create mode 100644 scripts/check-return.pl diff --git a/scripts/check-return.pl b/scripts/check-return.pl new file mode 100644 index 000000000..fdd1bcf56 --- /dev/null +++ b/scripts/check-return.pl @@ -0,0 +1,23 @@ +#!/bin/perl +# +# Usage: +# egrep -r -A8 -n 'xmlSec.*Error[0-9]?\(' ./src/ | sed 's/ //g' | perl ./scripts/check-return.pl +# + +my $has_return = 0; +my $where = ""; +foreach my $line ( ) { + chomp( $line ); + if($line eq "--" || $line eq '}' || $line eq 'continue' || $line eq 'break') { + if(not $has_return) { + print("FOUND MISSING RETURN: $where\n"); + } + $has_return = 0; + $where = ""; + } elsif($line =~ /.*Error.*/ && $where eq "") { + # print("Found error: $line\n"); + $where = $line + } elsif($line =~ /.*goto.*/ || $line =~ /.*return.*/ || $line =~ /.*ignoreerror.*/) { + $has_return = 1; + } +} \ No newline at end of file diff --git a/src/dl.c b/src/dl.c index 34677379e..e1189b7ca 100644 --- a/src/dl.c +++ b/src/dl.c @@ -223,6 +223,7 @@ xmlSecCryptoDLLibraryDestroy(xmlSecCryptoDLLibraryPtr lib) { ret = lt_dlclose(lib->handle); if(ret != 0) { xmlSecIOError("lt_dlclose", NULL, NULL); + /* ignore error */ } } #endif /* XMLSEC_DL_LIBLTDL */ @@ -234,6 +235,7 @@ xmlSecCryptoDLLibraryDestroy(xmlSecCryptoDLLibraryPtr lib) { res = FreeLibrary(lib->handle); if(!res) { xmlSecIOError("FreeLibrary", NULL, NULL); + /* ignore error */ } } #endif /* defined(XMLSEC_WINDOWS) && defined(XMLSEC_DL_WIN32)*/ @@ -395,6 +397,7 @@ xmlSecCryptoDLShutdown(void) { ret = lt_dlexit (); if(ret != 0) { xmlSecIOError("lt_dlexit", NULL, NULL); + /* ignore error */ } #else /* XMLSEC_DL_LIBLTDL */ UNREFERENCED_PARAMETER(ret); diff --git a/src/gcrypt/signatures.c b/src/gcrypt/signatures.c index 8fd3a53d0..1caadd05f 100644 --- a/src/gcrypt/signatures.c +++ b/src/gcrypt/signatures.c @@ -390,16 +390,16 @@ xmlSecGCryptPkSignatureVerify(xmlSecTransformPtr transform, } /* check result */ - if(ret == 1) { - transform->status = xmlSecTransformStatusOk; - } else { + if(ret != 1) { xmlSecOtherError(XMLSEC_ERRORS_R_DATA_NOT_MATCH, xmlSecTransformGetName(transform), - "ctx->verify: signature does not verify"); + "ctx->verify: signature verification failed"); transform->status = xmlSecTransformStatusFail; + return(0); } - /* done */ + /* success */ + transform->status = xmlSecTransformStatusOk; return(0); } diff --git a/src/gnutls/x509vfy.c b/src/gnutls/x509vfy.c index b0cbb6b81..b557b0960 100644 --- a/src/gnutls/x509vfy.c +++ b/src/gnutls/x509vfy.c @@ -406,12 +406,12 @@ xmlSecGnuTLSX509StoreVerify(xmlSecKeyDataStorePtr store, } if(err != GNUTLS_E_SUCCESS) { xmlSecGnuTLSError("gnutls_x509_crt_list_verify", err, NULL); - /* don't stop, continue! */ + /* ignore error, don't stop, continue! */ continue; } else if(verify != 0) { xmlSecOtherError2(XMLSEC_ERRORS_R_CERT_VERIFY_FAILED, NULL, "gnutls_x509_crt_list_verify: verification failed: status=%u", verify); - /* don't stop, continue! */ + /* ignore error, don't stop, continue! */ continue; } @@ -420,7 +420,7 @@ xmlSecGnuTLSX509StoreVerify(xmlSecKeyDataStorePtr store, ret = xmlSecGnuTLSX509CheckTime(cert_list, cert_list_cur_size, verification_time); if(ret != 1) { xmlSecInternalError("xmlSecGnuTLSX509CheckTime", NULL); - /* don't stop, continue! */ + /* ignore error, don't stop, continue! */ continue; } diff --git a/src/keyinfo.c b/src/keyinfo.c index 18c48cd83..dd17673f2 100644 --- a/src/keyinfo.c +++ b/src/keyinfo.c @@ -1053,9 +1053,10 @@ xmlSecKeyDataRetrievalMethodXmlRead(xmlSecKeyDataId id, xmlSecKeyPtr key, xmlNod if((keyInfoCtx->flags & XMLSEC_KEYINFO_FLAGS_RETRMETHOD_STOP_ON_UNKNOWN_HREF) != 0) { xmlSecInvalidNodeAttributeError(node, xmlSecAttrType, xmlSecKeyDataKlassGetName(id), "retrieval type is unknown"); - } else { - res = 0; + goto done; } + + res = 0; goto done; } diff --git a/src/mscng/certkeys.c b/src/mscng/certkeys.c index c0053d355..14659607e 100644 --- a/src/mscng/certkeys.c +++ b/src/mscng/certkeys.c @@ -304,6 +304,7 @@ xmlSecMSCngKeyDataFinalize(xmlSecKeyDataPtr data) { status = NCryptFreeObject(ctx->privkey); if(status != STATUS_SUCCESS) { xmlSecMSCngNtError("BCryptDestroyKey", NULL, status); + /* ignore error */ } } @@ -311,6 +312,7 @@ xmlSecMSCngKeyDataFinalize(xmlSecKeyDataPtr data) { status = BCryptDestroyKey(ctx->pubkey); if(status != STATUS_SUCCESS) { xmlSecMSCngNtError("BCryptDestroyKey", NULL, status); + /* ignore error */ } } diff --git a/src/mscng/x509.c b/src/mscng/x509.c index 3658c3841..8d2db3730 100644 --- a/src/mscng/x509.c +++ b/src/mscng/x509.c @@ -135,12 +135,14 @@ xmlSecMSCngKeyDataX509Finalize(xmlSecKeyDataPtr data) { if(ctx->cert != NULL) { if(!CertFreeCertificateContext(ctx->cert)) { xmlSecMSCngLastError("CertFreeCertificateContext", NULL); + /* ignore error */ } } if(ctx->hMemStore != 0) { if(!CertCloseStore(ctx->hMemStore, 0)) { xmlSecMSCngLastError("CertCloseStore", NULL); + /* ignore error */ } } @@ -821,6 +823,7 @@ xmlSecMSCngKeyDataX509DebugDump(xmlSecKeyDataPtr data, FILE* output) { xmlSecAssert(output != NULL); xmlSecNotImplementedError(NULL); + /* ignore error */ } static void @@ -829,6 +832,7 @@ xmlSecMSCngKeyDataX509DebugXmlDump(xmlSecKeyDataPtr data, FILE* output) { xmlSecAssert(output != NULL); xmlSecNotImplementedError(NULL); + /* ignore error */ } static xmlSecKeyDataKlass xmlSecMSCngKeyDataX509Klass = { diff --git a/src/mscng/x509vfy.c b/src/mscng/x509vfy.c index 95e7807b4..87da7110e 100644 --- a/src/mscng/x509vfy.c +++ b/src/mscng/x509vfy.c @@ -65,6 +65,7 @@ xmlSecMSCngX509StoreFinalize(xmlSecKeyDataStorePtr store) { ret = CertCloseStore(ctx->trusted, CERT_CLOSE_STORE_CHECK_FLAG); if(ret == FALSE) { xmlSecMSCngLastError("CertCloseStore", xmlSecKeyDataStoreGetName(store)); + /* ignore error */ } } @@ -72,6 +73,7 @@ xmlSecMSCngX509StoreFinalize(xmlSecKeyDataStorePtr store) { ret = CertCloseStore(ctx->trustedMemStore, CERT_CLOSE_STORE_CHECK_FLAG); if(ret == FALSE) { xmlSecMSCngLastError("CertCloseStore", xmlSecKeyDataStoreGetName(store)); + /* ignore error */ } } @@ -79,6 +81,7 @@ xmlSecMSCngX509StoreFinalize(xmlSecKeyDataStorePtr store) { ret = CertCloseStore(ctx->untrusted, CERT_CLOSE_STORE_CHECK_FLAG); if(ret == FALSE) { xmlSecMSCngLastError("CertCloseStore", xmlSecKeyDataStoreGetName(store)); + /* ignore error */ } } @@ -86,7 +89,8 @@ xmlSecMSCngX509StoreFinalize(xmlSecKeyDataStorePtr store) { ret = CertCloseStore(ctx->untrustedMemStore, CERT_CLOSE_STORE_CHECK_FLAG); if(ret == FALSE) { xmlSecMSCngLastError("CertCloseStore", xmlSecKeyDataStoreGetName(store)); - } + /* ignore error */ + } } memset(ctx, 0, sizeof(xmlSecMSCngX509StoreCtx)); diff --git a/src/mscrypto/signatures.c b/src/mscrypto/signatures.c index cbcc286fa..64ad383ee 100644 --- a/src/mscrypto/signatures.c +++ b/src/mscrypto/signatures.c @@ -408,7 +408,7 @@ static int xmlSecMSCryptoSignatureVerify(xmlSecTransformPtr transform, dwError = GetLastError(); if (NTE_BAD_SIGNATURE == HRESULT_FROM_WIN32(dwError)) { xmlSecOtherError(XMLSEC_ERRORS_R_DATA_NOT_MATCH, xmlSecTransformGetName(transform), - "CryptVerifySignature: signature does not verify"); + "CryptVerifySignature: signature verification failed"); transform->status = xmlSecTransformStatusFail; goto done; } else { diff --git a/src/nss/signatures.c b/src/nss/signatures.c index a13c77d5d..9c39d372c 100644 --- a/src/nss/signatures.c +++ b/src/nss/signatures.c @@ -459,7 +459,7 @@ xmlSecNssSignatureVerify(xmlSecTransformPtr transform, if (PORT_GetError() == SEC_ERROR_PKCS7_BAD_SIGNATURE) { xmlSecOtherError(XMLSEC_ERRORS_R_DATA_NOT_MATCH, xmlSecTransformGetName(transform), - "VFY_EndWithSignature: signature does not verify"); + "VFY_EndWithSignature: signature verification failed"); transform->status = xmlSecTransformStatusFail; } else { xmlSecNssError("VFY_EndWithSignature", diff --git a/src/openssl/evp_signatures.c b/src/openssl/evp_signatures.c index e1ce02b68..7e1605e2b 100644 --- a/src/openssl/evp_signatures.c +++ b/src/openssl/evp_signatures.c @@ -452,7 +452,7 @@ xmlSecOpenSSLEvpSignatureVerify(xmlSecTransformPtr transform, } else if(ret != 1) { xmlSecOtherError(XMLSEC_ERRORS_R_DATA_NOT_MATCH, xmlSecTransformGetName(transform), - "EVP_VerifyFinal: signature does not verify"); + "EVP_VerifyFinal: signature verification failed"); transform->status = xmlSecTransformStatusFail; return(0); } diff --git a/src/openssl/kt_rsa.c b/src/openssl/kt_rsa.c index 894007eba..83ce6747e 100644 --- a/src/openssl/kt_rsa.c +++ b/src/openssl/kt_rsa.c @@ -821,6 +821,7 @@ xmlSecOpenSSLRsaOaepSetKeyImpl(xmlSecOpenSSLRsaOaepCtxPtr ctx, EVP_PKEY* pKey, ctx->pKeyCtx = EVP_PKEY_CTX_new_from_pkey(xmlSecOpenSSLGetLibCtx(), pKey, NULL); if (ctx->pKeyCtx == NULL) { xmlSecOpenSSLError("EVP_PKEY_CTX_new_from_pkey", NULL); + return (-1); } if (encrypt != 0) { diff --git a/src/openssl/signatures.c b/src/openssl/signatures.c index b1131d90b..48d22b0c5 100644 --- a/src/openssl/signatures.c +++ b/src/openssl/signatures.c @@ -434,7 +434,7 @@ xmlSecOpenSSLSignatureVerify(xmlSecTransformPtr transform, } else { xmlSecOtherError(XMLSEC_ERRORS_R_DATA_NOT_MATCH, xmlSecTransformGetName(transform), - "ctx->verifyCallback: signature does not verify"); + "ctx->verifyCallback: signature verification failed"); transform->status = xmlSecTransformStatusFail; } diff --git a/src/openssl/x509vfy.c b/src/openssl/x509vfy.c index e168517e1..5f6e4dd1c 100644 --- a/src/openssl/x509vfy.c +++ b/src/openssl/x509vfy.c @@ -250,7 +250,6 @@ xmlSecOpenSSLX509StoreVerify(xmlSecKeyDataStorePtr store, XMLSEC_STACK_OF_X509* xmlSecAssert2(ctx != NULL, NULL); xmlSecAssert2(ctx->xst != NULL, NULL); - /* dup certs */ certs2 = sk_X509_dup(certs); if(certs2 == NULL) { xmlSecOpenSSLError("sk_X509_dup", @@ -391,6 +390,7 @@ xmlSecOpenSSLX509StoreVerify(xmlSecKeyDataStorePtr store, XMLSEC_STACK_OF_X509* xmlSecKeyDataStoreGetName(store), "X509_verify_cert: subject=%s; issuer=%s; err=%d; msg=%s", subject, issuer, err, xmlSecErrorsSafeString(err_msg)); + /* ignore error */ } } } @@ -410,27 +410,30 @@ xmlSecOpenSSLX509StoreVerify(xmlSecKeyDataStorePtr store, XMLSEC_STACK_OF_X509* xmlSecKeyDataStoreGetName(store), "subject=%s; issuer=%s; err=%d; msg=%s", subject, issuer, err, xmlSecErrorsSafeString(err_msg)); - break; + goto done; + case X509_V_ERR_CERT_NOT_YET_VALID: case X509_V_ERR_ERROR_IN_CERT_NOT_BEFORE_FIELD: xmlSecOtherError5(XMLSEC_ERRORS_R_CERT_NOT_YET_VALID, xmlSecKeyDataStoreGetName(store), "subject=%s; issuer=%s; err=%d; msg=%s", subject, issuer, err, xmlSecErrorsSafeString(err_msg)); - break; + goto done; + case X509_V_ERR_CERT_HAS_EXPIRED: case X509_V_ERR_ERROR_IN_CERT_NOT_AFTER_FIELD: xmlSecOtherError5(XMLSEC_ERRORS_R_CERT_HAS_EXPIRED, xmlSecKeyDataStoreGetName(store), "subject=%s; issuer=%s; err=%d; msg=%s", subject, issuer, err, xmlSecErrorsSafeString(err_msg)); - break; + goto done; + default: xmlSecOtherError5(XMLSEC_ERRORS_R_CERT_VERIFY_FAILED, xmlSecKeyDataStoreGetName(store), "subject=%s; issuer=%s; err=%d; msg=%s", subject, issuer, err, xmlSecErrorsSafeString(err_msg)); - break; + goto done; } } diff --git a/src/xmldsig.c b/src/xmldsig.c index e7a8b44c8..668ea2382 100644 --- a/src/xmldsig.c +++ b/src/xmldsig.c @@ -655,6 +655,7 @@ xmlSecDSigCtxProcessSignedInfoNode(xmlSecDSigCtxPtr dsigCtx, xmlNodePtr node, xm if(dsigCtx->preSignMemBufMethod == NULL) { xmlSecInternalError("xmlSecTransformCtxCreateAndAppend", xmlSecTransformKlassGetName(xmlSecTransformMemBufId)); + return(-1); } } diff --git a/src/xmlsec.c b/src/xmlsec.c index c36dc7f2a..6da57caa9 100644 --- a/src/xmlsec.c +++ b/src/xmlsec.c @@ -130,7 +130,7 @@ xmlSecInit(void) { */ int xmlSecShutdown(void) { - int res = 0; + int res = -1; xmlSecTransformIdsShutdown(); xmlSecKeyDataIdsShutdown(); @@ -138,10 +138,17 @@ xmlSecShutdown(void) { #ifndef XMLSEC_NO_CRYPTO_DYNAMIC_LOADING if(xmlSecCryptoDLShutdown() < 0) { xmlSecInternalError("xmlSecCryptoDLShutdown", NULL); - res = -1; + goto done; } #endif /* XMLSEC_NO_CRYPTO_DYNAMIC_LOADING */ + /* success */ + res = 0; + +#ifndef XMLSEC_NO_CRYPTO_DYNAMIC_LOADING +done: +#endif /* XMLSEC_NO_CRYPTO_DYNAMIC_LOADING */ + xmlSecIOShutdown(); xmlSecErrorsShutdown(); return(res); diff --git a/src/xmltree.c b/src/xmltree.c index 13e0a6d97..b2ef6e734 100644 --- a/src/xmltree.c +++ b/src/xmltree.c @@ -768,6 +768,7 @@ xmlSecAddIDs(xmlDocPtr doc, xmlNodePtr cur, const xmlChar** ids) { xmlAddID(NULL, doc, name, attr); } else if(tmp != attr) { xmlSecInvalidStringDataError("id", name, "unique id (id already defined)", NULL); + /* ignore error */ } xmlFree(name); }