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 <k.tak@samsung.com>

diff --git a/vcore/vcore/Client.cpp b/vcore/vcore/Client.cpp
index 6767ca8..d0f48ae 100644 (file)
--- 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 (file)
--- 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<char, void(*)(void *)> certbufptr(certbuf, free);
 
-               pBio = BIO_new(BIO_s_mem());
+               LogInfo("certbuf: " << certbuf);
 
-               if (pBio == NULL) {
+               std::unique_ptr<BIO, int(*)(BIO *)> 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<X509, void(*)(X509 *)> 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 (file)
--- 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;
 }