fix missing returns and full audit for all errors (issue lsh123#449) (l…

…sh123#450)
ycharts · Aug 19, 2024 · 570dc5c · 570dc5c
1 parent 76e1e61
commit 570dc5c
Show file tree

Hide file tree

Showing 17 changed files with 73 additions and 22 deletions.
diff --git a/scripts/check-return.pl 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 ( <STDIN> ) {
+    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;
+    }
+}
diff --git 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
@@ -388,16 +388,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
@@ -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
@@ -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
@@ -304,13 +304,15 @@ xmlSecMSCngKeyDataFinalize(xmlSecKeyDataPtr data) {
         status = NCryptFreeObject(ctx->privkey);
         if(status != STATUS_SUCCESS) {
             xmlSecMSCngNtError("BCryptDestroyKey", NULL, status);
+            /* ignore error */
         }
     }
 
     if(ctx->pubkey != 0) {
         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
@@ -134,12 +134,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 */
         }
     }
 
@@ -820,6 +822,7 @@ xmlSecMSCngKeyDataX509DebugDump(xmlSecKeyDataPtr data, FILE* output) {
     xmlSecAssert(output != NULL);
 
     xmlSecNotImplementedError(NULL);
+    /* ignore error */
 }
 
 static void
@@ -828,6 +831,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
@@ -65,28 +65,32 @@ xmlSecMSCngX509StoreFinalize(xmlSecKeyDataStorePtr store) {
         ret = CertCloseStore(ctx->trusted, CERT_CLOSE_STORE_CHECK_FLAG);
         if(ret == FALSE) {
             xmlSecMSCngLastError("CertCloseStore", xmlSecKeyDataStoreGetName(store));
+            /* ignore error */
         }
     }
 
     if(ctx->trustedMemStore != NULL) {
         ret = CertCloseStore(ctx->trustedMemStore, CERT_CLOSE_STORE_CHECK_FLAG);
         if(ret == FALSE) {
             xmlSecMSCngLastError("CertCloseStore", xmlSecKeyDataStoreGetName(store));
+            /* ignore error */
         }
     }
 
     if(ctx->untrusted != NULL) {
         ret = CertCloseStore(ctx->untrusted, CERT_CLOSE_STORE_CHECK_FLAG);
         if(ret == FALSE) {
             xmlSecMSCngLastError("CertCloseStore", xmlSecKeyDataStoreGetName(store));
+            /* ignore error */
         }
     }
 
     if(ctx->untrustedMemStore != NULL) {
         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
@@ -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
@@ -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
@@ -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
@@ -526,6 +526,7 @@ xmlSecOpenSSLRsaPkcs1Process(xmlSecTransformPtr transform) {
     if(ret < 0) {
         xmlSecInternalError("xmlSecOpenSSLRsaPkcs1ProcessImpl",
             xmlSecTransformGetName(transform));
+        return(-1);
     }
 
     ret = xmlSecBufferSetSize(out, outSize);
@@ -811,6 +812,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
@@ -501,7 +501,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
@@ -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
@@ -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
@@ -130,18 +130,25 @@ xmlSecInit(void) {
  */
 int
 xmlSecShutdown(void) {
-    int res = 0;
+    int res = -1;
 
     xmlSecTransformIdsShutdown();
     xmlSecKeyDataIdsShutdown();
 
 #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
@@ -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);
                     }