From 7b3b96cd9eadf23d94048ff025ef097a85263db9 Mon Sep 17 00:00:00 2001 From: Pete Batard Date: Tue, 13 Oct 2015 23:29:30 +0100 Subject: [PATCH] [pki] add signature check on update downloads - part 2 * Closes #158 --- res/localization/rufus.loc | 3 ++ src/localization_data.h | 7 ++++ src/pki.c | 66 ++++++++++++++++++++++++-------------- src/resource.h | 9 +++++- src/rufus.c | 5 --- src/rufus.h | 3 +- src/rufus.rc | 10 +++--- src/stdlg.c | 9 ++++-- 8 files changed, 73 insertions(+), 39 deletions(-) diff --git a/res/localization/rufus.loc b/res/localization/rufus.loc index 80a9bd9d..1c7fe359 100644 --- a/res/localization/rufus.loc +++ b/res/localization/rufus.loc @@ -539,6 +539,9 @@ t MSG_279 "Non bootable" t MSG_280 "Image selection" t MSG_281 "(Please select an image)" t MSG_282 "Drive locking" +t MSG_283 "Invalid signature" +t MSG_284 "The downloaded executable is missing a digital signature." +t MSG_285 "The downloaded executable was signed by '%s'.\nThis doesn't look right... Are you sure you want to run it?" ################################################################################ ############################# TRANSLATOR END COPY ############################## diff --git a/src/localization_data.h b/src/localization_data.h index c63d47d9..b277d29f 100644 --- a/src/localization_data.h +++ b/src/localization_data.h @@ -392,6 +392,13 @@ const loc_control_id control_id[] = { LOC_CTRL(MSG_280), LOC_CTRL(MSG_281), LOC_CTRL(MSG_282), + LOC_CTRL(MSG_283), + LOC_CTRL(MSG_284), + LOC_CTRL(MSG_285), + LOC_CTRL(MSG_286), + LOC_CTRL(MSG_287), + LOC_CTRL(MSG_288), + LOC_CTRL(MSG_289), LOC_CTRL(MSG_MAX), LOC_CTRL(IDOK), LOC_CTRL(IDCANCEL), diff --git a/src/pki.c b/src/pki.c index 0b4b0487..3fdb3812 100644 --- a/src/pki.c +++ b/src/pki.c @@ -30,10 +30,12 @@ #include "rufus.h" #include "msapi_utf8.h" +#include "resource.h" +#include "localization.h" #define ENCODING (X509_ASN_ENCODING | PKCS_7_ASN_ENCODING) -// Signatures names we accept (may be suffixed, but signature must start with one of those) +// Signatures names we accept (may be suffixed, but the signature should start with one of those) const char* valid_cert_names[] = { "Akeo Consulting", "Akeo Systems", "Pete Batard" }; typedef struct { @@ -43,9 +45,11 @@ typedef struct { } SPROG_PUBLISHERINFO, *PSPROG_PUBLISHERINFO; // Mostly from https://support.microsoft.com/en-us/kb/323809 -BOOL CheckSignatureAttributes(const char* path) +static char* GetSignatureName(const char* path) { - BOOL r = FALSE; + static char szSubjectName[128]; + char* p = NULL; + BOOL r; HCERTSTORE hStore = NULL; HCRYPTMSG hMsg = NULL; PCCERT_CONTEXT pCertContext = NULL; @@ -56,8 +60,6 @@ BOOL CheckSignatureAttributes(const char* path) CERT_INFO CertInfo = { 0 }; SPROG_PUBLISHERINFO ProgPubInfo = { 0 }; wchar_t *szFileName = utf8_to_wchar(path); - char szSubjectName[128]; - int i; // Get message handle and store handle from the signed file. r = CryptQueryObject(CERT_QUERY_OBJECT_FILE, szFileName, @@ -79,7 +81,6 @@ BOOL CheckSignatureAttributes(const char* path) pSignerInfo = (PCMSG_SIGNER_INFO)calloc(dwSignerInfo, 1); if (!pSignerInfo) { uprintf("PKI: Could not allocate memory for signer information"); - r = FALSE; goto out; } @@ -97,7 +98,6 @@ BOOL CheckSignatureAttributes(const char* path) pCertContext = CertFindCertificateInStore(hStore, ENCODING, 0, CERT_FIND_SUBJECT_CERT, (PVOID)&CertInfo, NULL); if (!pCertContext) { uprintf("PKI: Failed to locate signer certificate in temporary store: %s", WindowsErrorString()); - r = FALSE; goto out; } @@ -106,20 +106,11 @@ BOOL CheckSignatureAttributes(const char* path) szSubjectName, sizeof(szSubjectName)); if (dwSubjectSize <= 1) { uprintf("PKI: Failed to get Subject Name"); - r = FALSE; goto out; } - uprintf("Executable is signed by '%s'", szSubjectName); - - // Now check the signature name. Make it specific enough (i.e. don't simply check for "Akeo") - // so that, besides hacking our server, it'll place an extra hurdle on any malicious entity - // into also fooling a C.A. to issue a certificate that passes our test. - for (i = 0; i < ARRAYSIZE(valid_cert_names); i++) { - r = (strncmp(szSubjectName, valid_cert_names[i], strlen(valid_cert_names[i])) == 0); - if (r) - break; - } + uprintf("Downloaded executable is signed by '%s'", szSubjectName); + p = szSubjectName; out: safe_free(szFileName); @@ -134,7 +125,7 @@ out: CertCloseStore(hStore, 0); if (hMsg != NULL) CryptMsgClose(hMsg); - return r; + return p; } // From https://msdn.microsoft.com/en-us/library/windows/desktop/aa382384.aspx @@ -145,20 +136,47 @@ LONG ValidateSignature(HWND hDlg, const char* path) WINTRUST_FILE_INFO trust_file = { 0 }; GUID guid_generic_verify = // WINTRUST_ACTION_GENERIC_VERIFY_V2 { 0xaac56b, 0xcd44, 0x11d0,{ 0x8c, 0xc2, 0x0, 0xc0, 0x4f, 0xc2, 0x95, 0xee } }; - wchar_t *szFileName = utf8_to_wchar(path); + char *signature_name; + int i; + + // Check the signature name. Make it specific enough (i.e. don't simply check for "Akeo") + // so that, besides hacking our server, it'll place an extra hurdle on any malicious entity + // into also fooling a C.A. to issue a certificate that passes our test. + signature_name = GetSignatureName(path); + if (signature_name == NULL) { + uprintf("PKI: Could not get signature name"); + MessageBoxU(hDlg, lmprintf(MSG_284), lmprintf(MSG_283), MB_OK | MB_ICONERROR | MB_IS_RTL); + return TRUST_E_NOSIGNATURE; + } + for (i = 0; i < ARRAYSIZE(valid_cert_names); i++) { + if (strncmp(signature_name, valid_cert_names[i], strlen(valid_cert_names[i])) == 0) + break; + } + if (i >= ARRAYSIZE(valid_cert_names)) { + uprintf("PKI: Signature '%s' doesn't look right...", signature_name); + if (MessageBoxU(hDlg, lmprintf(MSG_285, signature_name), lmprintf(MSG_283), + MB_YESNO | MB_ICONWARNING | MB_IS_RTL) != IDYES) + return TRUST_E_EXPLICIT_DISTRUST; + } trust_file.cbStruct = sizeof(trust_file); - trust_file.pcwszFilePath = szFileName; + trust_file.pcwszFilePath = utf8_to_wchar(path); + if (trust_file.pcwszFilePath == NULL) { + uprintf("PKI: Unable to convert '%s' to UTF16", path); + return ERROR_SEVERITY_ERROR | FAC(FACILITY_CERT) | ERROR_NOT_ENOUGH_MEMORY; + } trust_data.cbStruct = sizeof(trust_data); trust_data.dwUIChoice = WTD_UI_ALL; + // We just downloaded from the Internet, so we should be able to check revocation trust_data.fdwRevocationChecks = WTD_REVOKE_WHOLECHAIN; - trust_data.dwUnionChoice = WTD_CHOICE_FILE; - trust_data.pFile = &trust_file; // 0x400 = WTD_MOTW for Windows 8.1 or later trust_data.dwProvFlags = WTD_REVOCATION_CHECK_CHAIN | 0x400; + trust_data.dwUnionChoice = WTD_CHOICE_FILE; + trust_data.pFile = &trust_file; r = WinVerifyTrust(NULL, &guid_generic_verify, &trust_data); - safe_free(szFileName); + safe_free(trust_file.pcwszFilePath); + return r; } diff --git a/src/resource.h b/src/resource.h index 7ebe5847..b5fa74e2 100644 --- a/src/resource.h +++ b/src/resource.h @@ -430,7 +430,14 @@ #define MSG_280 3280 #define MSG_281 3281 #define MSG_282 3282 -#define MSG_MAX 3283 +#define MSG_283 3283 +#define MSG_284 3284 +#define MSG_285 3285 +#define MSG_286 3286 +#define MSG_287 3287 +#define MSG_288 3288 +#define MSG_289 3289 +#define MSG_MAX 3290 // Next default values for new objects // diff --git a/src/rufus.c b/src/rufus.c index a21181d3..64a2db98 100644 --- a/src/rufus.c +++ b/src/rufus.c @@ -2054,8 +2054,6 @@ void SaveVHD(void) /* * Main dialog callback */ -extern BOOL CheckSignatureAttributes(const char* path); -extern LONG ValidateSignature(HWND hDlg, const char* path); static INT_PTR CALLBACK MainCallback(HWND hDlg, UINT message, WPARAM wParam, LPARAM lParam) { static DWORD DeviceNum = 0, LastRefresh = 0; @@ -2274,9 +2272,6 @@ static INT_PTR CALLBACK MainCallback(HWND hDlg, UINT message, WPARAM wParam, LPA break; #ifdef RUFUS_TEST case IDC_TEST: - uprintf("CHK: %d", CheckSignatureAttributes("C:\\Downloads\\rufus-2.4.exe")); - SetLastError(ValidateSignature(hDlg, "C:\\Downloads\\rufus-2.4.exe")); - uprintf("VAL: %s", WindowsErrorString()); break; #endif case IDC_ADVANCED: diff --git a/src/rufus.h b/src/rufus.h index bb5f40cd..03309df6 100644 --- a/src/rufus.h +++ b/src/rufus.h @@ -32,7 +32,7 @@ /* Program options */ #define RUFUS_DEBUG // print debug info to Debug facility /* Features not ready for prime time and that may *DESTROY* your data - USE AT YOUR OWN RISKS! */ -#define RUFUS_TEST +// #define RUFUS_TEST /* Languages for which translators are M.I.A. and that we could use help with */ #define LOST_TRANSLATORS { "ms-MY" } // NB: locales MUST be <= 5 chars /* Probability of getting the M.I.A. translator message. For more on this, see LostTranslatorCheck() */ @@ -449,6 +449,7 @@ extern BOOL IsBootableImage(const char* path); extern BOOL AppendVHDFooter(const char* vhd_path); extern int IsHDD(DWORD DriveIndex, uint16_t vid, uint16_t pid, const char* strid); extern void LostTranslatorCheck(void); +extern LONG ValidateSignature(HWND hDlg, const char* path); DWORD WINAPI FormatThread(void* param); DWORD WINAPI SaveImageThread(void* param); diff --git a/src/rufus.rc b/src/rufus.rc index 73a4e0f5..db864ce4 100644 --- a/src/rufus.rc +++ b/src/rufus.rc @@ -32,7 +32,7 @@ LANGUAGE LANG_NEUTRAL, SUBLANG_NEUTRAL IDD_DIALOG DIALOGEX 12, 12, 242, 376 STYLE DS_SETFONT | DS_MODALFRAME | DS_CENTER | WS_MINIMIZEBOX | WS_POPUP | WS_CAPTION | WS_SYSMENU -CAPTION "Rufus 2.5.766" +CAPTION "Rufus 2.5.767" FONT 8, "Segoe UI Symbol", 400, 0, 0x0 BEGIN LTEXT "Device",IDS_DEVICE_TXT,9,6,200,8 @@ -319,8 +319,8 @@ END // VS_VERSION_INFO VERSIONINFO - FILEVERSION 2,5,766,0 - PRODUCTVERSION 2,5,766,0 + FILEVERSION 2,5,767,0 + PRODUCTVERSION 2,5,767,0 FILEFLAGSMASK 0x3fL #ifdef _DEBUG FILEFLAGS 0x1L @@ -337,13 +337,13 @@ BEGIN BEGIN VALUE "CompanyName", "Akeo Consulting (http://akeo.ie)" VALUE "FileDescription", "Rufus" - VALUE "FileVersion", "2.5.766" + VALUE "FileVersion", "2.5.767" VALUE "InternalName", "Rufus" VALUE "LegalCopyright", "© 2011-2015 Pete Batard (GPL v3)" VALUE "LegalTrademarks", "http://www.gnu.org/copyleft/gpl.html" VALUE "OriginalFilename", "rufus.exe" VALUE "ProductName", "Rufus" - VALUE "ProductVersion", "2.5.766" + VALUE "ProductVersion", "2.5.767" END END BLOCK "VarFileInfo" diff --git a/src/stdlg.c b/src/stdlg.c index 10e95939..21f7b4d8 100644 --- a/src/stdlg.c +++ b/src/stdlg.c @@ -1435,11 +1435,11 @@ INT_PTR CALLBACK update_subclass_callback(HWND hDlg, UINT message, WPARAM wParam */ INT_PTR CALLBACK NewVersionCallback(HWND hDlg, UINT message, WPARAM wParam, LPARAM lParam) { - int i; - HWND hNotes; char cmdline[] = APPLICATION_NAME " -w 150"; static char* filepath = NULL; static int download_status = 0; + LONG i; + HWND hNotes; STARTUPINFOA si; PROCESS_INFORMATION pi; HFONT hyperlink_font = NULL; @@ -1497,12 +1497,15 @@ INT_PTR CALLBACK NewVersionCallback(HWND hDlg, UINT message, WPARAM wParam, LPAR break; case 2: // Launch newer version and close this one Sleep(1000); // Add a delay on account of antivirus scanners + + if (ValidateSignature(hDlg, filepath) != NO_ERROR) + break; + memset(&si, 0, sizeof(si)); memset(&pi, 0, sizeof(pi)); si.cb = sizeof(si); if (!CreateProcessU(filepath, cmdline, NULL, NULL, FALSE, 0, NULL, NULL, &si, &pi)) { PrintInfo(0, MSG_214); - // TODO: produce a message box and add a retry, as the file may be scanned by the Antivirus uprintf("Failed to launch new application: %s\n", WindowsErrorString()); } else { PrintInfo(0, MSG_213);