From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 2153E2265A32A for ; Wed, 11 Apr 2018 01:33:55 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 568B68010F67; Wed, 11 Apr 2018 08:33:54 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-97.rdu2.redhat.com [10.10.120.97]) by smtp.corp.redhat.com (Postfix) with ESMTP id A91A4215CDC6; Wed, 11 Apr 2018 08:33:53 +0000 (UTC) To: Long Qin , ting.ye@intel.com Cc: edk2-devel@lists.01.org References: <20180411081117.232-1-qin.long@intel.com> From: Laszlo Ersek Message-ID: Date: Wed, 11 Apr 2018 10:33:52 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180411081117.232-1-qin.long@intel.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Wed, 11 Apr 2018 08:33:54 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Wed, 11 Apr 2018 08:33:54 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH] CryptoPkg/OpensslLib: Update OpenSSL version to 1.1.0h X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 11 Apr 2018 08:33:55 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Hello Qin, On 04/11/18 10:11, Long Qin wrote: > (https://bugzilla.tianocore.org/show_bug.cgi?id=927) > > Update OpenSSL version to 1.1.0h release (27-Mar-2018) to include the > fix for CVE-2018-0739 issue (Handling of crafted recursive ASN.1 > structures can cause a stack overflow and resulting denial of service, > Refer to https://www.openssl.org/news/secadv/20180327.txt for more > information). Thank you for addressing this BZ so quickly. However, I have a comment on the commit message: > > Please note "git pull" will not update the submodule repository. > use the following commend to make your existing submodule track this > update: > $ git submodule update -–recursive --remote The "--remote" option is wrong here. The git-submodule documentation says, --remote This option is only valid for the update command. Instead of using the superproject's recorded SHA-1 to update the submodule, use the status of the submodule's remote-tracking branch. [...] [...] Use this option to integrate changes from the upstream subproject with your submodule's current HEAD. [...] That is exactly what normal edk2 consumers should *not* do -- because they do not want to update their openssl submodule to the latest upstream OpenSSL release; instead they want to update their openssl submodule to the commit hash that you are recording in this patch. ... In fact I've now found the same issue in our documentation, "CryptoPkg/Library/OpensslLib/OpenSSL-HOWTO.txt". It also recommends "--remote". I suggest the following: please post two patches. * The first patch should fix the documentation. The "--remote" option should be moved from the "user" section to the "maintainer" section -- that is, drop the "--remote" option from its current place, and explain it separately, similarly to "process_files.pl" (which is also only for maintainers). The "--remote" option is correct for *you*, the CryptoPkg maintainer, because you are pulling the new OpenSSL release into edk2, for the rest of the edk2 users. But those users only want to consume the OpenSSL commit hash that you record for them, not the OpenSSL master branch. * The second patch should be this patch, but the commit message should not contain the "--remote" option. One more comment below: > > Cc: Ye Ting > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Long Qin > --- > CryptoPkg/Library/OpensslLib/openssl | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/CryptoPkg/Library/OpensslLib/openssl b/CryptoPkg/Library/OpensslLib/openssl > index b2758a2292..d4e4bd2a81 160000 > --- a/CryptoPkg/Library/OpensslLib/openssl > +++ b/CryptoPkg/Library/OpensslLib/openssl > @@ -1 +1 @@ > -Subproject commit b2758a2292aceda93e9f44c219b94fe21bb9a650 > +Subproject commit d4e4bd2a8163f355fa8a3884077eaec7adc75ff7 > I agree that this commit corresponds to the "OpenSSL_1_1_0h" tag, in the upstream OpenSSL release. Once you post v2, I'll make an effort to review and test it reasonably quickly. (I have a Secure Boot test from hard disk, and an HTTPS boot test, in mind.) Thanks! Laszlo