Date
1 - 3 of 3
[PATCH V4 00/13] Disable the deprecated MD5 and SHA1 support
|
Gao, Zhichao
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3003
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3021 REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3027 MD5 is deprecated, make it disable as default for security. It required to set MD5 enable explicitly if the module is still using MD5. List the modules that are still using it: iSCSI, Hash2DxeCrypto, CryptoDxe(Pei, Smm) (with PACKAGE or ALL config). This patch set would affact the platforms that are using iSCSI function. V2: Remove MD5 and SHA1 support of Hash2DxeCrypto. Remove the MD5 GUID defination in MdePkg.dec. SHA1 related GUIDs are still using in TPM2, so keep them. No requirement to add MD5 enable MACRO in SecurityPkg. V3: Explicitly enable iSCSI for ArmVirtQemu, ArmVirtQemuKernel, OvmfPkgIa32, OvmfPkgIa32X64, OvmfPkgX64 and BhyveX64. And set the MD5 enable base on the new MD5 MACRO. Rejust the patch order. V14: Fix some typos. Change the commit message. Add NetworkBuildOptions.dsc.inc and add the MACRO for different toolchain. Using inc file in the related package dsc file: ArmVirtQemu, ArmVirtQemuKernel, OvmfPkgIa32, OvmfPkgIa32X64, OvmfPkgX64, OvmfXen and BhyveX64. Enable iSCSI in NetworkPkg.dsc for build test. Cc: Jordan Justen <jordan.l.justen@...> Cc: Laszlo Ersek <lersek@...> Cc: Ard Biesheuvel <ard.biesheuvel@...> Cc: Sami Mujawar <sami.mujawar@...> Cc: Leif Lindholm <leif@...> Cc: Jiewen Yao <jiewen.yao@...> Cc: Jian J Wang <jian.j.wang@...> Cc: Xiaoyu Lu <xiaoyux.lu@...> Cc: Guomin Jiang <guomin.jiang@...> Cc: Michael D Kinney <michael.d.kinney@...> Cc: Kelly Steele <kelly.steele@...> Cc: Zailiang Sun <zailiang.sun@...> Cc: Yi Qian <yi.qian@...> Cc: Liming Gao <gaoliming@...> Cc: Maciej Rabeda <maciej.rabeda@...> Cc: Jiaxin Wu <jiaxin.wu@...> Cc: Siyuan Fu <siyuan.fu@...> Cc: Roger Feng <roger.feng@...> Cc: Zhiguang Liu <zhiguang.liu@...> Signed-off-by: Zhichao Gao <zhichao.gao@...> Zhichao Gao (13): SecurityPkg/Hash2DxeCrypto: Remove MD5 support SecurityPkg/Hash2DxeCrypto: Remove SHA1 support CryptoPkg/dsc: Enable MD5 when CRYPTO_SERVICES enable MD5 NetworkPkg: Enable MD5 while enable iSCSI ArmVirtPkg/ArmVirtQemu.dsc: Enable MD5 while enable iSCSI ArmVirtPkg/ArmVirtQemuKernel.dsc: Enable MD5 while enable iSCSI OvmfPkg/OvmfPkgIa32.dsc: Enable MD5 while enable iSCSI OvmfPkg/OvmfPkgIa32X64.dsc: Enable MD5 while enable iSCSI OvmfPkg/OvmfPkgX64.dsc: Enable MD5 while enable iSCSI OvmfPkg/OvmfXen.dsc: Enable MD5 while enable iSCSI OvmfPkg/BhyveX64.dsc: Enable MD5 while enable iSCSI NetworkPkg/Defines: Make iSCSI disable as default CryptoPkg: Make the MD5 disable as default for security ArmVirtPkg/ArmVirtQemu.dsc | 6 ++++- ArmVirtPkg/ArmVirtQemuKernel.dsc | 5 ++++- CryptoPkg/CryptoPkg.dsc | 6 +++++ CryptoPkg/Driver/Crypto.c | 4 ++-- CryptoPkg/Include/Library/BaseCryptLib.h | 2 +- .../Library/BaseCryptLib/Hash/CryptMd5.c | 2 +- .../BaseCryptLibOnProtocolPpi/CryptLib.c | 2 +- NetworkPkg/Network.dsc.inc | 5 ++++- NetworkPkg/NetworkBuildOptions.dsc.inc | 22 +++++++++++++++++++ NetworkPkg/NetworkDefines.dsc.inc | 4 ++-- NetworkPkg/NetworkPkg.dsc | 4 +++- OvmfPkg/Bhyve/BhyveX64.dsc | 5 ++++- OvmfPkg/OvmfPkgIa32.dsc | 3 +++ OvmfPkg/OvmfPkgIa32X64.dsc | 3 +++ OvmfPkg/OvmfPkgX64.dsc | 3 +++ OvmfPkg/OvmfXen.dsc | 3 +++ SecurityPkg/Hash2DxeCrypto/Hash2DxeCrypto.c | 2 -- SecurityPkg/Hash2DxeCrypto/Hash2DxeCrypto.inf | 4 +--- 18 files changed, 68 insertions(+), 17 deletions(-) create mode 100644 NetworkPkg/NetworkBuildOptions.dsc.inc -- 2.21.0.windows.1 |
|
|
|
Laszlo Ersek
On 11/12/20 06:55, Gao, Zhichao wrote:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3003I'm in the process of merging this series. * For composing the branch / merge request: (a) I have applied the v4 patches locally. (b) I have added the feedback tags from the v4 thread. (c) I have implemented changes (1) through (3) suggested here: [edk2-devel] [PATCH V4 04/13] NetworkPkg: Enable MD5 while enable iSCSI https://edk2.groups.io/g/devel/message/67558 https://www.redhat.com/archives/edk2-devel-archive/2020-November/msg00718.html (d) I have also lifted the [BuildOptions] section above [Components] in: [edk2-devel] [PATCH V4 04/13] NetworkPkg: Enable MD5 while enable iSCSI according to the following discussion: https://edk2.groups.io/g/devel/message/67525 https://www.redhat.com/archives/edk2-devel-archive/2020-November/msg00684.html and https://edk2.groups.io/g/devel/message/67591 https://www.redhat.com/archives/edk2-devel-archive/2020-November/msg00751.html My (c) and (d) changes match the updates in Zhichao's commit 41ac3e7fa126, on his "MD5_disable" branch. * The above actions of mine result in the following range-diff, relative to the v4 posting on the list: 1: c476b165a734 = 1: 5ee36bcf2a98 SecurityPkg/Hash2DxeCrypto: Remove MD5 support* At this point, I have also compared my local branch (to be merged) with Zhichao's MD5_disable branch, currently at commit e9f94f099f83. Please see the range-diff below, with my comments (the changes are expressed as working from Zhichao's branch to mine): 1: 3be76c44753f ! 1: 5ee36bcf2a98 SecurityPkg/Hash2DxeCrypto: Remove MD5 supportThis is worth highlighting. Jiewen had given his R-b originally on patch #3 "CryptoPkg/dsc: Enable MD5 when CRYPTO_SERVICES enable MD5", namely in the v2 thread: https://edk2.groups.io/g/devel/message/66629 https://www.redhat.com/archives/edk2-devel-archive/2020-October/msg00848.html However, under v3 I pointed out that the patch was not right: https://edk2.groups.io/g/devel/message/67319 https://www.redhat.com/archives/edk2-devel-archive/2020-November/msg00476.html Accordingly, Zhichao updated the patch in v4, and *correctly* dropped Jiewen's R-b from v2 (as the patch has changed non-trivially): https://edk2.groups.io/g/devel/message/67362 https://www.redhat.com/archives/edk2-devel-archive/2020-November/msg00521.html Note that Jiewen did not re-review patch#3 in v4! Therefore, Zhichao, it was *incorrect* from you to just go back to v2 and reapply Jiewen's R-b from there. You were right to *drop* it in v4, and you were very wrong to *silently* re-add it on your "MD5_disble" branch! (Commit 2439f393cb9b.) Therefore, this CryptoPkg patch is now going in with *only* my review, even though I'm not even a designated reviewer for CryptoPkg, let alone a maintainer. Merging patch#3 like this is not "ideal", to say the least, but the v4 patch has been on the list for 5 days now, and Jiewen was correctly CC'd on it. It would have been a really simple incremental review. I'm quite fed up with reviewers ignoring their responsibilities, so it's either this, or we can delay (or revert) the series until after the stable tag is released. 4: 41ac3e7fa126 ! 4: c17e682d01ce NetworkPkg: Enable MD5 while enable iSCSIHere Zhichao mistakenly applied an R-b from Maciej. Maciej did not provide an R-b for this patch, as far as I can see. Siyuan did review the patch, so the patch is OK to merge: https://edk2.groups.io/g/devel/message/67602 https://www.redhat.com/archives/edk2-devel-archive/2020-November/msg00762.html 5: 7ffc00523613 ! 5: 408c15466aa6 ArmVirtPkg/ArmVirtQemu.dsc: Enable MD5 while enable iSCSIHere Zhichao missed my Build-tested-by: https://edk2.groups.io/g/devel/message/67559 https://www.redhat.com/archives/edk2-devel-archive/2020-November/msg00719.html 6: f832f46e1330 ! 6: 6f699db9003e ArmVirtPkg/ArmVirtQemuKernel.dsc: Enable MD5 while enable iSCSIThe above process mistakes exemplify why I'm very reluctant to merge branches from personal repositories "after review", as opposed to picking up patches, feedback, and updates from the list, and composing the branch pull request manually. I'm going to submit a pull request now with my branch, and report back with the commit range (hopefully). Thanks, Laszlo |
|
|
|
Laszlo Ersek
On 11/17/20 20:16, Laszlo Ersek wrote:
I'm going to submit a pull request now with my branch, and report backSeries merged as commit range 29d59baa3907..e6a12a0fc817, via <https://github.com/tianocore/edk2/pull/1130>. Thanks, Laszlo |
|
|