diff --git a/shim-bsc1088585-handle-mok-allocations-better.patch b/shim-bsc1088585-handle-mok-allocations-better.patch new file mode 100644 index 0000000..ddceb57 --- /dev/null +++ b/shim-bsc1088585-handle-mok-allocations-better.patch @@ -0,0 +1,219 @@ +From c232e8577b0608664fd4ce7a6b24b8ed7d2fc7a4 Mon Sep 17 00:00:00 2001 +From: Peter Jones +Date: Wed, 27 Sep 2017 14:17:20 -0400 +Subject: [PATCH] MokManager: handle mok parameter allocations better. + +Covscan daftly claims: + +288. var_compare_op: Comparing MokSB to null implies that MokSB might be null. +2330 if (MokSB) { +2331 menu_strings[i] = L"Change Secure Boot state"; +2332 menu_item[i] = MOK_CHANGE_SB; +2333 i++; +2334 } +2335 +... +2358 choice = console_select(perform_mok_mgmt, menu_strings, 0); +2359 if (choice < 0) +2360 goto out; +... +2362 switch (menu_item[choice]) { +... +2395 case MOK_CHANGE_SB: + CID 182841 (#1 of 1): Dereference after null check + (FORWARD_NULL)293. var_deref_model: Passing null pointer MokSB to + mok_sb_prompt, which dereferences it. [show details] +2396 efi_status = mok_sb_prompt(MokSB, MokSBSize); + +Which is, of course, entirely false, beause for menu_item[choice] to be +MOK_CHANGE_SB, MokSB must be !NULL. And then: + + 252. Condition efi_status == 0, taking true branch. +2397 if (efi_status == EFI_SUCCESS) +2398 MokSB = NULL; + +This guarantees it won't be in the list the next time through the loop. + +This adds tests for NULLness before mok_sb_prompt(), just to make it +more clear to covscan what's going on. + +Also do the same thing for all of: + MOK_CHANGE_SB + MOK_SET_PW + MOK_CHANGE_DB + MOK_ENROLL_MOKX + MOK_DELETE_MOKX + +I also Lindent-ed everything I had to touch. + +Three other minor errors are also fixed: +1) the loop in enter_mok_menu() leaked the menu allocations each time + through the loop +2) mok_sb_prompt(), mok_pw_prompt(), and mok_db_prompt() all call + FreePool() on their respective variables (MokSB, etc), and + check_mok_request() also calls FreePool() on these. This sounds + horrible, but it turns out it's not an issue, because they only free + them in their EFI_SUCCESS paths, and enter_mok_menu() resets the + system if any of the mok_XX_prompt() calls actually returned + EFI_SUCCESS, so we never get back to check_mok_request() for it to do + its FreePool() calls. +3) the loop in enter_mok_menu() winds up introducing a double free in + the call to free_menu(), but we also can't hit this bug, because all + the exit paths from the loop are "goto out" (or return error) rather + than actually exiting on the loop conditional. + +Signed-off-by: Peter Jones +(cherry picked from commit a32651360552559ee6a8978b5bcdc6e7dcc72b8c) +Gary Lin: Fixed the conflict against shim 14. +--- + MokManager.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++-------------- + 1 file changed, 46 insertions(+), 14 deletions(-) + +diff --git a/MokManager.c b/MokManager.c +index 55af321..42bf72d 100644 +--- a/MokManager.c ++++ b/MokManager.c +@@ -1060,9 +1060,6 @@ static EFI_STATUS mok_enrollment_prompt (void *MokNew, UINTN MokNewSize, int aut + } + } + +- if (MokNew) +- FreePool (MokNew); +- + return EFI_SUCCESS; + } + +@@ -1609,9 +1606,6 @@ static EFI_STATUS mok_sb_prompt (void *MokSB, UINTN MokSBSize) { + } + } + +- if (MokSB) +- FreePool(MokSB); +- + return EFI_SUCCESS; + } + +@@ -1729,9 +1723,6 @@ static EFI_STATUS mok_db_prompt (void *MokDB, UINTN MokDBSize) { + } + } + +- if (MokDB) +- FreePool(MokDB); +- + return EFI_SUCCESS; + } + +@@ -1800,9 +1791,6 @@ static EFI_STATUS mok_pw_prompt (void *MokPW, UINTN MokPWSize) { + mokpw_done: + LibDeleteVariable(L"MokPW", &shim_lock_guid); + +- if (MokPW) +- FreePool(MokPW); +- + return EFI_SUCCESS; + } + +@@ -2184,8 +2172,8 @@ static EFI_STATUS enter_mok_menu(EFI_HANDLE image_handle, + void *MokXNew, UINTN MokXNewSize, + void *MokXDel, UINTN MokXDelSize) + { +- CHAR16 **menu_strings; +- mok_menu_item *menu_item; ++ CHAR16 **menu_strings = NULL; ++ mok_menu_item *menu_item = NULL; + int choice = 0; + int mok_changed = 0; + EFI_STATUS efi_status; +@@ -2357,11 +2345,23 @@ static EFI_STATUS enter_mok_menu(EFI_HANDLE image_handle, + efi_status = mok_reset_prompt(FALSE); + break; + case MOK_ENROLL_MOK: ++ if (!MokNew) { ++ Print(L"MokManager: internal error: %s", ++ L"MokNew was !NULL but is now NULL\n"); ++ ret = EFI_ABORTED; ++ goto out; ++ } + efi_status = mok_enrollment_prompt(MokNew, MokNewSize, TRUE, FALSE); + if (efi_status == EFI_SUCCESS) + MokNew = NULL; + break; + case MOK_DELETE_MOK: ++ if (!MokDel) { ++ Print(L"MokManager: internal error: %s", ++ L"MokDel was !NULL but is now NULL\n"); ++ ret = EFI_ABORTED; ++ goto out; ++ } + efi_status = mok_deletion_prompt(MokDel, MokDelSize, FALSE); + if (efi_status == EFI_SUCCESS) + MokDel = NULL; +@@ -2370,26 +2370,56 @@ static EFI_STATUS enter_mok_menu(EFI_HANDLE image_handle, + efi_status = mok_reset_prompt(TRUE); + break; + case MOK_ENROLL_MOKX: ++ if (!MokXNew) { ++ Print(L"MokManager: internal error: %s", ++ L"MokXNew was !NULL but is now NULL\n"); ++ ret = EFI_ABORTED; ++ goto out; ++ } + efi_status = mok_enrollment_prompt(MokXNew, MokXNewSize, TRUE, TRUE); + if (efi_status == EFI_SUCCESS) + MokXNew = NULL; + break; + case MOK_DELETE_MOKX: ++ if (!MokXDel) { ++ Print(L"MokManager: internal error: %s", ++ L"MokXDel was !NULL but is now NULL\n"); ++ ret = EFI_ABORTED; ++ goto out; ++ } + efi_status = mok_deletion_prompt(MokXDel, MokXDelSize, TRUE); + if (efi_status == EFI_SUCCESS) + MokXDel = NULL; + break; + case MOK_CHANGE_SB: ++ if (!MokSB) { ++ Print(L"MokManager: internal error: %s", ++ L"MokSB was !NULL but is now NULL\n"); ++ ret = EFI_ABORTED; ++ goto out; ++ } + efi_status = mok_sb_prompt(MokSB, MokSBSize); + if (efi_status == EFI_SUCCESS) + MokSB = NULL; + break; + case MOK_SET_PW: ++ if (!MokPW) { ++ Print(L"MokManager: internal error: %s", ++ L"MokPW was !NULL but is now NULL\n"); ++ ret = EFI_ABORTED; ++ goto out; ++ } + efi_status = mok_pw_prompt(MokPW, MokPWSize); + if (efi_status == EFI_SUCCESS) + MokPW = NULL; + break; + case MOK_CHANGE_DB: ++ if (!MokDB) { ++ Print(L"MokManager: internal error: %s", ++ L"MokDB was !NULL but is now NULL\n"); ++ ret = EFI_ABORTED; ++ goto out; ++ } + efi_status = mok_db_prompt(MokDB, MokDBSize); + if (efi_status == EFI_SUCCESS) + MokDB = NULL; +@@ -2406,6 +2436,8 @@ static EFI_STATUS enter_mok_menu(EFI_HANDLE image_handle, + mok_changed = 1; + + free_menu(menu_item, menu_strings); ++ menu_item = NULL; ++ menu_strings = NULL; + } + + out: +-- +2.16.2 + diff --git a/shim-opensuse-cert-prompt.patch b/shim-opensuse-cert-prompt.patch index 30075ad..bc2be44 100644 --- a/shim-opensuse-cert-prompt.patch +++ b/shim-opensuse-cert-prompt.patch @@ -1,4 +1,4 @@ -From 7472a6ee1f01466df1a1de65de669ed0c20b12c4 Mon Sep 17 00:00:00 2001 +From aab03ce2522a3610ecfd5e2f9e896a1ccdd5a94a Mon Sep 17 00:00:00 2001 From: Gary Ching-Pang Lin Date: Tue, 18 Feb 2014 17:29:19 +0800 Subject: [PATCH 1/3] Show the build-in certificate prompt @@ -137,10 +137,10 @@ index 7b34868..be250b6 100644 if (EFI_ERROR(efi_status)) { Print(L"Something has gone seriously wrong: %r\n", efi_status); -- -2.15.1 +2.16.2 -From 3e3cf4589edf350c8c33d0f5069c6868c2810b80 Mon Sep 17 00:00:00 2001 +From d377f58aadd8c5579a922ef3c237d3ed25bb6d00 Mon Sep 17 00:00:00 2001 From: Gary Ching-Pang Lin Date: Thu, 20 Feb 2014 16:57:08 +0800 Subject: [PATCH 2/3] Support revoking the openSUSE cert @@ -156,10 +156,10 @@ will show up with an additional option to clear openSUSE_Verify 2 files changed, 60 insertions(+), 3 deletions(-) diff --git a/MokManager.c b/MokManager.c -index 55af321..678a9d9 100644 +index 42bf72d..7a2b5fe 100644 --- a/MokManager.c +++ b/MokManager.c -@@ -1806,6 +1806,33 @@ mokpw_done: +@@ -1794,6 +1794,33 @@ mokpw_done: return EFI_SUCCESS; } @@ -193,7 +193,7 @@ index 55af321..678a9d9 100644 static BOOLEAN verify_certificate(UINT8 *cert, UINTN size) { X509 *X509Cert; -@@ -2162,6 +2189,7 @@ typedef enum { +@@ -2150,6 +2177,7 @@ typedef enum { MOK_CHANGE_SB, MOK_SET_PW, MOK_CHANGE_DB, @@ -201,7 +201,7 @@ index 55af321..678a9d9 100644 MOK_KEY_ENROLL, MOK_HASH_ENROLL } mok_menu_item; -@@ -2182,7 +2210,8 @@ static EFI_STATUS enter_mok_menu(EFI_HANDLE image_handle, +@@ -2170,7 +2198,8 @@ static EFI_STATUS enter_mok_menu(EFI_HANDLE image_handle, void *MokPW, UINTN MokPWSize, void *MokDB, UINTN MokDBSize, void *MokXNew, UINTN MokXNewSize, @@ -209,9 +209,9 @@ index 55af321..678a9d9 100644 + void *MokXDel, UINTN MokXDelSize, + void *ClearVerify, UINTN ClearVerifySize) { - CHAR16 **menu_strings; - mok_menu_item *menu_item; -@@ -2262,6 +2291,9 @@ static EFI_STATUS enter_mok_menu(EFI_HANDLE image_handle, + CHAR16 **menu_strings = NULL; + mok_menu_item *menu_item = NULL; +@@ -2250,6 +2279,9 @@ static EFI_STATUS enter_mok_menu(EFI_HANDLE image_handle, if (MokDB) menucount++; @@ -221,7 +221,7 @@ index 55af321..678a9d9 100644 menu_strings = AllocateZeroPool(sizeof(CHAR16 *) * (menucount + 1)); if (!menu_strings) -@@ -2334,6 +2366,12 @@ static EFI_STATUS enter_mok_menu(EFI_HANDLE image_handle, +@@ -2322,6 +2354,12 @@ static EFI_STATUS enter_mok_menu(EFI_HANDLE image_handle, i++; } @@ -234,7 +234,7 @@ index 55af321..678a9d9 100644 menu_strings[i] = L"Enroll key from disk"; menu_item[i] = MOK_KEY_ENROLL; i++; -@@ -2394,6 +2432,9 @@ static EFI_STATUS enter_mok_menu(EFI_HANDLE image_handle, +@@ -2424,6 +2462,9 @@ static EFI_STATUS enter_mok_menu(EFI_HANDLE image_handle, if (efi_status == EFI_SUCCESS) MokDB = NULL; break; @@ -244,7 +244,7 @@ index 55af321..678a9d9 100644 case MOK_KEY_ENROLL: efi_status = mok_key_enroll(); break; -@@ -2424,6 +2465,7 @@ static EFI_STATUS check_mok_request(EFI_HANDLE image_handle) +@@ -2456,6 +2497,7 @@ static EFI_STATUS check_mok_request(EFI_HANDLE image_handle) EFI_GUID shim_lock_guid = SHIM_LOCK_GUID; UINTN MokNewSize = 0, MokDelSize = 0, MokSBSize = 0, MokPWSize = 0; UINTN MokDBSize = 0, MokXNewSize = 0, MokXDelSize = 0; @@ -252,7 +252,7 @@ index 55af321..678a9d9 100644 void *MokNew = NULL; void *MokDel = NULL; void *MokSB = NULL; -@@ -2431,6 +2473,7 @@ static EFI_STATUS check_mok_request(EFI_HANDLE image_handle) +@@ -2463,6 +2505,7 @@ static EFI_STATUS check_mok_request(EFI_HANDLE image_handle) void *MokDB = NULL; void *MokXNew = NULL; void *MokXDel = NULL; @@ -260,7 +260,7 @@ index 55af321..678a9d9 100644 EFI_STATUS status; status = get_variable(L"MokNew", (UINT8 **)&MokNew, &MokNewSize, -@@ -2503,9 +2546,20 @@ static EFI_STATUS check_mok_request(EFI_HANDLE image_handle) +@@ -2535,9 +2578,20 @@ static EFI_STATUS check_mok_request(EFI_HANDLE image_handle) console_error(L"Could not retrieve MokXDel", status); } @@ -282,7 +282,7 @@ index 55af321..678a9d9 100644 if (MokNew) FreePool (MokNew); -@@ -2528,6 +2582,9 @@ static EFI_STATUS check_mok_request(EFI_HANDLE image_handle) +@@ -2560,6 +2614,9 @@ static EFI_STATUS check_mok_request(EFI_HANDLE image_handle) if (MokXDel) FreePool (MokXDel); @@ -306,10 +306,10 @@ index be250b6..d461edd 100644 if (efi_status != EFI_SUCCESS) { -- -2.15.1 +2.16.2 -From b5348293dd95c6627f8fde0344650e006acc181b Mon Sep 17 00:00:00 2001 +From 5a60e36a5c2bad616bc842d7ffaa6acc1493650f Mon Sep 17 00:00:00 2001 From: Gary Ching-Pang Lin Date: Fri, 7 Mar 2014 16:17:20 +0800 Subject: [PATCH 3/3] Delete openSUSE_Verify the right way @@ -322,10 +322,10 @@ LibDeleteVariable only works on the runtime variables. 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/MokManager.c b/MokManager.c -index 678a9d9..c3f8f45 100644 +index 7a2b5fe..feae113 100644 --- a/MokManager.c +++ b/MokManager.c -@@ -1820,7 +1820,10 @@ static INTN mok_clear_verify_prompt(void *ClearVerify, UINTN ClearVerifySize) { +@@ -1808,7 +1808,10 @@ static INTN mok_clear_verify_prompt(void *ClearVerify, UINTN ClearVerifySize) { if (status != EFI_SUCCESS) return -1; @@ -338,5 +338,5 @@ index 678a9d9..c3f8f45 100644 console_error(L"Failed to delete openSUSE_Verify", status); return -1; -- -2.15.1 +2.16.2 diff --git a/shim.changes b/shim.changes index 74ad9ca..f001cdc 100644 --- a/shim.changes +++ b/shim.changes @@ -1,3 +1,11 @@ +------------------------------------------------------------------- +Tue Apr 10 03:45:39 UTC 2018 - glin@suse.com + +- Add shim-bsc1088585-handle-mok-allocations-better.patch to avoid + double-freeing after enrolling a key from the disk (bsc#1088585) + + Also refresh shim-opensuse-cert-prompt.patch due to the change + in MokManager.c + ------------------------------------------------------------------- Tue Apr 3 08:37:55 UTC 2018 - glin@suse.com diff --git a/shim.spec b/shim.spec index 527d439..07ab662 100644 --- a/shim.spec +++ b/shim.spec @@ -54,6 +54,8 @@ Patch3: shim-httpboot-include-console.h.patch Patch4: shim-remove-cryptpem.patch # PATCH-FIX-UPSTREAM shim-httpboot-amend-device-path.patch bsc#1065370 glin@suse.com -- Amend the device path matching rule for httpboot Patch5: shim-httpboot-amend-device-path.patch +# PATCH-FIX-UPSTREAM shim-bsc1088585-handle-mok-allocations-better.patch bsc#1088585 glin@suse.com -- Handle the mok parameter allocations better +Patch6: shim-bsc1088585-handle-mok-allocations-better.patch # PATCH-FIX-OPENSUSE shim-change-debug-file-path.patch glin@suse.com -- Change the default debug file path Patch50: shim-change-debug-file-path.patch # PATCH-FIX-OPENSUSE shim-opensuse-cert-prompt.patch glin@suse.com -- Show the prompt to ask whether the user trusts openSUSE certificate or not @@ -103,6 +105,7 @@ The source code of UEFI shim loader %patch3 -p1 %patch4 -p1 %patch5 -p1 +%patch6 -p1 %patch50 -p1 %if 0%{?is_opensuse} == 1 %patch100 -p1