# HG changeset patch # User Andre Heinecke # Date 1410429924 -7200 # Node ID edbf5e5e88f4cb2c5b18e54709a46cd9ba47efc4 # Parent 898b1ddcca11529d813b7923346c4d56609642d9 (issue118) Extend verify_binary to carry an open file * binverify.c: Change result to a structure containing an open fptr Use in Memory data for windows verification. * mainwindow.cpp, selftest.c: Handle the returend structure * binverifytest.cpp: Test for the exclusive read and update signature. * listutil.c: Add optional fptr parameter to read_file diff -r 898b1ddcca11 -r edbf5e5e88f4 common/binverify.c --- a/common/binverify.c Thu Sep 11 12:00:10 2014 +0200 +++ b/common/binverify.c Thu Sep 11 12:05:24 2014 +0200 @@ -10,6 +10,7 @@ #include "strhelp.h" #include "logging.h" +#include "listutil.h" #ifdef RELEASE_BUILD #include "pubkey-release.h" #else @@ -19,8 +20,13 @@ bin_verify_result verify_binary(const char *filename, size_t name_len) { - if (!filename || !name_len) - return VerifyUnknownError; + if (!filename || !name_len) { + bin_verify_result retval; + retval.fptr = NULL; + retval.result = VerifyUnknownError; + return retval; + } + #ifdef WIN32 return verify_binary_win(filename, name_len); #else @@ -101,7 +107,7 @@ bin_verify_result verify_binary_win(const char *filename, size_t name_len) { - bin_verify_result retval = VerifyUnknownError; + bin_verify_result retval; WCHAR *filenameW = NULL; BOOL result = FALSE; DWORD dwEncoding = 0, @@ -112,17 +118,34 @@ HCRYPTMSG hMsg = NULL; PCERT_INFO pSignerCert = NULL; PCCERT_CONTEXT pSignerCertContext = NULL; + FILE *fptr = NULL; + size_t data_size = 0; + char *data = NULL; + int ret = -1; + CRYPT_INTEGER_BLOB blob; + + retval.result = VerifyUnknownError; + retval.fptr = NULL; if (!filename || name_len > MAX_PATH || strlen(filename) != name_len) { ERRORPRINTF ("Invalid parameters\n"); - return VerifyUnknownError; + return retval; } - filenameW = utf8_to_wchar(filename, name_len); + ret = read_file(filename, &data, &data_size, MAX_VALID_BIN_SIZE, &fptr); - result = CryptQueryObject (CERT_QUERY_OBJECT_FILE, - filenameW, + if (ret != 0) + { + ERRORPRINTF ("Read file failed with error: %i\n", ret); + retval.result = VerifyReadFailed; + return retval; + } + blob.cbData = (DWORD) data_size; + blob.pbData = (PBYTE) data; + + result = CryptQueryObject (CERT_QUERY_OBJECT_BLOB, + &blob, CERT_QUERY_CONTENT_FLAG_PKCS7_SIGNED_EMBED, CERT_QUERY_FORMAT_FLAG_BINARY, 0, @@ -136,7 +159,7 @@ if (!result || !hMsg) { PRINTLASTERROR ("Failed to query crypto object"); - retval = VerifyReadFailed; + retval.result = VerifyReadFailed; goto done; } @@ -152,7 +175,7 @@ else { ERRORPRINTF ("Failed to get signer cert size."); - retval = VerifyUnknownError; + retval.result = VerifyUnknownError; goto done; } @@ -163,7 +186,7 @@ &dwSignerInfoSize))) { ERRORPRINTF ("Failed to get signer cert."); - retval = VerifyUnknownError; + retval.result = VerifyUnknownError; goto done; } @@ -175,7 +198,7 @@ if (!pSignerCertContext) { ERRORPRINTF ("Failed to find signer cert in store."); - retval = VerifyUnknownError; + retval.result = VerifyUnknownError; goto done; } @@ -186,7 +209,7 @@ pSignerCertContext->pCertInfo)) { ERRORPRINTF ("The signature is invalid. \n"); - retval = VerifyInvalidSignature; + retval.result = VerifyInvalidSignature; syslog_error_printf ("Software update embedded signature is invalid."); goto done; } @@ -194,22 +217,29 @@ if(check_certificate(pSignerCertContext)) { DEBUGPRINTF ("Valid signature with pinned certificate."); - retval = VerifyValid; + retval.result = VerifyValid; + retval.fptr = fptr; goto done; } else { ERRORPRINTF ("Certificate mismatch. \n"); - retval = VerifyInvalidCertificate; + retval.result = VerifyInvalidCertificate; syslog_error_printf ("Software update embedded signature " "created with wrong certificate."); goto done; } done: + xfree(data); xfree(filenameW); xfree(pSignerCert); + if (retval.result != VerifyValid) + { + fclose(fptr); + } + if(pSignerCertContext) { CertFreeCertificateContext(pSignerCertContext); @@ -226,8 +256,6 @@ } #else /* WIN32 */ -#include "listutil.h" - #pragma GCC diagnostic ignored "-Wconversion" /* Polarssl mh.h contains a conversion which gcc warns about */ #include @@ -248,29 +276,34 @@ sig_size = TRUSTBRIDGE_RSA_KEY_SIZE / 8; unsigned char signature[sig_size], hash[32]; + FILE *fptr = NULL; - bin_verify_result retval = VerifyUnknownError; + bin_verify_result retval; + retval.result = VerifyUnknownError; + retval.fptr = NULL; x509_crt codesign_cert; if (strnlen(filename, name_len + 1) != name_len || name_len == 0) { ERRORPRINTF ("Invalid call to verify_binary_linux\n"); - return VerifyUnknownError; + retval.result = VerifyUnknownError; + return retval; } - ret = read_file(filename, &data, &data_size, MAX_VALID_BIN_SIZE); + ret = read_file(filename, &data, &data_size, MAX_VALID_BIN_SIZE, &fptr); if (ret != 0) { ERRORPRINTF ("Read file failed with error: %i\n", ret); - return VerifyReadFailed; + retval.result = VerifyReadFailed; + return retval; } /* Fetch the signature from the end of data */ if (data_size < sig_b64_size + 5) { ERRORPRINTF ("File to small to contain a signature.\n"); - retval = VerifyInvalidSignature; + retval.result = VerifyInvalidSignature; goto done; } @@ -280,7 +313,7 @@ data[data_size - sig_b64_size - 5] != '\r') { ERRORPRINTF ("Failed to find valid signature line.\n"); - retval = VerifyInvalidSignature; + retval.result = VerifyInvalidSignature; goto done; } @@ -312,7 +345,8 @@ errbuf[1019] = '\0'; /* Just to be sure */ ERRORPRINTF ("x509_crt_parse failed with -0x%04x\n%s\n", -ret, errbuf); x509_crt_free(&codesign_cert); - return VerifyUnknownError; + retval.result = VerifyUnknownError; + goto done; } ret = pk_verify(&codesign_cert.pk, POLARSSL_MD_SHA256, hash, 0, @@ -325,14 +359,22 @@ errbuf[1019] = '\0'; /* Just to be sure */ ERRORPRINTF ("pk_verify failed with -0x%04x\n %s\n", -ret, errbuf); x509_crt_free(&codesign_cert); - retval = VerifyInvalidSignature; + retval.result = VerifyInvalidSignature; goto done; } x509_crt_free(&codesign_cert); - retval = VerifyValid; + retval.result = VerifyValid; + retval.fptr = fptr; done: + if (retval.result != VerifyValid) + { + if (fptr) + { + fclose(fptr); + } + } xfree (data); return retval; } diff -r 898b1ddcca11 -r edbf5e5e88f4 common/binverify.h --- a/common/binverify.h Thu Sep 11 12:00:10 2014 +0200 +++ b/common/binverify.h Thu Sep 11 12:05:24 2014 +0200 @@ -13,13 +13,14 @@ */ #include #include +#include #ifdef __cplusplus extern "C" { #endif /** - * @enum bin_verify_result + * @enum verify_result * @brief Result of a verification */ typedef enum { @@ -28,6 +29,19 @@ VerifyInvalidSignature = 4, /*! Signature was invalid */ VerifyInvalidCertificate = 5, /*! Certificate mismatch */ VerifyReadFailed = 6, /*! File exists but could not read the file */ +} verify_result; + +/** + * A structure containing a verify_result and a reference to the + * verified file. + */ +typedef struct { + /*@{*/ + verify_result result; /**< the result of the verification */ + FILE *fptr; /**< Pointer to the open file struct of the verified file + The ptr is only valid if verify_result is VerifyValid + and needs to be closed by the caller in that case.*/ + /*@}*/ } bin_verify_result; /** @@ -57,14 +71,15 @@ */ bin_verify_result verify_binary(const char *filename, size_t name_len); +/**@def Max size of a valid binary in byte */ +#define MAX_VALID_BIN_SIZE (32 * 1024 * 1024) + #ifdef WIN32 /** * @brief windows implementation of verify_binary */ bin_verify_result verify_binary_win(const char *filename, size_t name_len); #else /* WIN32 */ -/**@def Max size of a valid binary in byte */ -#define MAX_VALID_BIN_SIZE (32 * 1024 * 1024) /** * @brief linux implementation of verify_binary diff -r 898b1ddcca11 -r edbf5e5e88f4 common/listutil.c --- a/common/listutil.c Thu Sep 11 12:00:10 2014 +0200 +++ b/common/listutil.c Thu Sep 11 12:05:24 2014 +0200 @@ -16,6 +16,10 @@ #include #include +#ifdef WIN32 +#include +#endif + #include "strhelp.h" #include "logging.h" @@ -41,7 +45,7 @@ #define READ_FILE_INVALID_CALL -5 int read_file(const char *file_name, char **data, size_t *size, - const size_t max_size) + const size_t max_size, FILE **fptr) { FILE *f; long file_size; @@ -50,8 +54,23 @@ { return READ_FILE_INVALID_CALL; } - +#ifdef WIN32 + { + wchar_t *wFilename = utf8_to_wchar(file_name, strlen(file_name)); + if (!wFilename) + { + return READ_FILE_UNREADABLE; + } + /* We open and write protect the file here so that + as long as the file is open we can be sure that + it was not modified and can use it in subsequent + calls based on the filename. */ + f = _wfsopen(wFilename, L"rb", _SH_DENYWR); + xfree(wFilename); + } +#else f = fopen(file_name, "rb"); +#endif if (f == NULL) return READ_FILE_UNREADABLE; @@ -92,7 +111,14 @@ return READ_FILE_READ_FAILED; } - fclose(f); + if (fptr) + { + *fptr = f; + } + else + { + fclose(f); + } (*data)[*size] = '\0'; @@ -180,7 +206,7 @@ *size = 0; int ret = 0; - ret = read_file(file_name, data, size, MAX_FILESIZE); + ret = read_file(file_name, data, size, MAX_FILESIZE, NULL); /* printf ("Ret: %i \n", ret); */ if (ret != 0) diff -r 898b1ddcca11 -r edbf5e5e88f4 common/listutil.h --- a/common/listutil.h Thu Sep 11 12:00:10 2014 +0200 +++ b/common/listutil.h Thu Sep 11 12:05:24 2014 +0200 @@ -13,6 +13,7 @@ #endif #include +#include /** * @file listutil.h @@ -84,17 +85,22 @@ /** * @brief Read a file into memory. * - * The caller needs to free data + * The caller needs to free data. If fptr is not NULL it will + * recieve the pointer to the read file structure. The caller + * is responsible for closing this. + * fptr only needs to be closed and is only valid if the + * return value is 0. * * @param[in] file_name Name of the file. * @param[out] data the file content * @param[out] size size in bytes of the file content. * @param[in] max_size the maximum amount of bytes to read. + * @param[out] fptr pointer to recieve the FILE ptr or NULL * * @return 0 on success an error code otherwise. */ int read_file(const char *file_name, char **data, size_t *size, - const size_t max_size); + const size_t max_size, FILE **fptr); #ifdef __cplusplus } #endif diff -r 898b1ddcca11 -r edbf5e5e88f4 common/selftest.c --- a/common/selftest.c Thu Sep 11 12:00:10 2014 +0200 +++ b/common/selftest.c Thu Sep 11 12:05:24 2014 +0200 @@ -6,6 +6,7 @@ bool selftest() { + bin_verify_result res; #ifdef WIN32 wchar_t wPath[MAX_PATH]; char *utf8path = NULL; @@ -27,7 +28,8 @@ return false; } - if (verify_binary (utf8path, strlen(utf8path)) != VerifyValid) + res = verify_binary (utf8path, strlen(utf8path)); + if (res.result != VerifyValid) { ERRORPRINTF ("Verification of the binary failed"); syslog_error_printf ("Integrity check failed."); @@ -35,13 +37,17 @@ return false; } + fclose(res.fptr); xfree(utf8path); return true; #else - if (!verify_binary ("/proc/self/exe", 14) != VerifyValid) + res = verify_binary ("/proc/self/exe", 14); + if (res.result != VerifyValid) { syslog_error_printf ("Integrity check failed."); return false; } + fclose(res.fptr); + return true; #endif } diff -r 898b1ddcca11 -r edbf5e5e88f4 ui/mainwindow.cpp --- a/ui/mainwindow.cpp Thu Sep 11 12:00:10 2014 +0200 +++ b/ui/mainwindow.cpp Thu Sep 11 12:05:24 2014 +0200 @@ -234,14 +234,15 @@ } bin_verify_result verifyResult = verify_binary(swFileName.toUtf8().constData(), swFileName.toUtf8().size()); - qDebug() << "Binary verify result: " << verifyResult; - if (verifyResult != VerifyValid) { + qDebug() << "Binary verify result: " << verifyResult.result; + if (verifyResult.result != VerifyValid) { qDebug() << "Failed to verify downloaded data."; QFile::remove(swFileName); mSettings.remove("Software/available"); mSettings.remove("Software/availableDate"); return; } + fclose(verifyResult.fptr); } void MainWindow::handleNewList(const QString& fileName, const QDateTime& modDate) { @@ -289,9 +290,10 @@ void MainWindow::installNewSW(const QString& fileName, const QDateTime& modDate) { QFileInfo instProcInfo = QFileInfo(fileName); QString filePath = QDir::toNativeSeparators(instProcInfo.absoluteFilePath()); + bin_verify_result vres = verify_binary(filePath.toUtf8().constData(), + filePath.toUtf8().size()); - if (verify_binary(filePath.toUtf8().constData(), - filePath.toUtf8().size()) != VerifyValid) { + if (vres.result != VerifyValid) { qDebug() << "Invalid software. Not installing"; return; } @@ -328,6 +330,7 @@ free(errmsg); qDebug() << "Failed to start process: " << qerrmsg; setState(NewSoftwareAvailable); + fclose(vres.fptr); return; } #else /* WIN32 */ @@ -360,12 +363,15 @@ qDebug() << "Starting process " << filePath <<" params: " << parameters; if (!sudo_started && !QProcess::startDetached(filePath, parameters)) { qDebug() << "Failed to start process."; + fclose(vres.fptr); return; } #endif + + syslog_info_printf ("Installing update: %s\n", fileName.toUtf8().constData()); /* Installer process should now be running. We exit */ - + fclose(vres.fptr); closeApp(); } diff -r 898b1ddcca11 -r edbf5e5e88f4 ui/tests/binverifytest.cpp --- a/ui/tests/binverifytest.cpp Thu Sep 11 12:00:10 2014 +0200 +++ b/ui/tests/binverifytest.cpp Thu Sep 11 12:05:24 2014 +0200 @@ -30,17 +30,17 @@ /* Some general robustness checks */ void BinVerifyTest::testMiscErrors() { - QVERIFY (verify_binary (NULL, 10) != VerifyValid); - QVERIFY (verify_binary ("foo", 10) != VerifyValid); - QVERIFY (verify_binary ("bar", -1) != VerifyValid); + QVERIFY (verify_binary (NULL, 10).result != VerifyValid); + QVERIFY (verify_binary ("foo", 10).result != VerifyValid); + QVERIFY (verify_binary ("bar", -1).result!= VerifyValid); /* On windows the next line will check that a valid microsoft * signed executable is not valid for us (pinning). On linux * it will just fail with a read error which we tested above */ #ifdef Q_OS_WIN QVERIFY (verify_binary ("c:\\Windows\\System32\\mmc.exe", - strlen("c:\\Windows\\System32\\mmc.exe")) != VerifyInvalidCertificate); + strlen("c:\\Windows\\System32\\mmc.exe")).result != VerifyInvalidCertificate); #endif - QVERIFY (verify_binary ("/dev/null", strlen("/dev/null")) != VerifyValid); + QVERIFY (verify_binary ("/dev/null", strlen("/dev/null")).result != VerifyValid); } /* Check that a signature with only a different key (of the same size) @@ -48,14 +48,16 @@ void BinVerifyTest::testOtherKey() { QVERIFY(VerifyInvalidSignature == verify_binary ("fakeinst-other-key" EXE_SUFFIX, - strlen("fakeinst-other-key" EXE_SUFFIX))); + strlen("fakeinst-other-key" EXE_SUFFIX)).result); } /* Check that an invalid signature is not validated */ void BinVerifyTest::testInvalidSig() { - QVERIFY(VerifyValid != verify_binary ("fakeinst-invalid" EXE_SUFFIX, - strlen("fakeinst-invalid" EXE_SUFFIX))); + bin_verify_result res = verify_binary ("fakeinst-invalid" EXE_SUFFIX, + strlen("fakeinst-invalid" EXE_SUFFIX)); + QVERIFY(VerifyValid != res.result); + QVERIFY(res.fptr == NULL); } #ifdef Q_OS_WIN @@ -64,22 +66,34 @@ void BinVerifyTest::testOtherCert() { QVERIFY(VerifyInvalidCertificate == verify_binary ("fakeinst-other-cert" EXE_SUFFIX, - strlen("fakeinst-other-cert" EXE_SUFFIX))); + strlen("fakeinst-other-cert" EXE_SUFFIX)).result); } #endif /* Check that no signature is not validated */ void BinVerifyTest::testNoSignature() { - QVERIFY(VerifyValid != verify_binary ("fakeinst" EXE_SUFFIX, - strlen("fakeinst" EXE_SUFFIX))); + bin_verify_result res = verify_binary ("fakeinst" EXE_SUFFIX, + strlen("fakeinst" EXE_SUFFIX)); + QVERIFY(VerifyValid != res.result); + QVERIFY(res.fptr == NULL); } /* Check that a valid signed executable is verified */ void BinVerifyTest::testValidBinary() { - QVERIFY (VerifyValid == verify_binary ("fakeinst-signed" EXE_SUFFIX, - strlen("fakeinst-signed" EXE_SUFFIX))); + bin_verify_result res = verify_binary ("fakeinst-signed" EXE_SUFFIX, + strlen("fakeinst-signed" EXE_SUFFIX)); + QVERIFY (VerifyValid == res.result); + QFile thefile ("fakeinst-signed" EXE_SUFFIX); +#ifdef WIN32 + /* Verifies the deny write open mode. But on linuy we dont have it. */ + QVERIFY (!thefile.open(QIODevice::ReadWrite)); +#endif + QVERIFY (res.fptr != NULL); + fclose(res.fptr); + QVERIFY (thefile.open(QIODevice::ReadWrite)); + thefile.close(); } void BinVerifyTest::testSignatureCreation() @@ -95,8 +109,9 @@ bool ret = theDialog->appendTextSignatureToFile (garbage, outfile.fileName()); QVERIFY(QFile::remove(garbage)); QVERIFY(ret == true); - QVERIFY(VerifyValid == verify_binary (outfile.fileName().toUtf8().constData(), - outfile.fileName().toUtf8().size())); + bin_verify_result res = verify_binary (outfile.fileName().toUtf8().constData(), + outfile.fileName().toUtf8().size()); + QVERIFY(VerifyValid == res.result); } bool g_debug = true;