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

[Laszlo Ersek <lersek@redhat.com>]

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