From f26fd2fbe36882534414f72ca153bc28c7f41a8c Mon Sep 17 00:00:00 2001 From: Pete Batard Date: Thu, 27 May 2021 00:18:05 +0100 Subject: [PATCH] [fido] add additional Authenticode validation before running the script * This basically means that the script is validate *TWICE*, using two completely independent signatures, before it is allowed to run, which should add another mitigation layer against TOCTOU (which we already friggin' mitigated against anyway) and other potential vectors of attack. * Also remove -DisableFirstRunCustomize option and the associated cookie prompt monitoring, which the latest version of Fido no longer requires. * Also update WDK version for signtool and flesh out PKI error messages. --- _sign.cmd | 2 +- res/appstore/Package.appxmanifest | 2 +- src/net.c | 23 +++++++++++------ src/pki.c | 41 ++++++++++++++++++++++--------- src/rufus.c | 2 +- src/rufus.h | 2 +- src/rufus.rc | 10 ++++---- src/stdlg.c | 30 ++++++---------------- 8 files changed, 63 insertions(+), 49 deletions(-) diff --git a/_sign.cmd b/_sign.cmd index 1207bc91..73f01ed2 100644 --- a/_sign.cmd +++ b/_sign.cmd @@ -1,3 +1,3 @@ @echo off -"C:\Program Files (x86)\Windows Kits\10\bin\10.0.18362.0\x64\signtool" sign /v /sha1 9ce9a71ccab3b38a74781b975f1c228222cf7d3b /fd SHA256 /tr http://sha256timestamp.ws.symantec.com/sha256/timestamp %1 +"C:\Program Files (x86)\Windows Kits\10\bin\10.0.19041.0\x64\signtool" sign /v /sha1 9ce9a71ccab3b38a74781b975f1c228222cf7d3b /fd SHA256 /tr http://sha256timestamp.ws.symantec.com/sha256/timestamp %1 exit diff --git a/res/appstore/Package.appxmanifest b/res/appstore/Package.appxmanifest index 650152ef..9b54f8e2 100644 --- a/res/appstore/Package.appxmanifest +++ b/res/appstore/Package.appxmanifest @@ -11,7 +11,7 @@ + Version="3.14.1794.0" /> Rufus diff --git a/src/net.c b/src/net.c index 97f9b41e..1e1597c0 100644 --- a/src/net.c +++ b/src/net.c @@ -52,7 +52,7 @@ HANDLE update_check_thread = NULL; extern loc_cmd* selected_locale; extern HANDLE dialog_handle; -extern BOOL is_x86_32, close_fido_cookie_prompts; +extern BOOL is_x86_32; static DWORD error_code, fido_len = 0; static BOOL force_update_check = FALSE; static const char* request_headers = "Accept-Encoding: gzip, deflate"; @@ -918,7 +918,7 @@ static DWORD WINAPI DownloadISOThread(LPVOID param) static_sprintf(sig_url, "%s.sig", fido_url); dwSize = (DWORD)DownloadToFileOrBuffer(sig_url, NULL, &sig, NULL, FALSE); if ((dwSize != RSA_SIGNATURE_SIZE) || (!ValidateOpensslSignature(compressed, dwCompressedSize, sig, dwSize))) { - uprintf("FATAL: Signature is invalid ✗"); + uprintf("FATAL: Download signature is invalid ✗"); FormatStatus = ERROR_SEVERITY_ERROR | FAC(FACILITY_STORAGE) | APPERR(ERROR_BAD_SIGNATURE); SendMessage(hProgress, PBM_SETSTATE, (WPARAM)PBST_ERROR, 0); SetTaskbarProgressState(TASKBAR_ERROR); @@ -927,7 +927,7 @@ static DWORD WINAPI DownloadISOThread(LPVOID param) goto out; } free(sig); - uprintf("Signature is valid ✓"); + uprintf("Download signature is valid ✓"); uncompressed_size = *((uint64_t*)&compressed[5]); if ((uncompressed_size < 1 * MB) && (bled_init(_uprintf, NULL, NULL, NULL, NULL, &FormatStatus) >= 0)) { fido_script = malloc((size_t)uncompressed_size); @@ -983,13 +983,22 @@ static DWORD WINAPI DownloadISOThread(LPVOID param) } static_sprintf(cmdline, "\"%s\" -NonInteractive -Sta -NoProfile –ExecutionPolicy Bypass " - "-File \"%s\" -DisableFirstRunCustomize -PipeName %s -LocData \"%s\" -Icon \"%s\" -AppTitle \"%s\"", + "-File \"%s\" -PipeName %s -LocData \"%s\" -Icon \"%s\" -AppTitle \"%s\"", powershell_path, script_path, &pipe[9], locale_str, icon_path, lmprintf(MSG_149)); - // Signal our Windows alert hook that it should close the IE cookie prompts from Fido - close_fido_cookie_prompts = TRUE; + + // For extra security, even after we validated that the LZMA download is properly + // signed, we also validate the Authenticode signature of the local script. + if (ValidateSignature(INVALID_HANDLE_VALUE, script_path) != NO_ERROR) { + uprintf("FATAL: Script signature is invalid ✗"); + FormatStatus = ERROR_SEVERITY_ERROR | FAC(FACILITY_STORAGE) | APPERR(ERROR_BAD_SIGNATURE); + SendMessage(hProgress, PBM_SETSTATE, (WPARAM)PBST_ERROR, 0); + SetTaskbarProgressState(TASKBAR_ERROR); + goto out; + } + uprintf("Script signature is valid ✓"); + dwExitCode = RunCommand(cmdline, app_dir, TRUE); uprintf("Exited download script with code: %d", dwExitCode); - close_fido_cookie_prompts = FALSE; if ((dwExitCode == 0) && PeekNamedPipe(hPipe, NULL, dwPipeSize, NULL, &dwAvail, NULL) && (dwAvail != 0)) { url = malloc(dwAvail + 1); dwSize = 0; diff --git a/src/pki.c b/src/pki.c index a94a15fb..251d6cb1 100644 --- a/src/pki.c +++ b/src/pki.c @@ -192,12 +192,24 @@ const char* WinPKIErrorString(void) return "None of the signers of the cryptographic message or certificate trust list is trusted."; case CERT_E_UNTRUSTEDROOT: return "The root certificate is not trusted."; + case TRUST_E_SYSTEM_ERROR: + return "A system-level error occurred while verifying trust."; + case TRUST_E_NO_SIGNER_CERT: + return "The certificate for the signer of the message is invalid or not found."; + case TRUST_E_COUNTER_SIGNER: + return "One of the counter signatures was invalid."; + case TRUST_E_CERT_SIGNATURE: + return "The signature of the certificate cannot be verified."; + case TRUST_E_TIME_STAMP: + return "The timestamp could not be verified."; + case TRUST_E_BAD_DIGEST: + return "The file content has been altered."; + case TRUST_E_BASIC_CONSTRAINTS: + return "A certificate's basic constraint extension has not been observed."; case TRUST_E_NOSIGNATURE: return "Not digitally signed."; case TRUST_E_EXPLICIT_DISTRUST: return "One of the certificates used was marked as untrusted by the user."; - case TRUST_E_TIME_STAMP: - return "The timestamp could not be verified."; default: static_sprintf(error_string, "Unknown PKI error 0x%08lX", error_code); return error_string; @@ -205,7 +217,7 @@ const char* WinPKIErrorString(void) } // Mostly from https://support.microsoft.com/en-us/kb/323809 -char* GetSignatureName(const char* path, const char* country_code) +char* GetSignatureName(const char* path, const char* country_code, BOOL bSilent) { static char szSubjectName[128]; char szCountry[3] = "__"; @@ -308,9 +320,9 @@ char* GetSignatureName(const char* path, const char* country_code) } if (szCountry[0] == '_') - uprintf("Binary executable is signed by '%s'", szSubjectName); + suprintf("Binary executable is signed by '%s'", szSubjectName); else - uprintf("Binary executable is signed by '%s' (%s)", szSubjectName, szCountry); + suprintf("Binary executable is signed by '%s' (%s)", szSubjectName, szCountry); p = szSubjectName; out: @@ -572,10 +584,11 @@ LONG ValidateSignature(HWND hDlg, const char* path) // 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, cert_country); + signature_name = GetSignatureName(path, cert_country, (hDlg == INVALID_HANDLE_VALUE)); if (signature_name == NULL) { uprintf("PKI: Could not get signature name"); - MessageBoxExU(hDlg, lmprintf(MSG_284), lmprintf(MSG_283), MB_OK | MB_ICONERROR | MB_IS_RTL, selected_langid); + if (hDlg != INVALID_HANDLE_VALUE) + MessageBoxExU(hDlg, lmprintf(MSG_284), lmprintf(MSG_283), MB_OK | MB_ICONERROR | MB_IS_RTL, selected_langid); return TRUST_E_NOSIGNATURE; } for (i = 0; i < ARRAYSIZE(cert_name); i++) { @@ -584,8 +597,9 @@ LONG ValidateSignature(HWND hDlg, const char* path) } if (i >= ARRAYSIZE(cert_name)) { uprintf("PKI: Signature '%s' is unexpected...", signature_name); - if (MessageBoxExU(hDlg, lmprintf(MSG_285, signature_name), lmprintf(MSG_283), - MB_YESNO | MB_ICONWARNING | MB_IS_RTL, selected_langid) != IDYES) + if ((hDlg == INVALID_HANDLE_VALUE) || (MessageBoxExU(hDlg, + lmprintf(MSG_285, signature_name), lmprintf(MSG_283), + MB_YESNO | MB_ICONWARNING | MB_IS_RTL, selected_langid) != IDYES)) return TRUST_E_EXPLICIT_DISTRUST; } @@ -615,6 +629,9 @@ LONG ValidateSignature(HWND hDlg, const char* path) safe_free(trust_file.pcwszFilePath); switch (r) { case ERROR_SUCCESS: + // hDlg = INVALID_HANDLE_VALUE is used when validating the Fido PS1 script + if (hDlg == INVALID_HANDLE_VALUE) + break; // Verify that the timestamp of the downloaded update is in the future of our current one. // This is done to prevent the use of an officially signed, but older binary, as potential attack vector. current_ts = GetSignatureTimeStamp(NULL); @@ -634,11 +651,13 @@ LONG ValidateSignature(HWND hDlg, const char* path) case TRUST_E_NOSIGNATURE: // Should already have been reported, but since we have a custom message for it... uprintf("PKI: File does not appear to be signed: %s", WinPKIErrorString()); - MessageBoxExU(hDlg, lmprintf(MSG_284), lmprintf(MSG_283), MB_OK | MB_ICONERROR | MB_IS_RTL, selected_langid); + if (hDlg != INVALID_HANDLE_VALUE) + MessageBoxExU(hDlg, lmprintf(MSG_284), lmprintf(MSG_283), MB_OK | MB_ICONERROR | MB_IS_RTL, selected_langid); break; default: uprintf("PKI: Failed to validate signature: %s", WinPKIErrorString()); - MessageBoxExU(hDlg, lmprintf(MSG_240), lmprintf(MSG_283), MB_OK | MB_ICONERROR | MB_IS_RTL, selected_langid); + if (hDlg != INVALID_HANDLE_VALUE) + MessageBoxExU(hDlg, lmprintf(MSG_240), lmprintf(MSG_283), MB_OK | MB_ICONERROR | MB_IS_RTL, selected_langid); break; } diff --git a/src/rufus.c b/src/rufus.c index d8a20c14..4682c3c9 100755 --- a/src/rufus.c +++ b/src/rufus.c @@ -3335,7 +3335,7 @@ skip_args_processing: static_sprintf(ini_path, "%s\\rufus.ini", app_dir); fd = fopenU(ini_path, ini_flags); // Will create the file if portable mode is requested // Using the string directly in safe_strcmp() would call GetSignatureName() twice - tmp = GetSignatureName(NULL, NULL); + tmp = GetSignatureName(NULL, NULL, FALSE); vc |= (safe_strcmp(tmp, cert_name[0]) == 0); if (fd != NULL) { ini_file = ini_path; diff --git a/src/rufus.h b/src/rufus.h index 8555e25f..80e60d4c 100644 --- a/src/rufus.h +++ b/src/rufus.h @@ -572,7 +572,7 @@ extern uint8_t IsBootableImage(const char* path); extern BOOL AppendVHDFooter(const char* vhd_path); extern int SetWinToGoIndex(void); extern int IsHDD(DWORD DriveIndex, uint16_t vid, uint16_t pid, const char* strid); -extern char* GetSignatureName(const char* path, const char* country_code); +extern char* GetSignatureName(const char* path, const char* country_code, BOOL bSilent); extern uint64_t GetSignatureTimeStamp(const char* path); extern LONG ValidateSignature(HWND hDlg, const char* path); extern BOOL ValidateOpensslSignature(BYTE* pbBuffer, DWORD dwBufferLen, BYTE* pbSignature, DWORD dwSigLen); diff --git a/src/rufus.rc b/src/rufus.rc index 16628880..f1f56cb6 100644 --- a/src/rufus.rc +++ b/src/rufus.rc @@ -33,7 +33,7 @@ LANGUAGE LANG_NEUTRAL, SUBLANG_NEUTRAL IDD_DIALOG DIALOGEX 12, 12, 232, 326 STYLE DS_SETFONT | DS_MODALFRAME | DS_CENTER | WS_MINIMIZEBOX | WS_POPUP | WS_CAPTION | WS_SYSMENU EXSTYLE WS_EX_ACCEPTFILES -CAPTION "Rufus 3.14.1793" +CAPTION "Rufus 3.14.1794" FONT 9, "Segoe UI Symbol", 400, 0, 0x0 BEGIN LTEXT "Drive Properties",IDS_DRIVE_PROPERTIES_TXT,8,6,53,12,NOT WS_GROUP @@ -395,8 +395,8 @@ END // VS_VERSION_INFO VERSIONINFO - FILEVERSION 3,14,1793,0 - PRODUCTVERSION 3,14,1793,0 + FILEVERSION 3,14,1794,0 + PRODUCTVERSION 3,14,1794,0 FILEFLAGSMASK 0x3fL #ifdef _DEBUG FILEFLAGS 0x1L @@ -414,13 +414,13 @@ BEGIN VALUE "Comments", "https://rufus.ie" VALUE "CompanyName", "Akeo Consulting" VALUE "FileDescription", "Rufus" - VALUE "FileVersion", "3.14.1793" + VALUE "FileVersion", "3.14.1794" VALUE "InternalName", "Rufus" VALUE "LegalCopyright", "© 2011-2021 Pete Batard (GPL v3)" VALUE "LegalTrademarks", "https://www.gnu.org/licenses/gpl-3.0.html" VALUE "OriginalFilename", "rufus-3.14.exe" VALUE "ProductName", "Rufus" - VALUE "ProductVersion", "3.14.1793" + VALUE "ProductVersion", "3.14.1794" END END BLOCK "VarFileInfo" diff --git a/src/stdlg.c b/src/stdlg.c index 5fe5c4fc..9769a599 100644 --- a/src/stdlg.c +++ b/src/stdlg.c @@ -59,9 +59,8 @@ static const notification_info* notification_more_info; static const char* notification_dont_display_setting; static WNDPROC update_original_proc = NULL; static HWINEVENTHOOK ap_weh = NULL; -static char title_str[3][128], button_str[128]; +static char title_str[2][128], button_str[128]; HWND hFidoDlg = NULL; -BOOL close_fido_cookie_prompts = FALSE; static int update_settings_reposition_ids[] = { IDI_ICON, @@ -1989,14 +1988,13 @@ INT_PTR MyDialogBox(HINSTANCE hInstance, int Dialog_ID, HWND hWndParent, DLGPROC /* * The following function calls are used to automatically detect and close the native - * Windows format prompt "You must format the disk in drive X:" as well as the cookies - * alert being popped by Windows when running our Download script. To do that, we use + * Windows format prompt "You must format the disk in drive X:". To do that, we use * an event hook that gets triggered whenever a window is placed in the foreground. * In that hook, we look for a dialog that has style WS_POPUPWINDOW and has the relevant - * title. However, in case of the Format prompt, because the title in itself is too - * generic (the expectation is that it will be "Microsoft Windows") we also enumerate - * all the child controls from that prompt, using another callback, until we find one - * that contains the text we expect for the "Format disk" button. + * title. However, because the title in itself is too generic (the expectation is that + * it will be "Microsoft Windows") we also enumerate all the child controls from that + * prompt, using another callback, until we find one that contains the text we expect + * for the "Format disk" button. * Oh, and since all of these strings are localized, we must first pick them up from * the relevant mui's. */ @@ -2028,9 +2026,7 @@ static void CALLBACK AlertPromptHook(HWINEVENTHOOK hWinEventHook, DWORD Event, H SendMessage(hWnd, WM_COMMAND, (WPARAM)IDCANCEL, (LPARAM)0); uprintf("Closed Windows format prompt"); } - } else if (close_fido_cookie_prompts && strcmp(str, title_str[1]) == 0) { - SendMessage(hWnd, WM_COMMAND, (WPARAM)IDCANCEL, (LPARAM)0); - } else if ((strcmp(str, title_str[2]) == 0) && (hWnd != hFidoDlg)) { + } else if ((strcmp(str, title_str[1]) == 0) && (hWnd != hFidoDlg)) { // A wild Fido dialog appeared! => Keep track of its handle and center it hFidoDlg = hWnd; CenterDialog(hWnd, hMainDialog); @@ -2062,17 +2058,7 @@ void SetAlertPromptMessages(void) } FreeLibrary(mui_lib); } - static_sprintf(mui_path, "%s\\%s\\urlmon.dll.mui", sysnative_dir, GetCurrentMUI()); - mui_lib = LoadLibraryExU(mui_path, NULL, LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR); - if (mui_lib != NULL) { - // 2070 = "Windows Security Warning" (yes, that's what MS uses for a stupid cookie!) - if (LoadStringU(mui_lib, 2070, title_str[1], sizeof(title_str[1])) <= 0) { - static_strcpy(title_str[1], "Windows Security Warning"); - uprintf("Warning: Could not locate localized cookie prompt title string in '%s': %s", mui_path, WindowsErrorString()); - } - FreeLibrary(mui_lib); - } - static_strcpy(title_str[2], lmprintf(MSG_149)); + static_strcpy(title_str[1], lmprintf(MSG_149)); } BOOL SetAlertPromptHook(void)