Re: [PATCH] CryptoPkg/OpensslLib: Update OpenSSL version to 1.1.0h

["Long, Qin" <qin.long@intel.com>]

Thank you so much about this clarification, Laszlo.
The submodule maintenance (commands for update / sync) looks a little  confused to me. 

Let me check more locally before the V2.


Best Regards & Thanks,
LONG, Qin


-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Wednesday, April 11, 2018 4:34 PM
To: Long, Qin <qin.long@intel.com>; Ye, Ting <ting.ye@intel.com>
Cc: edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH] CryptoPkg/OpensslLib: Update OpenSSL version to 1.1.0h

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 <ting.ye@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Long Qin <qin.long@intel.com>
> ---
>  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