From 15930fdbbfebd2415ae1f9fc829f916d09b80f73 Mon Sep 17 00:00:00 2001 From: Kyungwook Tak Date: Tue, 12 Jul 2016 20:41:22 +0900 Subject: Fix bugs in getting certs and pass check Password check on pkcs12 makes undefined behavior. peaking last error is suspicious so ERR_get_error used and works well. Parsing certificate of PEM format with TRUSTED CERTIFICATE header didn't work. For trusted certificate case, use PEM_read_bio_X509_AUX first because it works well on both of TRUSETD CERTIFICATE and CERTIFICATE. Try 4 formats step by step. PEM(AUX), PEM, BASE64, DER. Change-Id: I6d81393bc31b2e740365ae3b0b4962fd9a6e55dc Signed-off-by: Kyungwook Tak --- vcore/vcore/Client.cpp | 7 +++- vcore/vcore/api.cpp | 109 ++++++++++++++++++++++++------------------------- vcore/vcore/pkcs12.cpp | 22 ++++++++-- 3 files changed, 77 insertions(+), 61 deletions(-) diff --git a/vcore/vcore/Client.cpp b/vcore/vcore/Client.cpp index 6767ca8..d0f48ae 100644 --- a/vcore/vcore/Client.cpp +++ b/vcore/vcore/Client.cpp @@ -489,16 +489,19 @@ int vcore_client_get_certificate_from_store(CertStoreType storeType, const char if (recvData.dataBlockLen > 0 && recvData.dataBlockLen <= VCORE_MAX_RECV_DATA_SIZE) { outData = (char *)malloc(recvData.dataBlockLen + 1); + if (outData == nullptr) + return CERTSVC_BAD_ALLOC; + memset(outData, 0x00, recvData.dataBlockLen + 1); memcpy(outData, recvData.dataBlock, recvData.dataBlockLen); *certData = outData; *certSize = recvData.dataBlockLen; + + return recvData.result; } else { LogError("revcData length is wrong : " << recvData.dataBlockLen); return CERTSVC_WRONG_ARGUMENT; } - - return recvData.result; } int vcore_client_delete_certificate_from_store(CertStoreType storeType, const char *gname) diff --git a/vcore/vcore/api.cpp b/vcore/vcore/api.cpp index 6d125bf..4554e77 100644 --- a/vcore/vcore/api.cpp +++ b/vcore/vcore/api.cpp @@ -1559,86 +1559,85 @@ int certsvc_get_certificate(CertSvcInstance instance, const char *gname, CertSvcCertificate *certificate) { - int result = CERTSVC_SUCCESS; - char *certBuffer = NULL; - std::string fileName; - size_t length = 0; - FILE *fp_write = NULL; - BIO *pBio = NULL; - X509 *x509Struct = NULL; - try { - result = vcore_client_get_certificate_from_store(storeType, gname, &certBuffer, &length, PEM_CRT); + size_t len = 0; + char *certbuf = nullptr; + int result = vcore_client_get_certificate_from_store(storeType, gname, &certbuf, + &len, PEM_CRT); if (result != CERTSVC_SUCCESS) { LogError("Failed to get certificate buffer from store."); return result; } + std::unique_ptr certbufptr(certbuf, free); - pBio = BIO_new(BIO_s_mem()); + LogInfo("certbuf: " << certbuf); - if (pBio == NULL) { + std::unique_ptr bioptr(BIO_new(BIO_s_mem()), BIO_free); + if (bioptr == nullptr) { LogError("Failed to allocate memory."); - result = CERTSVC_BAD_ALLOC; + return CERTSVC_BAD_ALLOC; } - length = BIO_write(pBio, (const void *)certBuffer, (int)length); + len = BIO_write(bioptr.get(), (const void *)certbuf, (int)len); - if ((int)length < 1) { + if ((int)len < 1) { LogError("Failed to load cert into bio."); - result = CERTSVC_BAD_ALLOC; + return CERTSVC_BAD_ALLOC; } - x509Struct = PEM_read_bio_X509(pBio, NULL, 0, NULL); - - if (x509Struct != NULL) { - CertificatePtr cert(new Certificate(x509Struct)); - certificate->privateInstance = instance; - certificate->privateHandler = impl(instance)->addCert(cert); - - if (certBuffer != NULL) free(certBuffer); - } else { - fileName.append(CERTSVC_PKCS12_STORAGE_DIR); - fileName.append(gname); - - if (!(fp_write = fopen(fileName.c_str(), "w"))) { - LogError("Failed to open the file for writing, [" << fileName << "]."); - result = CERTSVC_FAIL; - goto error; - } + std::unique_ptr x509ptr( + PEM_read_bio_X509_AUX(bioptr.get(), nullptr, nullptr, nullptr), X509_free); - if (fwrite(certBuffer, sizeof(char), length, fp_write) != length) { - LogError("Fail to write certificate."); - result = CERTSVC_FAIL; - goto error; - } + CertificatePtr cert; - result = certsvc_certificate_new_from_file(instance, fileName.c_str(), certificate); + if (x509ptr != nullptr) { + LogInfo("PEM_read_bio_X509_AUX returned x509 struct!"); + try { + cert.reset(new Certificate(x509ptr.get())); + LogInfo("Parse cert success with PEM(AUX) form!"); + } catch (...) {} + } - if (result != CERTSVC_SUCCESS) { - LogError("Failed to construct certificate from buffer."); - goto error; + if (cert == nullptr) { + x509ptr.reset(PEM_read_bio_X509(bioptr.get(), nullptr, nullptr, nullptr)); + if (x509ptr != nullptr) { + LogInfo("PEM_read_bio_X509 returned x509 struct!"); + try { + cert.reset(new Certificate(x509ptr.get())); + LogInfo("Parse cert success with PEM form!"); + } catch (...) {} } - - unlink(fileName.c_str()); } - result = CERTSVC_SUCCESS; - } catch (std::bad_alloc &) { - result = CERTSVC_BAD_ALLOC; - } catch (...) {} + if (cert == nullptr) { + try { + cert.reset(new Certificate(certbuf, Certificate::FormType::FORM_BASE64)); + LogInfo("Parse cert success with Base64 form!"); + } catch (...) {} + } -error: - if (x509Struct) - X509_free(x509Struct); + if (cert == nullptr) { + try { + cert.reset(new Certificate(certbuf, Certificate::FormType::FORM_DER)); + LogInfo("Parse cert success with DER form!"); + } catch (...) {} + } - if (pBio) - BIO_free(pBio); + if (cert == nullptr) { + LogError("Failed to parse cert on all of PEM/DER/Base64 form!"); + return CERTSVC_INVALID_CERTIFICATE; + } - if (fp_write) - fclose(fp_write); + certificate->privateInstance = instance; + certificate->privateHandler = impl(instance)->addCert(cert); - return result; + return CERTSVC_SUCCESS; + } catch (std::bad_alloc &) { + return CERTSVC_BAD_ALLOC; + } catch (...) { + return CERTSVC_FAIL; + } } int certsvc_pkcs12_check_alias_exists_in_store(CertSvcInstance instance, diff --git a/vcore/vcore/pkcs12.cpp b/vcore/vcore/pkcs12.cpp index 0d818cc..5985005 100644 --- a/vcore/vcore/pkcs12.cpp +++ b/vcore/vcore/pkcs12.cpp @@ -560,7 +560,10 @@ int extractPkcs12(const std::string &path, PKCS12_free(container); if (result != 1) { - LogError("Failed to parse the file passed. openssl err : " << ERR_get_error()); + unsigned long e = ERR_get_error(); + char buf[1024]; + ERR_error_string_n(e, buf, 1023); + LogError("Failed to parse the file passed. openssl err: " << buf); return CERTSVC_FAIL; } @@ -837,9 +840,20 @@ int pkcs12_has_password(const char *filepath, int *passworded) if (cert != NULL) X509_free(cert); - if (result != 1 && ERR_GET_REASON(ERR_peek_last_error()) != PKCS12_R_MAC_VERIFY_FAILURE) - return CERTSVC_FAIL; + if (result != 1) { + unsigned long e = ERR_get_error(); + if (ERR_GET_REASON(e) == PKCS12_R_MAC_VERIFY_FAILURE) { + LogInfo("verify failed without password. file(" << filepath << ") is password-protected."); + *passworded = CERTSVC_TRUE; + } else { + char buf[1024]; + ERR_error_string_n(e, buf, 1023); + LogError("Error on PKCS12_pasre file(" << filepath << "): " << buf); + return CERTSVC_FAIL; + } + } else { + *passworded = CERTSVC_FALSE; + } - *passworded = (result == 1) ? 1 : 0; return CERTSVC_SUCCESS; } -- cgit v1.2.3