Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-24 Thread Long, Qin 
Laszlo, this is really cool. Thanks for the analysis and root-cause this. 


Best Regards & Thanks,
LONG, Qin

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Thursday, February 25, 2016 3:39 AM
> To: David Woodhouse; Long, Qin
> Cc: Ye, Ting; edk2-de...@ml01.01.org; Bruce Cran
> Subject: Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version
> to 1.0.2f
> 
> On 02/24/16 20:30, David Woodhouse wrote:
> > On Wed, 2016-02-24 at 18:20 +0100, Laszlo Ersek wrote:
> >>
> >> Now, in the edk2 build, OPENSSL_free() boils down to a FreePool().
> >> However, *unlike* the free() function of the standard C library,
> >> FreePool() does *not* handle a NULL argument transparently.
> >
> > Well that's just utterly batshit insane, now isn't it?
> >
> > I'm amazed that didn't bite us before. If we're providing a free()
> > function especially for OpenSSL because the NIH principle guiding UEFI
> > was *so* strong that we even had to eschew even such *fundamentals*
> of
> > the C environment, then the least we can do is provide a *correct* one:
> >
> > diff --git
> > a/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c
> > b/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c
> > index 544f072..7c7818a 100644
> > --- a/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c
> > +++ b/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c
> > @@ -38,5 +38,6 @@ void *realloc (void *ptr, size_t size)
> >  /* De-allocates or frees a memory block */  void free (void *ptr)  {
> > -  FreePool (ptr);
> > +  if (ptr)
> > +FreePool (ptr);
> >  }
> >
> 
> I started composing my other email before yours arrived, and finished and
> sent it after yours arrived, it looks like :)
> 
> So yes, this is all. Can I take credit for the analysis, by submitting the 
> patch? :)
> 
> Thanks
> Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-24 Thread Laszlo Ersek 
On 02/24/16 20:30, David Woodhouse wrote:
> On Wed, 2016-02-24 at 18:20 +0100, Laszlo Ersek wrote:
>>
>> Now, in the edk2 build, OPENSSL_free() boils down to a FreePool().
>> However, *unlike* the free() function of the standard C library,
>> FreePool() does *not* handle a NULL argument transparently.
> 
> Well that's just utterly batshit insane, now isn't it?
> 
> I'm amazed that didn't bite us before. If we're providing a free()
> function especially for OpenSSL because the NIH principle guiding UEFI
> was *so* strong that we even had to eschew even such *fundamentals* of
> the C environment, then the least we can do is provide a *correct* one:
> 
> diff --git a/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c 
> b/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c
> index 544f072..7c7818a 100644
> --- a/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c
> +++ b/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c
> @@ -38,5 +38,6 @@ void *realloc (void *ptr, size_t size)
>  /* De-allocates or frees a memory block */
>  void free (void *ptr)
>  {
> -  FreePool (ptr);
> +  if (ptr)
> +FreePool (ptr);
>  }
> 

I started composing my other email before yours arrived, and finished
and sent it after yours arrived, it looks like :)

So yes, this is all. Can I take credit for the analysis, by submitting
the patch? :)

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-24 Thread Laszlo Ersek 
On 02/24/16 18:20, Laszlo Ersek wrote:
> On 02/24/16 18:12, Laszlo Ersek wrote:
> 
>>> #4  0x7fdf0917 in CRYPTO_free (str=0x0)
>>> at CryptoPkg/Library/OpensslLib/openssl-1.0.2f/crypto/mem.c:442
>>> #5  0x7fe20b47 in PKCS7_verify (p7=0x7ee6ff98, certs=0x0, 
>>> store=0x7ee62e58, indata=0x7ee62c18, out=0x0, 
>>> flags=128)
>>> at 
>>> CryptoPkg/Library/OpensslLib/openssl-1.0.2f/crypto/pkcs7/pk7_smime.c:415
> 
> These are the key stack frames. "pk7_smime.c:415" is an error handling
> section at the end of PKCS7_verify():
> 
>  err:
> OPENSSL_free(buf);
> if (tmpin == indata) {
> if (indata)
> BIO_pop(p7bio);
> }
> BIO_free_all(p7bio);
> sk_X509_free(signers);
> return ret;
> }
> 
> Now, in the edk2 build, OPENSSL_free() boils down to a FreePool().
> However, *unlike* the free() function of the standard C library,
> FreePool() does *not* handle a NULL argument transparently.
> 
> So we should look for jumps to the err label in PKCS7_verify() that
> happen before "buf" is set to anything different from NULL. (It is
> initialized to NULL at the top of the function.)

Okay, I found it. There is no error in your patches, just a minimal omission. :)

The above is actually the *entire error*. There is nothing else.

Namely, verifying the "shim.efi" binary takes *two iterations* of the loop(s) 
in IsAllowedByDb() 
[SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c].

More precisely, two calls to AuthenticodeVerify().

The first call is supposed to fail. The second call is supposed to succeed.

There is a bug in the free() function, in file 
"CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c". As I wrote above, 
unlike in standard C, this implementation of free() doesn't handle a NULL 
argument transparently -- it is passed to FreePool(), but FreePool() blows up 
on a NULL argument, in the assertion that I quoted earlier, in file 
"MdePkg/Library/UefiMemoryAllocationLib/MemoryAllocationLib.c":

VOID
EFIAPI
FreePool (
  IN VOID   *Buffer
  )
{
  EFI_STATUSStatus;

  Status = gBS->FreePool (Buffer);
  ASSERT_EFI_ERROR (Status);
}

The difference is that this bug in free() -- in the 
"CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c" file -- is *only* 
exposed by David's patches. Namely, when the first call to AuthenticodeVerify() 
fails internally (as expected), we trigger the assertion before we could get 
back out to IsAllowedByDb(), for the second call. This is how:

IsAllowedByDb()
[SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c]
  //
  // first call in the nested loops, supposed to fail:
  //
  AuthenticodeVerify() 
[CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c]
Pkcs7Verify()  
[CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c]
  PKCS7_verify()   
[CryptoPkg/Library/OpensslLib/openssl-1.0.2f/crypto/pkcs7/pk7_smime.c]
//
// we jump to the "err" label with buf == NULL
//
OPENSSL_free(buf)
  ...
free() 
[CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c]
  FreePool()   
[MdePkg/Library/UefiMemoryAllocationLib/MemoryAllocationLib.c]
ASSERT_EFI_ERROR()
  //
  // second call in the nested loops, supposed to succeed -- never reached 
because FreePool() never returns!
  //
  AuthenticodeVerify()   
[CryptoPkg/Library/BaseCryptLib/Pk/CryptAuthenticode.c]

The bug is exposed by the following backport in David's work:

commit e890487ff1ecfaa27d3f035861fba0c5912b8b4b
Author: Rich Salz 
Date:   Fri Sep 4 08:13:19 2015 -0400

RT3955: Reduce some stack usage

Use malloc/free instead of big onstack buffers.

Reviewed-by: Tim Hudson 
(cherry picked from commit 8e704858f21983383be2b77e986f475b51719a1e)

It adds an "OPENSSL_free(buf)" call to the error handling / final part of the 
PKCS7_verify() function (among other things). This function call can be reached 
with (buf == NULL). Which is fully valid, in standard C, but it exposes the bug 
in CryptoPkg's free() implementation.

I will send a trivial patch for CryptoPkg in a moment.

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-24 Thread David Woodhouse 
On Wed, 2016-02-24 at 18:20 +0100, Laszlo Ersek wrote:
> 
> Now, in the edk2 build, OPENSSL_free() boils down to a FreePool().
> However, *unlike* the free() function of the standard C library,
> FreePool() does *not* handle a NULL argument transparently.

Well that's just utterly batshit insane, now isn't it?

I'm amazed that didn't bite us before. If we're providing a free()
function especially for OpenSSL because the NIH principle guiding UEFI
was *so* strong that we even had to eschew even such *fundamentals* of
the C environment, then the least we can do is provide a *correct* one:

diff --git a/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c 
b/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c
index 544f072..7c7818a 100644
--- a/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c
+++ b/CryptoPkg/Library/BaseCryptLib/SysCall/BaseMemAllocation.c
@@ -38,5 +38,6 @@ void *realloc (void *ptr, size_t size)
 /* De-allocates or frees a memory block */
 void free (void *ptr)
 {
-  FreePool (ptr);
+  if (ptr)
+FreePool (ptr);
 }

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-24 Thread Laszlo Ersek 
On 02/24/16 17:26, David Woodhouse wrote:
> On Wed, 2016-02-24 at 15:46 +0100, Laszlo Ersek wrote:

>> Interestingly, the failure reproduces even when I build OVMF at your
>> commit a35e4359d; with identical symptoms.
> 
> Also useful to know; thanks.

The first commit that breaks it is:

commit 6a48e82abc5174e4810e388fdf0fc67564dabb03
Author: David Woodhouse 
Date:   Wed Aug 12 12:54:28 2015 +0100

CryptoPkg/OpensslLib: Update OpenSSL patch

Thanks
Laszlo

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-24 Thread David Woodhouse 
On Wed, 2016-02-24 at 15:46 +0100, Laszlo Ersek wrote:
> On 02/24/16 13:05, David Woodhouse wrote:
> > On Tue, 2016-02-23 at 21:57 +0100, Laszlo Ersek wrote:
> 
> > > First of all, I built it for:
> > > - OvmfPkg/OvmfIa32.dsc
> > > - OvmfPkg/OvmfIa32X64.dsc
> > > - OvmfPkg/OvmfX64.dsc
> > > - ArmVirtPkg/ArmVirtQemu.dsc (AARCH64 architecture)
> > > 
> > > The builds complete for the first three DSC files, but it fails
> > > for the last one:
> > > 
> > > > .../Build/ArmVirtQemu-
> > > > AARCH64/DEBUG_GCC48/AARCH64/CryptoPkg/Library/OpensslLib/Openss
> > > > lLib/OUTPUT/OpensslLib.lib(poly1305.obj): In function
> > > > `poly1305_blocks':
> > > > .../CryptoPkg/Library/OpensslLib/openssl/crypto/poly1305/poly13
> > > > 05.c:194: undefined reference to `__multi3'
> > > > .../CryptoPkg/Library/OpensslLib/openssl/crypto/poly1305/poly13
> > > > 05.c:195: undefined reference to `__multi3'
> > > > .../CryptoPkg/Library/OpensslLib/openssl/crypto/poly1305/poly13
> > > > 05.c:196: undefined reference to `__multi3'
> > > > .../CryptoPkg/Library/OpensslLib/openssl/crypto/poly1305/poly13
> > > > 05.c:197: undefined reference to `__multi3'
> > 
> > That's easily fixed by adding no-poly1305 to my process_files.sh
> > script. I've pushed a new version to my edk2.git tree with that
> > change.
> 
> Yes, with your master branch @ 55eb3465c077, the ArmVirtQemu build
> completes as well. Thanks.

Excellent; thanks for confirming that.

> (By the way, do you think that your series should revert, or at least
> customize, the following commit:
> 
> commit ffbb1b25402c38d25cbbfb059139e76900bdc843
> Author: Ard Biesheuvel 
> Date:   Tue Jun 16 15:09:19 2015 +
> 
> CryptoPkg: add .gitignore for OpenSSL source files

Yes, it should. Thanks for spotting that. I'll roll that into the next
series when I respin it... hopefully after finding out why it doesn't
*work*.

In fact they might be related. If you're using the headers which were
previously copied into CryptoPkg/Include/openssl because that's on the
include path before CryptoPkg/Library/OpensslLib/openssl-1.0.2f/include
then that might explain some weird breakage.

Perhaps my updated Install.sh should *delete* those stale copied
headers if they exist.

> Interestingly, the failure reproduces even when I build OVMF at your
> commit a35e4359d; with identical symptoms.

Also useful to know; thanks.

> Please find the repro steps below. (Sorry that it took so long to write
> them up, but generally I use libvirt, and I had to create these
> instructions on the spot, and I also tested them.)

I'm happy enough using libvirt, FWIW. I'm running on Fedora 23 — which
means I have a reasonably recent qemu. And if it wasn't for the poxy
FAT driver and its broken licence, I assume I'd even have a fully
functional OVMF package supplied as part of the distribution, Secure
Boot and all. :(

I'll start working through your instructions. Thanks!

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-24 Thread David Woodhouse 
On Tue, 2016-02-23 at 21:57 +0100, Laszlo Ersek wrote:
> 
> I'm testing David's patches from his repo referenced above, master branch, 
> commits
> 
>  1  81009e3cff24 CryptoPkg: Use OpenSSL include directory directly
>  2  8a40ff734a1e CryptoPkg/OpensslLib: Include complete copy of 
> opensslconf.h
>  3  d8b5c31bed60 CryptoPkg/OpensslLib: Update OpenSSL patch
>  4  b68dc8e0bb53 CryptoPkg/OpensslLib: Automatically configure OpenSSL 
> and generate file list
>  5  61e047fb19dd CryptoPkg: Support building with OpenSSL HEAD 
> (1.1.0-devel)
>  6  1e89cb2399ba CryptoPkg: Abuse internal headers to make OpenSSL HEAD 
> build work
> 
> First of all, I built it for:
> - OvmfPkg/OvmfIa32.dsc
> - OvmfPkg/OvmfIa32X64.dsc
> - OvmfPkg/OvmfX64.dsc
> - ArmVirtPkg/ArmVirtQemu.dsc (AARCH64 architecture)
> 
> The builds complete for the first three DSC files, but it fails for the last 
> one:
> 
> > .../Build/ArmVirtQemu-AARCH64/DEBUG_GCC48/AARCH64/CryptoPkg/Library/OpensslLib/OpensslLib/OUTPUT/OpensslLib.lib(poly1305.obj):
> >  In function `poly1305_blocks':
> > .../CryptoPkg/Library/OpensslLib/openssl/crypto/poly1305/poly1305.c:194: 
> > undefined reference to `__multi3'
> > .../CryptoPkg/Library/OpensslLib/openssl/crypto/poly1305/poly1305.c:195: 
> > undefined reference to `__multi3'
> > .../CryptoPkg/Library/OpensslLib/openssl/crypto/poly1305/poly1305.c:196: 
> > undefined reference to `__multi3'
> > .../CryptoPkg/Library/OpensslLib/openssl/crypto/poly1305/poly1305.c:197: 
> > undefined reference to `__multi3'

That's easily fixed by adding no-poly1305 to my process_files.sh
script. I've pushed a new version to my edk2.git tree with that change.

I've also updated the 1.0.2f-based patch, now that the PKCS7_verify()
regression has actually been fixed upstream (for 1.0.2g).

We have now merged *everything* from our EDKII_openssl-1.0.2f patch
into upstream OpenSSL HEAD, and our own patch can be *entirely*
represented as backports of existing commits from 1.1.

> Anyway, for runtime testing, I used the OvmfIa32X64 build:
> 
> > (1a) Enroll keys, and confirm SB being active in the Fedora guest,
> >  using my current build.
> > (1b) Rebuild the firmware binary with your patches & instructions. Do
> >  not touch the VM's varstore.
> > (1c) Confirm SB is still active in the Fedora guest.
> 
> This step failed, with the OVMF debug output ending with:
> 
> > Booting Fedora
> > FSOpen: Open '\EFI\fedora\shim.efi' Success
> > 
> > ASSERT_EFI_ERROR (Status = Invalid Parameter)
> > ASSERT 
> > .../MdePkg/Library/UefiMemoryAllocationLib/MemoryAllocationLib.c(819): 
> > !EFI_ERROR (Status)
> 
> I didn't continue testing after this point.

OK, thanks very much for testing this.

It sounds like there's a new issue in OpenSSL HEAD that needs fixing,
and I'm going to need to reproduce that myself to see what's going on.

Would you mind talking me through the setup above, please? To enroll
keys, I assume I need to start with a version of qemu that can support
running OVMF from a writeable flash chip, so it can store NV vars? 

Also, if you could test just the antepenultimate commit a35e4359d in
what I've just pushed — which is all the build system improvements but
still using OpenSSL 1.0.2f — that would also be very much appreciated.

Thanks again!

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-23 Thread Laszlo Ersek 
On 02/19/16 10:43, Laszlo Ersek wrote:
> On 02/19/16 09:59, David Woodhouse wrote:
>> On Fri, 2016-02-19 at 07:59 +, Long, Qin wrote:
>>> I agree those changes really make sense for better alignment, under
>>> both 1.0.2xx and 1.1 HEAD. The final out-of-box support will be
>>> wonderful. 
>>> The updates (http://git.infradead.org/users/dwmw2/edk2.git) looks
>>> good to me. And I will follow more validations, and start the next
>>> integration step-by-step. 
>>
>> Great, thanks. I should note that, as before, I haven't actually done
>> any real *testing* of this lot. Only build testing under Linux. I think
>> I did manage to create a Windows VM with a build environment for EDK2
>> at one point; I should at least dust that off.
> 
> I can test this for you, if you give me precise instructions.
> 
> (I'm asking for instructions because CryptoPkg/Include/openssl/README is
> deleted in one of the early patches.)

I'm testing David's patches from his repo referenced above, master branch, 
commits

 1  81009e3cff24 CryptoPkg: Use OpenSSL include directory directly
 2  8a40ff734a1e CryptoPkg/OpensslLib: Include complete copy of 
opensslconf.h
 3  d8b5c31bed60 CryptoPkg/OpensslLib: Update OpenSSL patch
 4  b68dc8e0bb53 CryptoPkg/OpensslLib: Automatically configure OpenSSL and 
generate file list
 5  61e047fb19dd CryptoPkg: Support building with OpenSSL HEAD (1.1.0-devel)
 6  1e89cb2399ba CryptoPkg: Abuse internal headers to make OpenSSL HEAD 
build work

First of all, I built it for:
- OvmfPkg/OvmfIa32.dsc
- OvmfPkg/OvmfIa32X64.dsc
- OvmfPkg/OvmfX64.dsc
- ArmVirtPkg/ArmVirtQemu.dsc (AARCH64 architecture)

The builds complete for the first three DSC files, but it fails for the last 
one:

> .../Build/ArmVirtQemu-AARCH64/DEBUG_GCC48/AARCH64/CryptoPkg/Library/OpensslLib/OpensslLib/OUTPUT/OpensslLib.lib(poly1305.obj):
>  In function `poly1305_blocks':
> .../CryptoPkg/Library/OpensslLib/openssl/crypto/poly1305/poly1305.c:194: 
> undefined reference to `__multi3'
> .../CryptoPkg/Library/OpensslLib/openssl/crypto/poly1305/poly1305.c:195: 
> undefined reference to `__multi3'
> .../CryptoPkg/Library/OpensslLib/openssl/crypto/poly1305/poly1305.c:196: 
> undefined reference to `__multi3'
> .../CryptoPkg/Library/OpensslLib/openssl/crypto/poly1305/poly1305.c:197: 
> undefined reference to `__multi3'

Anyway, for runtime testing, I used the OvmfIa32X64 build:

> (1a) Enroll keys, and confirm SB being active in the Fedora guest,
>  using my current build.
> (1b) Rebuild the firmware binary with your patches & instructions. Do
>  not touch the VM's varstore.
> (1c) Confirm SB is still active in the Fedora guest.

This step failed, with the OVMF debug output ending with:

> Booting Fedora
> FSOpen: Open '\EFI\fedora\shim.efi' Success
> 
> ASSERT_EFI_ERROR (Status = Invalid Parameter)
> ASSERT .../MdePkg/Library/UefiMemoryAllocationLib/MemoryAllocationLib.c(819): 
> !EFI_ERROR (Status)

I didn't continue testing after this point.


Just to be sure, I tested the same with edk2 upstream @ 
5458faf845a8c0e2ee37499ad410bb8ba1d45b15 ("MdePkg: BaseLib: fix AArch64 DAIF 
interrupt mask definitions").

At that revision, all four DSCs build fine, and runtime step (1c) succeeds as 
well.

Thanks
Laszlo

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-22 Thread Laszlo Ersek 
David,

On 02/19/16 21:24, David Woodhouse wrote:
> On Fri, 2016-02-19 at 14:26 +0100, Laszlo Ersek wrote:
>>
>> In file included from
>> .../CryptoPkg/Library/OpensslLib/openssl/crypto/bn/bn_add.c:59:0:
>> .../CryptoPkg/Library/OpensslLib/openssl/crypto/bn/bn_lcl.h:114:31:
>> fatal error: internal/bn_conf.h: No such file or directory
>>  # include "internal/bn_conf.h"
>>^
> 
> Hm, that's a new autogenerated file. Screw it, please run
> ./process_files.sh in the OpensslLib directory and I believe that'll be
> created for you. I'll work out the alternative build procedure with a
> *clean* tree in the meantime...

I haven't forgotten about this; I just need some time to get back to it.
If you got updates for the series meanwhile, please feel free (from my
side anyway) to submit them / push them.

Thanks
Laszlo

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-19 Thread David Woodhouse 
On Fri, 2016-02-19 at 14:26 +0100, Laszlo Ersek wrote:
> 
> In file included from
> .../CryptoPkg/Library/OpensslLib/openssl/crypto/bn/bn_add.c:59:0:
> .../CryptoPkg/Library/OpensslLib/openssl/crypto/bn/bn_lcl.h:114:31:
> fatal error: internal/bn_conf.h: No such file or directory
>  # include "internal/bn_conf.h"
>    ^

Hm, that's a new autogenerated file. Screw it, please run
./process_files.sh in the OpensslLib directory and I believe that'll be
created for you. I'll work out the alternative build procedure with a
*clean* tree in the meantime...

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-19 Thread Laszlo Ersek 
On 02/19/16 10:48, David Woodhouse wrote:
> On Fri, 2016-02-19 at 10:43 +0100, Laszlo Ersek wrote:
>>
>> I can test this for you, if you give me precise instructions.
>>
>> (I'm asking for instructions because CryptoPkg/Include/openssl/README is
>> deleted in one of the early patches.)
> 
> It moved from Patch-HOWTO.txt to OpenSSL-HOWTO.txt since there was no
> longer a patch involved. Or did I forget to add the new file?

No, the file is there alright.

So, I started executing my test plan. (If you hadn't snipped it, I could
now refer to it more easily. ;))

Using your master branch at c46b545e4193fc9fbe7807ff17883b44a1bf8305
("CryptoPkg: Abuse internal headers to make OpenSSL HEAD build work"),
and cloning/using your openssl repo at
f2bfbfd71e57e3802863ef5c2332fc6217c99f64 ("RT4175: Fix regression using
PKCS7_verify() with Authenticode"), I get the following build failure in
step (1b), building for IA32:

In file included from
.../CryptoPkg/Library/OpensslLib/openssl/crypto/bn/bn_add.c:59:0:
.../CryptoPkg/Library/OpensslLib/openssl/crypto/bn/bn_lcl.h:114:31:
fatal error: internal/bn_conf.h: No such file or directory
 # include "internal/bn_conf.h"
   ^

I get the same error for the Ia32X64, and the X64 builds as well.

I also tried with ArmVirtPkg/ArmVirtQemu, for AARCH64: same build error.

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-19 Thread David Woodhouse 
On Fri, 2016-02-19 at 10:43 +0100, Laszlo Ersek wrote:
> 
> I can test this for you, if you give me precise instructions.
> 
> (I'm asking for instructions because CryptoPkg/Include/openssl/README is
> deleted in one of the early patches.)

It moved from Patch-HOWTO.txt to OpenSSL-HOWTO.txt since there was no
longer a patch involved. Or did I forget to add the new file?


 Introduction

  OpenSSL is a well-known open source implementation of SSL and TLS protocols.
The core library implements the basic cryptographic functions and provides 
various
utility functions. The OpenSSL library is widely used in variety of security
products development as base crypto provider. (See http://www.openssl.org/ for 
more
information on OpenSSL).
  UEFI (Unified Extensible Firmware Interface) is a specification detailing the
interfaces between OS and platform firmware. Several security features were
introduced (e.g. Authenticated Variable Service, Driver Signing, etc) from UEFI
2.2 (http://www.uefi.org/). These security features highly depend on the
cryptography. This HOWTO documents OpenSSL building under UEFI environment.



OpenSSL-Version

  EDKII supports building with the master branch of OpenSSL, which will
  eventually be released as OpenSSL 1.1.0. In additional, fixes for the
  following OpenSSL "Request Tracker" tickets are required:

   RT4175: Fix regression using PKCS7_verify() with Authenticode
   RT4309: Define PRIu64 for UEFI build

  A clone of the OpenSSL git repository with all the required fixes is
  available at git://git.infradead.org/users/dwmw2/openssl.git



  HOW to Install Openssl for UEFI Building


1. Clone the above-referenced git repository into the directory
  CryptoPkg/Library/OpensslLib/openssl/

2. Copy the opensslconf.h file from this directory into the newly-created
   CryptoPkg/Library/OpensslLib/openssl/include/openssl directory next to
   the other OpenSSL include files.

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-19 Thread Laszlo Ersek 
On 02/19/16 09:59, David Woodhouse wrote:
> On Fri, 2016-02-19 at 07:59 +, Long, Qin wrote:
>> I agree those changes really make sense for better alignment, under
>> both 1.0.2xx and 1.1 HEAD. The final out-of-box support will be
>> wonderful. 
>> The updates (http://git.infradead.org/users/dwmw2/edk2.git) looks
>> good to me. And I will follow more validations, and start the next
>> integration step-by-step. 
> 
> Great, thanks. I should note that, as before, I haven't actually done
> any real *testing* of this lot. Only build testing under Linux. I think
> I did manage to create a Windows VM with a build environment for EDK2
> at one point; I should at least dust that off.

I can test this for you, if you give me precise instructions.

(I'm asking for instructions because CryptoPkg/Include/openssl/README is
deleted in one of the early patches.)

I'm thinking of the following tests, with OVMF:

(1a) Enroll keys, and confirm SB being active in the Fedora guest,
 using my current build.
(1b) Rebuild the firmware binary with your patches & instructions. Do
 not touch the VM's varstore.
(1c) Confirm SB is still active in the Fedora guest.
(1d) Confirm an unsigned UEFI shell binary from removable media cannot
 be launched.

(2a) Enroll keys, and confirm SB being active in the Fedora guest,
 using your build from the outset.
(2b) Confirm an unsigned UEFI shell binary from removable media cannot
 be launched.

Sound good?

Thanks
Laszlo

> 
>> Yeah, also will do more follow-ups about the remaining opens...
> 
> Like using the OpenSSL TS support instead of our own? That would be
> good. Likewise I think I remember a vague plan of making it possible to
> disable the OCSP code, since we don't use it?
> 
> Note that the OpenSSL 1.1 Beta 1 release is scheduled in "about a
> month"¹, and that is the feature/API freeze deadline. Anything we want
> added (other than bug fixes, of course), needs to be in by then.
> 
> In the fullness of time, I would *also* like to clean up the litter of
> include files we provide in CryptoPkg/Include purely for the benefit of
> OpenSSL — removing stuff from OpenSslSupport.h as we go. If we really
> want to stop OpenSSL from including/requiring those headers then that
> makes sense to do before Beta 1 too.
> 
> I might start by looking for things which can be considered "obviously"
> bugs — like including anything in netinet/ when configured with no-sock 
> for example. And I really don't see why it needs syslog.h for the UEFI
> build, or dirent.h for a no-stdio (which really means no file access)
> build.
> 

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-19 Thread David Woodhouse 
On Fri, 2016-02-19 at 09:19 +, Long, Qin wrote:
> 
> //nod
> Some old include / structure definitions were put there just to satisfy the 
> compiler in early phase, which looks like one rough style to make it work. 
> It's
> really the time to have one clean-up for our "out-of-the-box" integration.

Yeah, I might actually throw the whole lot away and start again, adding
only what's needed (and only then when I can't make OpenSSL *stop*
needing it)...

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-19 Thread Long, Qin 

> -Original Message-
> From: David Woodhouse [mailto:dw...@infradead.org]
> Sent: Friday, February 19, 2016 5:00 PM
> To: Long, Qin; Laszlo Ersek
> Cc: Ye, Ting; edk2-de...@ml01.01.org
> Subject: Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version
> to 1.0.2f
> 
> On Fri, 2016-02-19 at 07:59 +, Long, Qin wrote:
> > I agree those changes really make sense for better alignment, under
> > both 1.0.2xx and 1.1 HEAD. The final out-of-box support will be
> > wonderful.
> > The updates (http://git.infradead.org/users/dwmw2/edk2.git) looks
> > good to me. And I will follow more validations, and start the next
> > integration step-by-step.
> 
> Great, thanks. I should note that, as before, I haven't actually done
> any real *testing* of this lot. Only build testing under Linux. I think
> I did manage to create a Windows VM with a build environment for EDK2
> at one point; I should at least dust that off.
> 
> > Yeah, also will do more follow-ups about the remaining opens...
> 
> Like using the OpenSSL TS support instead of our own? That would be
> good. Likewise I think I remember a vague plan of making it possible to
> disable the OCSP code, since we don't use it?
> 
> Note that the OpenSSL 1.1 Beta 1 release is scheduled in "about a
> month"¹, and that is the feature/API freeze deadline. Anything we want
> added (other than bug fixes, of course), needs to be in by then.

Right. Just re-pickup those tasks. So will hurry up. 

> 
> In the fullness of time, I would *also* like to clean up the litter of
> include files we provide in CryptoPkg/Include purely for the benefit of
> OpenSSL — removing stuff from OpenSslSupport.h as we go. If we really
> want to stop OpenSSL from including/requiring those headers then that
> makes sense to do before Beta 1 too.
> 
> I might start by looking for things which can be considered "obviously"
> bugs — like including anything in netinet/ when configured with no-sock
> for example. And I really don't see why it needs syslog.h for the UEFI
> build, or dirent.h for a no-stdio (which really means no file access)
> build.
> 

//nod
Some old include / structure definitions were put there just to satisfy the 
compiler in early phase, which looks like one rough style to make it work. It's
really the time to have one clean-up for our "out-of-the-box" integration. 

> --
> David WoodhouseOpen Source Technology Centre
> david.woodho...@intel.com  Intel Corporation
> 
> 
> ¹ https://www.mail-archive.com/openssl-dev@openssl.org/msg42723.html

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-19 Thread David Woodhouse 
On Fri, 2016-02-19 at 07:59 +, Long, Qin wrote:
> I agree those changes really make sense for better alignment, under
> both 1.0.2xx and 1.1 HEAD. The final out-of-box support will be
> wonderful. 
> The updates (http://git.infradead.org/users/dwmw2/edk2.git) looks
> good to me. And I will follow more validations, and start the next
> integration step-by-step. 

Great, thanks. I should note that, as before, I haven't actually done
any real *testing* of this lot. Only build testing under Linux. I think
I did manage to create a Windows VM with a build environment for EDK2
at one point; I should at least dust that off.

> Yeah, also will do more follow-ups about the remaining opens...

Like using the OpenSSL TS support instead of our own? That would be
good. Likewise I think I remember a vague plan of making it possible to
disable the OCSP code, since we don't use it?

Note that the OpenSSL 1.1 Beta 1 release is scheduled in "about a
month"¹, and that is the feature/API freeze deadline. Anything we want
added (other than bug fixes, of course), needs to be in by then.

In the fullness of time, I would *also* like to clean up the litter of
include files we provide in CryptoPkg/Include purely for the benefit of
OpenSSL — removing stuff from OpenSslSupport.h as we go. If we really
want to stop OpenSSL from including/requiring those headers then that
makes sense to do before Beta 1 too.

I might start by looking for things which can be considered "obviously"
bugs — like including anything in netinet/ when configured with no-sock 
for example. And I really don't see why it needs syslog.h for the UEFI
build, or dirent.h for a no-stdio (which really means no file access)
build.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation


¹ https://www.mail-archive.com/openssl-dev@openssl.org/msg42723.html



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-18 Thread Long, Qin 
I agree those changes really make sense for better alignment, under both 
1.0.2xx and 1.1 HEAD. The final out-of-box support will be wonderful. 
The updates (http://git.infradead.org/users/dwmw2/edk2.git) looks good to me. 
And I will follow more validations, and start the next integration 
step-by-step. 

Yeah, also will do more follow-ups about the remaining opens...


Best Regards & Thanks,
LONG, Qin

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> David Woodhouse
> Sent: Friday, February 19, 2016 7:01 AM
> To: Long, Qin; Laszlo Ersek
> Cc: Ye, Ting; edk2-de...@ml01.01.org
> Subject: Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version
> to 1.0.2f
> 
> On Thu, 2016-02-18 at 17:36 +, Long, Qin wrote:
> > I think I also need to apologize, that it's my decision to pending
> > some part of Dave's patch series posted in last year (with simple
> > sync-up with Dave), which includes some changes for include path,
> > build process, etc.
> 
> I agreed with that then, and in retrospect it looks like the right decision. 
> It
> turns out that from 1.0.2e onwards, the symlinks are no longer present in the
> include/openssl/ directory of the release tarballs. So if you had actually
> merged my patch to use the OpenSSL include directory, back in the 1.0.2d
> days, it would have broken with the update to 1.0.2e.
> 
> I've revamped that patch so that we retain Install.sh even on POSIX
> platforms. Basically we just copy the files to where OpenSSL normally has
> them, instead of copying them to our *own* CryptoPkg/Include/ directory.
> 
> I've also rebuilt my OpenSSL_1_0_2-stable branch at
> http://git.infradead.org/users/dwmw2/openssl.git with freshly cherry-
> picked patches from HEAD... now that fairly much every change we had
> *is* committed to OpenSSL HEAD.
> 
> My main motivation for doing this right now is because we need to ensure
> that OpenSSL 1.1, when it comes out, *does* do everything we need out of
> the box.
> 
> I won't send another patchbomb to the list right now, but I've updated the
> tree at http://git.infradead.org/users/dwmw2/edk2.git
> 
> Again, up to and including the 'Automatically configure OpenSSL and
> generate file list' patch are applicable even while we stick with 1.0.2. The 
> final
> two commits need more work (and I'm hoping you follow through on the
> discussion about the HMAC APIs), but are mostly useful for ensuring that
> OpenSSL HEAD *stays* working as it approaches release.
> 
> --
> David WoodhouseOpen Source Technology Centre
> david.woodho...@intel.com  Intel Corporation

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-18 Thread David Woodhouse 
On Thu, 2016-02-18 at 17:36 +, Long, Qin wrote:
> I think I also need to apologize, that it's my decision to pending
> some part of Dave's patch series posted in last year (with simple
> sync-up with Dave), which includes some changes for include path,
> build process, etc.

I agreed with that then, and in retrospect it looks like the right
decision. It turns out that from 1.0.2e onwards, the symlinks are no
longer present in the include/openssl/ directory of the release
tarballs. So if you had actually merged my patch to use the OpenSSL
include directory, back in the 1.0.2d days, it would have broken with
the update to 1.0.2e.

I've revamped that patch so that we retain Install.sh even on POSIX
platforms. Basically we just copy the files to where OpenSSL normally
has them, instead of copying them to our *own* CryptoPkg/Include/
directory.

I've also rebuilt my OpenSSL_1_0_2-stable branch at
http://git.infradead.org/users/dwmw2/openssl.git with freshly cherry-
picked patches from HEAD... now that fairly much every change we had
*is* committed to OpenSSL HEAD.

My main motivation for doing this right now is because we need to
ensure that OpenSSL 1.1, when it comes out, *does* do everything we
need out of the box.

I won't send another patchbomb to the list right now, but I've updated
the tree at http://git.infradead.org/users/dwmw2/edk2.git

Again, up to and including the 'Automatically configure OpenSSL and
generate file list' patch are applicable even while we stick with
1.0.2. The final two commits need more work (and I'm hoping you follow
through on the discussion about the HMAC APIs), but are mostly useful
for ensuring that OpenSSL HEAD *stays* working as it approaches
release.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-18 Thread Long, Qin 
Wow, I just noticed these were so many discussion against this, with so many 
tech scopes. :-), Thanks for them, Dave & Laszlo.
I think I also need to apologize, that it's my decision to pending some part of 
Dave's patch series posted in last year (with simple sync-up with Dave), which 
includes some changes for include path, build process, etc. My original plan is 
to have one overall updates based on 1.1 formal release (which should be 
scheduled at the end of April, 2016), since we still cannot remove all patches 
under 1.0.x branch. 

Looks it is really better and valuable to bring them earlier. I may fasten this 
validation and integration. 
This simple 1.0.2f upgrade may still be handled firstly to align some 
requirement, after double-checking those hunk issues.  

Thanks again for Dave to bridge EDKII and OpenSSL dev team to make all those 
RTs happened.  And leave me in the rooms. :-)


Best Regards & Thanks,
LONG, Qin

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
> Ersek
> Sent: Friday, February 19, 2016 1:08 AM
> To: David Woodhouse <dw...@infradead.org>; Long, Qin <qin.l...@intel.com>
> Cc: Ye, Ting <ting...@intel.com>; edk2-de...@ml01.01.org
> Subject: Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 
> 1.0.2f
> 
> On 02/18/16 16:45, David Woodhouse wrote:
> > On Thu, 2016-02-18 at 14:27 +, David Woodhouse wrote:
> >> On Thu, 2016-02-18 at 14:58 +0100, Laszlo Ersek wrote:
> >>>
> >>> Then I gave my R-b to this patch, admitting that I couldn't verify the
> >>> edk2-only customizations against the 1.0.2f release.
> >>>
> >>> Turns out those customizations are indeed no longer correct, so my R-b
> >>> was in error.
> >>
> >> Er, aren't they? Are you referring to my comment about the RT#3955 fix?
> >>
> >> That isn't actually *less* correct than it ever was.
> >
> > And, embarrassingly, it looks like OpenSSL 1.1 has a fix which looks
> > much more like the one in EDKII_openssl-1.0.2f.patch, than the one in
> > my own tree.
> >
> > Is there room for me to come and crawl under that rock *with* you...?
> >
> 
> I pretty much made myself convenient and used up all the room here, in
> my shame. If you want to join me, you're going to have to transgress way
> harder than this! ;)
> 
> I'll be in my bunk ^W^W^W under my rock.
> 
> Laszlo
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-18 Thread Laszlo Ersek 
On 02/18/16 16:45, David Woodhouse wrote:
> On Thu, 2016-02-18 at 14:27 +, David Woodhouse wrote:
>> On Thu, 2016-02-18 at 14:58 +0100, Laszlo Ersek wrote:
>>>  
>>> Then I gave my R-b to this patch, admitting that I couldn't verify the
>>> edk2-only customizations against the 1.0.2f release.
>>>  
>>> Turns out those customizations are indeed no longer correct, so my R-b
>>> was in error.
>>
>> Er, aren't they? Are you referring to my comment about the RT#3955 fix?
>>
>> That isn't actually *less* correct than it ever was.
> 
> And, embarrassingly, it looks like OpenSSL 1.1 has a fix which looks
> much more like the one in EDKII_openssl-1.0.2f.patch, than the one in
> my own tree.
> 
> Is there room for me to come and crawl under that rock *with* you...?
> 

I pretty much made myself convenient and used up all the room here, in
my shame. If you want to join me, you're going to have to transgress way
harder than this! ;)

I'll be in my bunk ^W^W^W under my rock.

Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-18 Thread Laszlo Ersek 
On 02/18/16 15:27, David Woodhouse wrote:
> On Thu, 2016-02-18 at 14:58 +0100, Laszlo Ersek wrote:
>>
>> Then I gave my R-b to this patch, admitting that I couldn't verify the
>> edk2-only customizations against the 1.0.2f release.
>>
>> Turns out those customizations are indeed no longer correct, so my R-b
>> was in error.
> 
> Er, aren't they? Are you referring to my comment about the RT#3955 fix?

Yes, I am.

> That isn't actually *less* correct than it ever was.

Sigh. You just proved my point. I can't even participate in a discussion
about this module without getting confused / misunderstanding comments.

Honestly, I'm not following OpenSSL development, and I don't think I'll
find the cycles for it. I'll let you guys sort this out.

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-18 Thread David Woodhouse 
On Thu, 2016-02-18 at 14:27 +, David Woodhouse wrote:
> On Thu, 2016-02-18 at 14:58 +0100, Laszlo Ersek wrote:
> > 
> > Then I gave my R-b to this patch, admitting that I couldn't verify the
> > edk2-only customizations against the 1.0.2f release.
> > 
> > Turns out those customizations are indeed no longer correct, so my R-b
> > was in error.
> 
> Er, aren't they? Are you referring to my comment about the RT#3955 fix?
> 
> That isn't actually *less* correct than it ever was.

And, embarrassingly, it looks like OpenSSL 1.1 has a fix which looks
much more like the one in EDKII_openssl-1.0.2f.patch, than the one in
my own tree.

Is there room for me to come and crawl under that rock *with* you...?

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-18 Thread David Woodhouse 
On Thu, 2016-02-18 at 14:58 +0100, Laszlo Ersek wrote:
> 
> Then I gave my R-b to this patch, admitting that I couldn't verify the
> edk2-only customizations against the 1.0.2f release.
> 
> Turns out those customizations are indeed no longer correct, so my R-b
> was in error.

Er, aren't they? Are you referring to my comment about the RT#3955 fix?

That isn't actually *less* correct than it ever was.

-- 
dwmw2



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-18 Thread David Woodhouse 
On Thu, 2016-02-18 at 14:43 +0100, Laszlo Ersek wrote:
> 
> > Actually, in practice is *isn't* so bad. Given that we don't update our
> > EDKII_openssl-1.0.2X.patch very often (if at all) between OpenSSL
> > releases, what happens is that your build tree ends up with *multiple*
> > CryptoPkg/Library/OpensslLib/openssl-1.0.2[def] directories.
> 
> Interesting! The OPENSSL_PATH define in
> "CryptoPkg/Library/OpensslLib/OpensslLib.inf" would change, and the
> untracked files under the separate openssl-* directories would co-exist.
> I'm not sure about the untracked files created by Install.sh though --
> they come from the different openssl source trees, but go into a common
> location. Maybe Install.sh should be reexecuted at each step of the
> bisection.

Ah, good point. But I *killed* Install.sh, even for the 1.0.2 build,
many months ago. It temporarily escaped my notice that that fix wasn't
actually merged into EDK2 upstream yet. It's one of the set I sent
yesterday. Once that's done, *then* what I said is true because we
actually use the OpenSSL headers from within the OpenSSL source tree,
and don't stupidly *copy* them to our own Include/ directory.

> Do you think it will be possible to add openssl as a git submodule to
> the upstream edk2 repo? (I'd rather not explore that myself down-stream...)

I don't see why not. But let's concentrate on getting the even *more*
basic and obvious "you are completely insane if you aren't using git
properly" things fixed first... like using LF in the stored files.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-18 Thread Laszlo Ersek 
On 02/18/16 13:14, Laszlo Ersek wrote:

> Okay, I'm done ranting.
> 
> Reviewed-by: Laszlo Ersek 

So let me review how my comments have worked out for this posting.

First I nacked it, because I thought the fact was not sufficiently
appreciated that David had posted a conflicting / more comprehensive /
potentially more correct patch series.

Then I withdrew my NACK, after David said he was okay with this patch
going in first -- assuming it was correct.

Then I gave my R-b to this patch, admitting that I couldn't verify the
edk2-only customizations against the 1.0.2f release.

Turns out those customizations are indeed no longer correct, so my R-b
was in error.

Not exactly a spotless track record for me, in this thread. I think I
shall stay away from reviewing OpenSSL updates.

In parallel, I propose that David be added to the CryptoPkg maintainers
in "Maintainers.txt" (provided he wants it). Then he should get
automatically CC'd on such rebases.

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-18 Thread Laszlo Ersek 
On 02/18/16 14:19, David Woodhouse wrote:

> Doesn't git
> convert automatically if it finds LF in the files it checks out, on
> Windows?

No clue. Never used git on Windows.

> And if things do get checked out with LF-only line endings on
> Windows, isn't that *also* harmless?

I don't think so. I know NOTEPAD.EXE garbles (well, used to garble)
LF-only text files. Maybe other commonly used text editors do that as well.

Plus, I'm worried that *new* files would be created with CRLF, and that
would lead to inconsistency.

> Either way, such a change will be *very* quickly lost in the dim and
> distant past — rather than being a constant problem and a barrier to
> contributions.

I'm not the one in need of convincing! ;)

I didn't mean to "defend" the current status.

Laszlo

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-18 Thread Laszlo Ersek 
On 02/18/16 13:59, David Woodhouse wrote:

[snip ;)]

> Please trim citations. You didn't have to cite the *entire* patch just
> to say that it seems right!
> 
> You aren't one of the Outlook-afflicted. We need you to set an example
> of how email is actually *supposed* to be used :)

Some people think trimming is not helpful, others think it's (almost)
required. I do trim emails, at the latest when they get overlong -- but
when replying to a patch directly, I think I rarely do.

I guess in this case it would have been justified, yes.

>> I also tried to diff the "openssl-1.0.2e" and "openssl-1.0.2f" trees
>> against each other, to see if "no new source changes [...] introduced
>> for 1.0.2f enabling" is indeed the right thing to do.
>>
>> The result of that diffing is a 3000 line patch, with the following
>> diffstat:
>>
>>  144 files changed, 815 insertions(+), 476 deletions(-)
> 
> This is the 21st century. Nobody in their right mind deals with
> software management except through git. More edifying would be to run:
> 
>  git log OpenSSL_1_0_2e..OpenSSL_1_0_2f  --stat
> 
> and/or
> 
>  gitk OpenSSL_1_0_2e..OpenSSL_1_0_2f

Sure, but that requires one to clone the OpenSSL repo, recognize the tag
names (yes, not too hard :)), and it's not the view that will be
ultimately visible in edk2. (After embedding the exploded OpenSSL tree.)

I assume this would be more manageable if the upstream openssl repo was
a submodule for edk2.

[snip]

>> So, I hereby declare the OpenSSL updates practically un-review-able,
>> even just for *scope* -- i.e., in order to see how far those changes
>> extend. I also claim that the OpenSSL release strategy is not being
>> implemented in reality -- the "letter releases" actually seem to be
>> vulnerability-triggered *snapshots* of the 1.0.2 tree, where the code
>> influx, albeit low volume, definitely meanders outside of bug and
>> security fixes.
> 
> Yeah, there's a certain amount of truth in what you say. When a high-
> priority vulnerability comes up, we need a new release and people need
> to upgrade. If we fix a minor functionality issue which is fairly
> esoteric and *doesn't* have security implications, there *isn't* an
> immediate release the same day. We'd run out of letters very quickly if
> we did that! :)
> 
> So yes, when the high-visibility issues trigger a release, a bunch of
> less important fixes go along for the ride. I don't think that's a
> problem.
> 
> But trust me — as someone who has occasionally wanted to get
> improvements into a stable branch of OpenSSL when they have been
> considered "new features" — when I say that they aren't adding whole
> piles of exciting new stuff to the 1.0.2 branch :)

I can trust you alright. :)

[snip]

>> Now, one might ask why I care. I care because for some downstreams of
>> edk2 at least, the situation that openssl has to be patched in before a
>> secure boot enabled build is completely unacceptable. That makes it
>> super-unwieldy to bisect a secure boot enabled build for example. It
>> also requires all people who clone your tree to patch in OpenSSL
>> manually.
> 
> Actually, in practice is *isn't* so bad. Given that we don't update our
> EDKII_openssl-1.0.2X.patch very often (if at all) between OpenSSL
> releases, what happens is that your build tree ends up with *multiple*
> CryptoPkg/Library/OpensslLib/openssl-1.0.2[def] directories.

Interesting! The OPENSSL_PATH define in
"CryptoPkg/Library/OpensslLib/OpensslLib.inf" would change, and the
untracked files under the separate openssl-* directories would co-exist.
I'm not sure about the untracked files created by Install.sh though --
they come from the different openssl source trees, but go into a common
location. Maybe Install.sh should be reexecuted at each step of the
bisection.

> And as you
> flit back and forth between EDK2 commits in your bisect, you use the
> different OpenSSL directories, as appropriate.
> 
>> Importing openssl should be a run-of-the-mill *commit* in the git
>> history (and it is, for us).
> 
> Surely this is what git submodules are for?

Probably. :)

[snip]

>> Honestly, edk2 should either incorporate OpenSSL permanently, or build
>> it 100% from an external, unmodified upstream tarball (I think this is
>> what David has been working on, right?)
> 
> As noted yesterday, we're two trivial patches from being able to use
> the next OpenSSL 1.1.0 beta snapshot "out of the box" with EDK2.

Do you think it will be possible to add openssl as a git submodule to
the upstream edk2 repo? (I'd rather not explore that myself down-stream...)

Thanks!
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-18 Thread David Woodhouse 
On Thu, 2016-02-18 at 14:12 +0100, Laszlo Ersek wrote:
> 
> > And even if we've missed the chance to do it "in retrospect" for
> > historical commits in our canonical git repository, we could still make
> > a commit now which *changes* the line endings to what git expects, and
> > quite soon this whole nonsense would be a thing of the past.
> 
> Yes. I think that would take a commit that converted everything to
> LF-only. Then everybody on Windows would have to toggle some git knobs,
> correct? Would that be safe when moving between commits?

I'm not even sure if they'd have to toggle anything. Doesn't git
convert automatically if it finds LF in the files it checks out, on
Windows? And if things do get checked out with LF-only line endings on
Windows, isn't that *also* harmless?

I have fairly much reached my masochism quota for this week though; I'm
not about to attempt to boot a Windows machine and check :)

Either way, such a change will be *very* quickly lost in the dim and
distant past — rather than being a constant problem and a barrier to
contributions.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-18 Thread Laszlo Ersek 
On 02/18/16 13:59, David Woodhouse wrote:
> On Thu, 2016-02-18 at 13:40 +0100, Laszlo Ersek wrote:
>>> We should not be carrying patches which *differ* from the fixes that
>>> went into OpenSSL upstream.
>>
>> So yeah, I guess the recent irrelevance of this edk2-only hunk should
>> have been "obvious" from the 3000-line diff between OpenSSL 1.0.2e and
>> 1.0.2f -- a diff that no person not involved in day-to-day OpenSSL
>> development can review.
> 
> Actually, that isn't fixed in OpenSSL 1.0.2f; it's fixed only in 1.1
> and I've backported the fix.
> 
>>> Can we *please* keep native line endings in the git tree
>>
>> No, we can't. This issue has been discussed with Junio C Hamano, the git
>> maintainer. Git can convert text files to platform-native line endings
>> on checkout, but only if the internal (object) representation of the
>> text files is LF only.
>>
>> And, because edk2 started out with SVN, and the (now primary) git repo
>> started out as a mirror of the SVN repo, the internal representation is
>> CRLF, not LF. So git's auto-conversion doesn't apply.
> 
> Apologies, I misspoke. I didn't mean "native line endings in the git
> tree", which is obviously impossible — I meant "sane line endings in
> the git tree", which means LF — so git's auto-conversion *would* apply.
> 
>> Everyone else did not start with a git mirror of an SVN repo of text
>> files that were created and edited exclusively on Windows. :(
> 
> A poor excuse. It wasn't hard to fix up the SVN->git mirror, and we
> knew we needed to do this *long* before git actually became the
> "primary" system.
> 
> And even if we've missed the chance to do it "in retrospect" for
> historical commits in our canonical git repository, we could still make
> a commit now which *changes* the line endings to what git expects, and
> quite soon this whole nonsense would be a thing of the past.

Yes. I think that would take a commit that converted everything to
LF-only. Then everybody on Windows would have to toggle some git knobs,
correct? Would that be safe when moving between commits?

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-18 Thread David Woodhouse 
On Thu, 2016-02-18 at 13:14 +0100, Laszlo Ersek wrote:
> This patch seems right.

Please trim citations. You didn't have to cite the *entire* patch just
to say that it seems right!

You aren't one of the Outlook-afflicted. We need you to set an example
of how email is actually *supposed* to be used :)

> I also tried to diff the "openssl-1.0.2e" and "openssl-1.0.2f" trees
> against each other, to see if "no new source changes [...] introduced
> for 1.0.2f enabling" is indeed the right thing to do.
> 
> The result of that diffing is a 3000 line patch, with the following
> diffstat:
> 
>  144 files changed, 815 insertions(+), 476 deletions(-)

This is the 21st century. Nobody in their right mind deals with
software management except through git. More edifying would be to run:

 git log OpenSSL_1_0_2e..OpenSSL_1_0_2f  --stat

and/or

 gitk OpenSSL_1_0_2e..OpenSSL_1_0_2f

There are 60 commits. Browsing through them with gitk, they do mostly
seem like sane, simple bug fixes. Some are cosmetic but basically risk-
free, such as the example you gave — that commit touched 74 files, but
*only* in the comments, with fairly much zero chance of causing
problems.

> While I'm absolutely no crypto expert, I have a hard time believing
> that fixing the two major issues listed by the release notes required
> touching *144 files*!

There were plenty more issues fixed other than the "headline" issues.

> So, I hereby declare the OpenSSL updates practically un-review-able,
> even just for *scope* -- i.e., in order to see how far those changes
> extend. I also claim that the OpenSSL release strategy is not being
> implemented in reality -- the "letter releases" actually seem to be
> vulnerability-triggered *snapshots* of the 1.0.2 tree, where the code
> influx, albeit low volume, definitely meanders outside of bug and
> security fixes.

Yeah, there's a certain amount of truth in what you say. When a high-
priority vulnerability comes up, we need a new release and people need
to upgrade. If we fix a minor functionality issue which is fairly
esoteric and *doesn't* have security implications, there *isn't* an
immediate release the same day. We'd run out of letters very quickly if
we did that! :)

So yes, when the high-visibility issues trigger a release, a bunch of
less important fixes go along for the ride. I don't think that's a
problem.

But trust me — as someone who has occasionally wanted to get
improvements into a stable branch of OpenSSL when they have been
considered "new features" — when I say that they aren't adding whole
piles of exciting new stuff to the 1.0.2 branch :)

Take a look at the discussion about fixing ECDH for engines, on the
openssl-dev list at the moment. It would be fairly trivial to expose
what's necessary there, but because it's a new feature (although some
would want to consider a bug fix), the OpenSSL team really don't want
to do it.

> Now, one might ask why I care. I care because for some downstreams of
> edk2 at least, the situation that openssl has to be patched in before a
> secure boot enabled build is completely unacceptable. That makes it
> super-unwieldy to bisect a secure boot enabled build for example. It
> also requires all people who clone your tree to patch in OpenSSL
> manually.

Actually, in practice is *isn't* so bad. Given that we don't update our
EDKII_openssl-1.0.2X.patch very often (if at all) between OpenSSL
releases, what happens is that your build tree ends up with *multiple*
CryptoPkg/Library/OpensslLib/openssl-1.0.2[def] directories. And as you
flit back and forth between EDK2 commits in your bisect, you use the
different OpenSSL directories, as appropriate.

> Importing openssl should be a run-of-the-mill *commit* in the git
> history (and it is, for us).

Surely this is what git submodules are for?

> Then you can understand why I care about actual OpenSSL differences --
> because when the OpenSSL addition is an actual commit in your repo, the
> upgrade looks like this:
> 
> - revert the commit that captured the execution of the previous
>   Patch-HOWTO.txt
> - perform the current Patch-HOWTO.txt, commit the results as a new
>   commit
> - *squash* these two commits (unless you are happy with two, several MB
>   long patches -- good luck to your mailing list!)
> - now you have an incremental patch in your history that takes you from
>   1.0.2e to 1.0.2f
> 
> Except, of course, that patch is fully unreviewable -- it is
> no better than a binary-only code drop.

Again, not if it's done properly as a git submodule.

> Honestly, edk2 should either incorporate OpenSSL permanently, or build
> it 100% from an external, unmodified upstream tarball (I think this is
> what David has been working on, right?)

As noted yesterday, we're two trivial patches from being able to use
the next OpenSSL 1.1.0 beta snapshot "out of the box" with EDK2.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-18 Thread David Woodhouse 
On Thu, 2016-02-18 at 13:40 +0100, Laszlo Ersek wrote:
> > We should not be carrying patches which *differ* from the fixes that
> > went into OpenSSL upstream.
> 
> So yeah, I guess the recent irrelevance of this edk2-only hunk should
> have been "obvious" from the 3000-line diff between OpenSSL 1.0.2e and
> 1.0.2f -- a diff that no person not involved in day-to-day OpenSSL
> development can review.

Actually, that isn't fixed in OpenSSL 1.0.2f; it's fixed only in 1.1
and I've backported the fix.

> > Can we *please* keep native line endings in the git tree
>
> No, we can't. This issue has been discussed with Junio C Hamano, the git
> maintainer. Git can convert text files to platform-native line endings
> on checkout, but only if the internal (object) representation of the
> text files is LF only.
> 
> And, because edk2 started out with SVN, and the (now primary) git repo
> started out as a mirror of the SVN repo, the internal representation is
> CRLF, not LF. So git's auto-conversion doesn't apply.

Apologies, I misspoke. I didn't mean "native line endings in the git
tree", which is obviously impossible — I meant "sane line endings in
the git tree", which means LF — so git's auto-conversion *would* apply.

> Everyone else did not start with a git mirror of an SVN repo of text
> files that were created and edited exclusively on Windows. :(

A poor excuse. It wasn't hard to fix up the SVN->git mirror, and we
knew we needed to do this *long* before git actually became the
"primary" system.

And even if we've missed the chance to do it "in retrospect" for
historical commits in our canonical git repository, we could still make
a commit now which *changes* the line endings to what git expects, and
quite soon this whole nonsense would be a thing of the past.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-18 Thread Laszlo Ersek 
tangent:

On 02/18/16 13:40, Laszlo Ersek wrote:

> https://github.com/lersek/edk2/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#maint-07

> https://github.com/lersek/edk2/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-05

Sigh. Clearly I pasted the wrong link from my browser's history. The
correct link, into the tianocore wiki, is

https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers

Sorry about the noise.

Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-18 Thread Laszlo Ersek 
On 02/18/16 12:40, David Woodhouse wrote:
> On Thu, 2016-02-18 at 00:33 +0800, Qin Long wrote:
>>
>>  crypto/pkcs7/pk7_smime.c   Thu Jun 11 21:01:06 2015
>> -+++ crypto/pkcs7/pk7_smime.c   Fri Jun 12 11:23:38 2015
>> +--- crypto/pkcs7/pk7_smime.c   Thu Jan 28 21:56:08 2016
>>  crypto/pkcs7/pk7_smime.c   Wed Feb 17 16:22:45 2016
>>  @@ -254,7 +254,8 @@
>>   STACK_OF(PKCS7_SIGNER_INFO) *sinfos;
>>   PKCS7_SIGNER_INFO *si;
>> @@ -114,20 +114,19 @@ diff U3 crypto/pkcs7/pk7_smime.c
>> crypto/pkcs7/pk7_smime.c
>>   if (i <= 0)
>>   break;
>>   if (tmpout)
>> -@@ -394,6 +394,10 @@
>> +@@ -394,6 +394,9 @@
>>   }
>>   BIO_free_all(p7bio);
>>   sk_X509_free(signers);
>> -+
>>  +if (buf != NULL) {
>> -+  OPENSSL_free(buf);
>> ++OPENSSL_free(buf);
>>  +}
>>   return ret;
>>   }
>>  
> 
> This bit of code addresses OpenSSL RT#3955, although you don't actually
> *mention* that fact anywhere. A different fix has been committed to
> OpenSSL to close that RT.
> 
> We should not be carrying patches which *differ* from the fixes that
> went into OpenSSL upstream.

So yeah, I guess the recent irrelevance of this edk2-only hunk should
have been "obvious" from the 3000-line diff between OpenSSL 1.0.2e and
1.0.2f -- a diff that no person not involved in day-to-day OpenSSL
development can review.

> 
> That's why part of my patch series (qv) actually *replaces* this whole
> EDKII_openssl-1.0.2X.patch with a cleanly generated one from a 1.0.2-
> based git tree, *with* its full changelog:
> 
> http://git.infradead.org/users/dwmw2/edk2.git/commitdiff/cf8dd4aee409

Yes, it mentions

  RT3955: Reduce stack usage in PKCS7_verify() and PKCS7_decrypt()

> I mention this just to reinforce the need for that change, even before
> we make the switch to OpenSSL 1.1.
> 
> FWIW I was unable to apply the patch from your email; if there was ever
> a trick to managing the bogus line endings, I've forgotten it.

There are four tricks:
(a) use git-am with --keep-cr
(b) the above will not work if the patch email that you save from the
list uses plaintext content encoding, *and* creates or deletes files
-- in that case, /dev/null\r\n hunk headers will be present, and
those will trip up git. So, the second option is: patch git itself.
(c) as contributor, send the email with a non-plaintext content
encoding. Paolo got some patches for this into upstream git at some
point.
(d) As contributor, push your changes to a public repo, and include
the branch URL in the posting -- then reviewers can fetch the patch
intact.

See also

https://github.com/lersek/edk2/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#maint-07

(Note that the git-am command given there doesn't spell out --keep-cr --
because at that point it's been set permanently:
.)

> Can we
> *please* keep native line endings in the git tree

No, we can't. This issue has been discussed with Junio C Hamano, the git
maintainer. Git can convert text files to platform-native line endings
on checkout, but only if the internal (object) representation of the
text files is LF only.

And, because edk2 started out with SVN, and the (now primary) git repo
started out as a mirror of the SVN repo, the internal representation is
CRLF, not LF. So git's auto-conversion doesn't apply.

(This is also the reason for my

  core.whitespace = cr-at-eol

setting in

-- because otherwise git diff & friends will yell at you constantly, for
"\r" as line-trailing whitespace.)

> and let it be checked
> out into the native form — like everyone else does?

Everyone else did not start with a git mirror of an SVN repo of text
files that were created and edited exclusively on Windows. :(

Laszlo

> 
> 
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-18 Thread Laszlo Ersek 
On 02/17/16 17:33, Qin Long wrote:
> OpenSSL has released version 1.0.2f with two security fixes
> (http://www.openssl.org/news/secadv/20160128.txt) at 28-Jan-2016.
> Upgrade the supported OpenSSL version in CryptoPkg/OpensslLib
> to catch the latest release 1.0.2f.
> (NOTE: The patch file was just re-generated, and no new source
>changes was introduced for 1.0.2f enabling)
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Qin Long 
> CC: Ting Ye 
> ---
>  ...ssl-1.0.2e.patch => EDKII_openssl-1.0.2f.patch} | 63 
> +++---
>  CryptoPkg/Library/OpensslLib/Install.cmd   |  2 +-
>  CryptoPkg/Library/OpensslLib/Install.sh|  2 +-
>  CryptoPkg/Library/OpensslLib/OpensslLib.inf|  4 +-
>  CryptoPkg/Library/OpensslLib/Patch-HOWTO.txt   | 26 -
>  5 files changed, 48 insertions(+), 49 deletions(-)
>  rename CryptoPkg/Library/OpensslLib/{EDKII_openssl-1.0.2e.patch => 
> EDKII_openssl-1.0.2f.patch} (89%)
> 
> diff --git a/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2e.patch 
> b/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2f.patch
> similarity index 89%
> rename from CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2e.patch
> rename to CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2f.patch
> index e4eaff6..c42b776 100644
> --- a/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2e.patch
> +++ b/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2f.patch
> @@ -1,7 +1,7 @@
>  diff U3 crypto/bio/bio.h crypto/bio/bio.h
>  crypto/bio/bio.h Thu Jun 11 21:50:12 2015
> -+++ crypto/bio/bio.h Fri Jun 12 11:00:52 2015
> -@@ -646,10 +646,10 @@
> +--- crypto/bio/bio.h Thu Jan 28 21:56:08 2016
>  crypto/bio/bio.h Wed Feb 17 16:43:40 2016
> +@@ -650,10 +650,10 @@
>   int BIO_asn1_get_suffix(BIO *b, asn1_ps_func **psuffix,
>   asn1_ps_func **psuffix_free);
>   
> @@ -14,8 +14,8 @@ diff U3 crypto/bio/bio.h crypto/bio/bio.h
>   # endif
>   BIO *BIO_new(BIO_METHOD *type);
>  diff U3 crypto/bio/bss_file.c crypto/bio/bss_file.c
>  crypto/bio/bss_file.cThu Jun 11 21:01:06 2015
> -+++ crypto/bio/bss_file.cFri Jun 12 11:01:28 2015
> +--- crypto/bio/bss_file.cThu Jan 28 21:38:30 2016
>  crypto/bio/bss_file.cWed Feb 17 16:01:02 2016
>  @@ -467,6 +467,23 @@
>   return (ret);
>   }
> @@ -41,8 +41,8 @@ diff U3 crypto/bio/bss_file.c crypto/bio/bss_file.c
>   
>   #endif  /* HEADER_BSS_FILE_C */
>  diff U3 crypto/dh/dh_pmeth.c crypto/dh/dh_pmeth.c
>  crypto/dh/dh_pmeth.c Thu Jun 11 21:50:12 2015
> -+++ crypto/dh/dh_pmeth.c Fri Jun 12 11:08:48 2015
> +--- crypto/dh/dh_pmeth.c Thu Jan 28 21:56:08 2016
>  crypto/dh/dh_pmeth.c Wed Feb 17 16:15:58 2016
>  @@ -449,6 +449,9 @@
>   *keylen = ret;
>   return 1;
> @@ -62,8 +62,8 @@ diff U3 crypto/dh/dh_pmeth.c crypto/dh/dh_pmeth.c
>   return 1;
>   }
>  diff U3 crypto/pem/pem.h crypto/pem/pem.h
>  crypto/pem/pem.h Thu Jun 11 21:50:12 2015
> -+++ crypto/pem/pem.h Fri Jun 12 10:58:18 2015
> +--- crypto/pem/pem.h Thu Jan 28 21:56:08 2016
>  crypto/pem/pem.h Wed Feb 17 15:56:26 2016
>  @@ -324,6 +324,7 @@
>   
>   #  define DECLARE_PEM_read_fp(name, type) /**/
> @@ -73,8 +73,8 @@ diff U3 crypto/pem/pem.h crypto/pem/pem.h
>   # else
>   
>  diff U3 crypto/pkcs7/pk7_smime.c crypto/pkcs7/pk7_smime.c
>  crypto/pkcs7/pk7_smime.c Thu Jun 11 21:01:06 2015
> -+++ crypto/pkcs7/pk7_smime.c Fri Jun 12 11:23:38 2015
> +--- crypto/pkcs7/pk7_smime.c Thu Jan 28 21:56:08 2016
>  crypto/pkcs7/pk7_smime.c Wed Feb 17 16:22:45 2016
>  @@ -254,7 +254,8 @@
>   STACK_OF(PKCS7_SIGNER_INFO) *sinfos;
>   PKCS7_SIGNER_INFO *si;
> @@ -114,20 +114,19 @@ diff U3 crypto/pkcs7/pk7_smime.c 
> crypto/pkcs7/pk7_smime.c
>   if (i <= 0)
>   break;
>   if (tmpout)
> -@@ -394,6 +394,10 @@
> +@@ -394,6 +394,9 @@
>   }
>   BIO_free_all(p7bio);
>   sk_X509_free(signers);
> -+
>  +if (buf != NULL) {
> -+  OPENSSL_free(buf);
> ++OPENSSL_free(buf);
>  +}
>   return ret;
>   }
>   
>  diff U3 crypto/rand/rand_unix.c crypto/rand/rand_unix.c
>  crypto/rand/rand_unix.c  Thu Jun 11 21:01:06 2015
> -+++ crypto/rand/rand_unix.c  Fri Jun 12 10:51:21 2015
> +--- crypto/rand/rand_unix.c  Thu Jan 28 21:38:32 2016
>  crypto/rand/rand_unix.c  Wed Feb 17 15:40:02 2016
>  @@ -116,7 +116,7 @@
>   #include 
>   #include "rand_lcl.h"
> @@ -147,8 +146,8 @@ diff U3 crypto/rand/rand_unix.c crypto/rand/rand_unix.c
>   {
>   return 0;
>  diff U3 crypto/rsa/rsa_ameth.c crypto/rsa/rsa_ameth.c
>  crypto/rsa/rsa_ameth.c   Thu Jun 11 21:50:12 2015
> -+++ crypto/rsa/rsa_ameth.c   Fri Jun 12 10:45:38 2015
> +--- crypto/rsa/rsa_ameth.c   Thu Jan 28 21:56:08 2016
>  crypto/rsa/rsa_ameth.c   Wed Feb 17 15:09:46 2016
>  @@ -68,10 +68,12 @@
>   #endif
>   #include "asn1_locl.h"
> @@ -221,8 +220,8 @@ diff U3 crypto/rsa/rsa_ameth.c

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-18 Thread David Woodhouse 
On Thu, 2016-02-18 at 00:33 +0800, Qin Long wrote:
> 
>  crypto/pkcs7/pk7_smime.c   Thu Jun 11 21:01:06 2015
> -+++ crypto/pkcs7/pk7_smime.c   Fri Jun 12 11:23:38 2015
> +--- crypto/pkcs7/pk7_smime.c   Thu Jan 28 21:56:08 2016
>  crypto/pkcs7/pk7_smime.c   Wed Feb 17 16:22:45 2016
>  @@ -254,7 +254,8 @@
>   STACK_OF(PKCS7_SIGNER_INFO) *sinfos;
>   PKCS7_SIGNER_INFO *si;
> @@ -114,20 +114,19 @@ diff U3 crypto/pkcs7/pk7_smime.c
> crypto/pkcs7/pk7_smime.c
>   if (i <= 0)
>   break;
>   if (tmpout)
> -@@ -394,6 +394,10 @@
> +@@ -394,6 +394,9 @@
>   }
>   BIO_free_all(p7bio);
>   sk_X509_free(signers);
> -+
>  +    if (buf != NULL) {
> -+  OPENSSL_free(buf);
> ++    OPENSSL_free(buf);
>  +    }
>   return ret;
>   }
>  

This bit of code addresses OpenSSL RT#3955, although you don't actually
*mention* that fact anywhere. A different fix has been committed to
OpenSSL to close that RT.

We should not be carrying patches which *differ* from the fixes that
went into OpenSSL upstream.

That's why part of my patch series (qv) actually *replaces* this whole
EDKII_openssl-1.0.2X.patch with a cleanly generated one from a 1.0.2-
based git tree, *with* its full changelog:

http://git.infradead.org/users/dwmw2/edk2.git/commitdiff/cf8dd4aee409

I mention this just to reinforce the need for that change, even before
we make the switch to OpenSSL 1.1.

FWIW I was unable to apply the patch from your email; if there was ever
a trick to managing the bogus line endings, I've forgotten it. Can we
*please* keep native line endings in the git tree and let it be checked
out into the native form — like everyone else does?

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation





smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-18 Thread Laszlo Ersek 
On 02/18/16 12:20, David Woodhouse wrote:
> On Thu, 2016-02-18 at 09:50 +0100, Laszlo Ersek wrote:
>> On 02/18/16 09:44, Long, Qin wrote:
>>> Thanks for raising this, Laszlo.
>>>  
>>> Exactly, the posted patch series from David also included one
>> 1.0.2f enabling. The patch series will bring one direct / smooth
>> supports for EDKII-CryptoPkg with some patch integration in both
>> EDKII and OpenSSL sides, and also introduce some source generation
>> mechanism for more native build support. 
>>>  
>>> I will work on more validations based on David's post, and also
>> work with David on other possible updates (e.g. include file issue).
>> This may need some extra times.
>>>  
>>> Before all patches were integrated, my plan is to have one 1.0.2f
>> upgrade firstly based on my last patch, which will not change any
>> build process, and just to catch the latest release for some
>> requirements. 
>>>  
>>> (David, apology for my late feedback to your patch post.)
>>>  
>>> Let me know if any concerns. 
>>
>> Works for me if it works for David.
> 
> Yeah, that's fine. I'm happy to rebase my tree and put the upgrade to
> 1.0.2f first.

Thank you for confirming -- consider my NACK rescinded.

> Although I *did* like having "it's easy now..." as the commit comment.
> 
> That exercise has highlighted one more potential improvement — the
> upgrade from 1.0.2e to 1.0.2f did require changing about 18 instances
> of the string "1.0.2e" to "1.0.2f". I'll see if I can cut that down.

Thanks!
Laszlo

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-18 Thread David Woodhouse 
On Thu, 2016-02-18 at 09:50 +0100, Laszlo Ersek wrote:
> On 02/18/16 09:44, Long, Qin wrote:
> > Thanks for raising this, Laszlo.
> > 
> > Exactly, the posted patch series from David also included one
> 1.0.2f enabling. The patch series will bring one direct / smooth
> supports for EDKII-CryptoPkg with some patch integration in both
> EDKII and OpenSSL sides, and also introduce some source generation
> mechanism for more native build support. 
> > 
> > I will work on more validations based on David's post, and also
> work with David on other possible updates (e.g. include file issue).
> This may need some extra times.
> > 
> > Before all patches were integrated, my plan is to have one 1.0.2f
> upgrade firstly based on my last patch, which will not change any
> build process, and just to catch the latest release for some
> requirements. 
> > 
> > (David, apology for my late feedback to your patch post.)
> > 
> > Let me know if any concerns. 
> 
> Works for me if it works for David.

Yeah, that's fine. I'm happy to rebase my tree and put the upgrade to
1.0.2f first.

Although I *did* like having "it's easy now..." as the commit comment.

That exercise has highlighted one more potential improvement — the
upgrade from 1.0.2e to 1.0.2f did require changing about 18 instances
of the string "1.0.2e" to "1.0.2f". I'll see if I can cut that down.

-- 
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com  Intel Corporation



smime.p7s
Description: S/MIME cryptographic signature
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-18 Thread Laszlo Ersek 
On 02/18/16 09:44, Long, Qin wrote:
> Thanks for raising this, Laszlo.
> 
> Exactly, the posted patch series from David also included one 1.0.2f 
> enabling. The patch series will bring one direct / smooth supports for 
> EDKII-CryptoPkg with some patch integration in both EDKII and OpenSSL sides, 
> and also introduce some source generation mechanism for more native build 
> support. 
> 
> I will work on more validations based on David's post, and also work with 
> David on other possible updates (e.g. include file issue). This may need some 
> extra times.
> 
> Before all patches were integrated, my plan is to have one 1.0.2f upgrade 
> firstly based on my last patch, which will not change any build process, and 
> just to catch the latest release for some requirements. 
> 
> (David, apology for my late feedback to your patch post.)
> 
> Let me know if any concerns. 

Works for me if it works for David.

Thanks
Laszlo

> 
> 
> Best Regards & Thanks,
> LONG, Qin
> 
>> -Original Message-
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Thursday, February 18, 2016 4:00 PM
>> To: Ye, Ting; Long, Qin; edk2-devel@lists.01.org; David Woodhouse
>> Subject: Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version
>> to 1.0.2f
>>
>> On 02/18/16 06:59, Ye, Ting wrote:
>>> Looks good to me.
>>>
>>> Reviewed-by: Ye Ting <ting...@intel.com>
>>
>> For now:
>>
>> Nacked-by: Laszlo Ersek <ler...@redhat.com>
>>
>> This is only a technical NACK -- I'd just like to make everyone aware that
>> David has concurrently posted a patch series, that does the same, and
>> significantly more:
>>
>>   EDK2 vs. OpenSSL HEAD update
>>   http://thread.gmane.org/gmane.comp.bios.edk2.devel/7716
>>
>> (In particular see [edk2] [PATCH 5/7] CryptoPkg/OpensslLib: Update to
>> OpenSSL 1.0.2f.)
>>
>> We should figure out which of the two is the way forward -- before that
>> happens, this patch should not be pushed.
>>
>> (I know Qin Long is aware of David's posting: David CC'd Qin Long.
>> Still, let's connect the threads like this.)
>>
>> Thanks
>> Laszlo
>>
>>
>>
>>> -Original Message-
>>> From: Long, Qin
>>> Sent: Thursday, February 18, 2016 12:33 AM
>>> To: edk2-devel@lists.01.org; Ye, Ting
>>> Subject: [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to
>>> 1.0.2f
>>>
>>> OpenSSL has released version 1.0.2f with two security fixes
>>> (http://www.openssl.org/news/secadv/20160128.txt) at 28-Jan-2016.
>>> Upgrade the supported OpenSSL version in CryptoPkg/OpensslLib to catch
>> the latest release 1.0.2f.
>>> (NOTE: The patch file was just re-generated, and no new source
>>>changes was introduced for 1.0.2f enabling)
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Qin Long <qin.l...@intel.com>
>>> CC: Ting Ye <ting...@intel.com>
>>> ---
>>>  ...ssl-1.0.2e.patch => EDKII_openssl-1.0.2f.patch} | 63 +++
>> ---
>>>  CryptoPkg/Library/OpensslLib/Install.cmd   |  2 +-
>>>  CryptoPkg/Library/OpensslLib/Install.sh|  2 +-
>>>  CryptoPkg/Library/OpensslLib/OpensslLib.inf|  4 +-
>>>  CryptoPkg/Library/OpensslLib/Patch-HOWTO.txt   | 26 -
>>>  5 files changed, 48 insertions(+), 49 deletions(-)  rename
>>> CryptoPkg/Library/OpensslLib/{EDKII_openssl-1.0.2e.patch =>
>>> EDKII_openssl-1.0.2f.patch} (89%)
>>>
>>> diff --git a/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2e.patch
>>> b/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2f.patch
>>> similarity index 89%
>>> rename from CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2e.patch
>>> rename to CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2f.patch
>>> index e4eaff6..c42b776 100644
>>> --- a/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2e.patch
>>> +++ b/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2f.patch
>>> @@ -1,7 +1,7 @@
>>>  diff U3 crypto/bio/bio.h crypto/bio/bio.h
>>>  crypto/bio/bio.h   Thu Jun 11 21:50:12 2015
>>> -+++ crypto/bio/bio.h   Fri Jun 12 11:00:52 2015
>>> -@@ -646,10 +646,10 @@
>>> +--- crypto/bio/bio.h   Thu Jan 28 21:56:08 2016
>>>  crypto/bio/bio.h   Wed Feb 17 16:43:40 2016
>>> +@@ -650,10 +650,10 @@
>>>   int BIO_asn1_get_suffix(BIO *b, asn1_ps_func **psuffix,
>>>

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-18 Thread Long, Qin 
Thanks for raising this, Laszlo.

Exactly, the posted patch series from David also included one 1.0.2f enabling. 
The patch series will bring one direct / smooth supports for EDKII-CryptoPkg 
with some patch integration in both EDKII and OpenSSL sides, and also introduce 
some source generation mechanism for more native build support. 

I will work on more validations based on David's post, and also work with David 
on other possible updates (e.g. include file issue). This may need some extra 
times.

Before all patches were integrated, my plan is to have one 1.0.2f upgrade 
firstly based on my last patch, which will not change any build process, and 
just to catch the latest release for some requirements. 

(David, apology for my late feedback to your patch post.)

Let me know if any concerns. 


Best Regards & Thanks,
LONG, Qin

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Thursday, February 18, 2016 4:00 PM
> To: Ye, Ting; Long, Qin; edk2-devel@lists.01.org; David Woodhouse
> Subject: Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version
> to 1.0.2f
> 
> On 02/18/16 06:59, Ye, Ting wrote:
> > Looks good to me.
> >
> > Reviewed-by: Ye Ting <ting...@intel.com>
> 
> For now:
> 
> Nacked-by: Laszlo Ersek <ler...@redhat.com>
> 
> This is only a technical NACK -- I'd just like to make everyone aware that
> David has concurrently posted a patch series, that does the same, and
> significantly more:
> 
>   EDK2 vs. OpenSSL HEAD update
>   http://thread.gmane.org/gmane.comp.bios.edk2.devel/7716
> 
> (In particular see [edk2] [PATCH 5/7] CryptoPkg/OpensslLib: Update to
> OpenSSL 1.0.2f.)
> 
> We should figure out which of the two is the way forward -- before that
> happens, this patch should not be pushed.
> 
> (I know Qin Long is aware of David's posting: David CC'd Qin Long.
> Still, let's connect the threads like this.)
> 
> Thanks
> Laszlo
> 
> 
> 
> > -Original Message-
> > From: Long, Qin
> > Sent: Thursday, February 18, 2016 12:33 AM
> > To: edk2-devel@lists.01.org; Ye, Ting
> > Subject: [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to
> > 1.0.2f
> >
> > OpenSSL has released version 1.0.2f with two security fixes
> > (http://www.openssl.org/news/secadv/20160128.txt) at 28-Jan-2016.
> > Upgrade the supported OpenSSL version in CryptoPkg/OpensslLib to catch
> the latest release 1.0.2f.
> > (NOTE: The patch file was just re-generated, and no new source
> >changes was introduced for 1.0.2f enabling)
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Qin Long <qin.l...@intel.com>
> > CC: Ting Ye <ting...@intel.com>
> > ---
> >  ...ssl-1.0.2e.patch => EDKII_openssl-1.0.2f.patch} | 63 +++
> ---
> >  CryptoPkg/Library/OpensslLib/Install.cmd   |  2 +-
> >  CryptoPkg/Library/OpensslLib/Install.sh|  2 +-
> >  CryptoPkg/Library/OpensslLib/OpensslLib.inf|  4 +-
> >  CryptoPkg/Library/OpensslLib/Patch-HOWTO.txt   | 26 -
> >  5 files changed, 48 insertions(+), 49 deletions(-)  rename
> > CryptoPkg/Library/OpensslLib/{EDKII_openssl-1.0.2e.patch =>
> > EDKII_openssl-1.0.2f.patch} (89%)
> >
> > diff --git a/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2e.patch
> > b/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2f.patch
> > similarity index 89%
> > rename from CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2e.patch
> > rename to CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2f.patch
> > index e4eaff6..c42b776 100644
> > --- a/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2e.patch
> > +++ b/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2f.patch
> > @@ -1,7 +1,7 @@
> >  diff U3 crypto/bio/bio.h crypto/bio/bio.h
> >  crypto/bio/bio.h   Thu Jun 11 21:50:12 2015
> > -+++ crypto/bio/bio.h   Fri Jun 12 11:00:52 2015
> > -@@ -646,10 +646,10 @@
> > +--- crypto/bio/bio.h   Thu Jan 28 21:56:08 2016
> >  crypto/bio/bio.h   Wed Feb 17 16:43:40 2016
> > +@@ -650,10 +650,10 @@
> >   int BIO_asn1_get_suffix(BIO *b, asn1_ps_func **psuffix,
> >   asn1_ps_func **psuffix_free);
> >
> > @@ -14,8 +14,8 @@ diff U3 crypto/bio/bio.h crypto/bio/bio.h
> >   # endif
> >   BIO *BIO_new(BIO_METHOD *type);
> >  diff U3 crypto/bio/bss_file.c crypto/bio/bss_file.c
> >  crypto/bio/bss_file.c  Thu Jun 11 21:01:06 2015
> > -+++ crypto/bio/bss_file.c  Fri Jun 12 11:01:28 2015
> > +--- crypto/bio/bss_file.c  Thu Jan 28 21:38:30 2016
> >  crypto/bio/bss_file.c  Wed Feb 17

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-18 Thread Laszlo Ersek 
On 02/18/16 06:59, Ye, Ting wrote:
> Looks good to me.
> 
> Reviewed-by: Ye Ting  

For now:

Nacked-by: Laszlo Ersek 

This is only a technical NACK -- I'd just like to make everyone aware
that David has concurrently posted a patch series, that does the same,
and significantly more:

  EDK2 vs. OpenSSL HEAD update
  http://thread.gmane.org/gmane.comp.bios.edk2.devel/7716

(In particular see [edk2] [PATCH 5/7] CryptoPkg/OpensslLib: Update to
OpenSSL 1.0.2f.)

We should figure out which of the two is the way forward -- before that
happens, this patch should not be pushed.

(I know Qin Long is aware of David's posting: David CC'd Qin Long.
Still, let's connect the threads like this.)

Thanks
Laszlo



> -Original Message-
> From: Long, Qin 
> Sent: Thursday, February 18, 2016 12:33 AM
> To: edk2-devel@lists.01.org; Ye, Ting
> Subject: [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f
> 
> OpenSSL has released version 1.0.2f with two security fixes
> (http://www.openssl.org/news/secadv/20160128.txt) at 28-Jan-2016.
> Upgrade the supported OpenSSL version in CryptoPkg/OpensslLib to catch the 
> latest release 1.0.2f.
> (NOTE: The patch file was just re-generated, and no new source
>changes was introduced for 1.0.2f enabling)
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Qin Long 
> CC: Ting Ye 
> ---
>  ...ssl-1.0.2e.patch => EDKII_openssl-1.0.2f.patch} | 63 
> +++---
>  CryptoPkg/Library/OpensslLib/Install.cmd   |  2 +-
>  CryptoPkg/Library/OpensslLib/Install.sh|  2 +-
>  CryptoPkg/Library/OpensslLib/OpensslLib.inf|  4 +-
>  CryptoPkg/Library/OpensslLib/Patch-HOWTO.txt   | 26 -
>  5 files changed, 48 insertions(+), 49 deletions(-)  rename 
> CryptoPkg/Library/OpensslLib/{EDKII_openssl-1.0.2e.patch => 
> EDKII_openssl-1.0.2f.patch} (89%)
> 
> diff --git a/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2e.patch 
> b/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2f.patch
> similarity index 89%
> rename from CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2e.patch
> rename to CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2f.patch
> index e4eaff6..c42b776 100644
> --- a/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2e.patch
> +++ b/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2f.patch
> @@ -1,7 +1,7 @@
>  diff U3 crypto/bio/bio.h crypto/bio/bio.h
>  crypto/bio/bio.h Thu Jun 11 21:50:12 2015
> -+++ crypto/bio/bio.h Fri Jun 12 11:00:52 2015
> -@@ -646,10 +646,10 @@
> +--- crypto/bio/bio.h Thu Jan 28 21:56:08 2016
>  crypto/bio/bio.h Wed Feb 17 16:43:40 2016
> +@@ -650,10 +650,10 @@
>   int BIO_asn1_get_suffix(BIO *b, asn1_ps_func **psuffix,
>   asn1_ps_func **psuffix_free);
>   
> @@ -14,8 +14,8 @@ diff U3 crypto/bio/bio.h crypto/bio/bio.h
>   # endif
>   BIO *BIO_new(BIO_METHOD *type);
>  diff U3 crypto/bio/bss_file.c crypto/bio/bss_file.c
>  crypto/bio/bss_file.cThu Jun 11 21:01:06 2015
> -+++ crypto/bio/bss_file.cFri Jun 12 11:01:28 2015
> +--- crypto/bio/bss_file.cThu Jan 28 21:38:30 2016
>  crypto/bio/bss_file.cWed Feb 17 16:01:02 2016
>  @@ -467,6 +467,23 @@
>   return (ret);
>   }
> @@ -41,8 +41,8 @@ diff U3 crypto/bio/bss_file.c crypto/bio/bss_file.c
>   
>   #endif  /* HEADER_BSS_FILE_C */
>  diff U3 crypto/dh/dh_pmeth.c crypto/dh/dh_pmeth.c
>  crypto/dh/dh_pmeth.c Thu Jun 11 21:50:12 2015
> -+++ crypto/dh/dh_pmeth.c Fri Jun 12 11:08:48 2015
> +--- crypto/dh/dh_pmeth.c Thu Jan 28 21:56:08 2016
>  crypto/dh/dh_pmeth.c Wed Feb 17 16:15:58 2016
>  @@ -449,6 +449,9 @@
>   *keylen = ret;
>   return 1;
> @@ -62,8 +62,8 @@ diff U3 crypto/dh/dh_pmeth.c crypto/dh/dh_pmeth.c
>   return 1;
>   }
>  diff U3 crypto/pem/pem.h crypto/pem/pem.h
>  crypto/pem/pem.h Thu Jun 11 21:50:12 2015
> -+++ crypto/pem/pem.h Fri Jun 12 10:58:18 2015
> +--- crypto/pem/pem.h Thu Jan 28 21:56:08 2016
>  crypto/pem/pem.h Wed Feb 17 15:56:26 2016
>  @@ -324,6 +324,7 @@
>   
>   #  define DECLARE_PEM_read_fp(name, type) /**/ @@ -73,8 +73,8 @@ diff U3 
> crypto/pem/pem.h crypto/pem/pem.h
>   # else
>   
>  diff U3 crypto/pkcs7/pk7_smime.c crypto/pkcs7/pk7_smime.c
>  crypto/pkcs7/pk7_smime.c Thu Jun 11 21:01:06 2015
> -+++ crypto/pkcs7/pk7_smime.c Fri Jun 12 11:23:38 2015
> +--- crypto/pkcs7/pk7_smime.c Thu Jan 28 21:56:08 2016
>  crypto/pkcs7/pk7_smime.c Wed Feb 17 16:22:45 2016
>  @@ -254,7 +254,8 @@
>   STACK_OF(PKCS7_SIGNER_INFO) *sinfos;
>   PKCS7_SIGNER_INFO *si;
> @@ -114,20 +114,19 @@ diff U3 crypto/pkcs7/pk7_smime.c 
> crypto/pkcs7/pk7_smime.c
>   if (i <= 0)
>   break;
>   if (tmpout)
> -@@ -394,6 +394,10 @@
> +@@ -394,6 +394,9 @@
>   }
>   BIO_free_all(p7bio);
>   sk_X509_free(signers);
> -+
>  +if (buf != NULL) {
> -+  OPENSSL_free(buf);
>

Re: [edk2] [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

2016-02-17 Thread Ye, Ting 
Looks good to me.

Reviewed-by: Ye Ting  

-Original Message-
From: Long, Qin 
Sent: Thursday, February 18, 2016 12:33 AM
To: edk2-devel@lists.01.org; Ye, Ting
Subject: [Patch] CryptoPkg/OpensslLib: Upgrade OpenSSL version to 1.0.2f

OpenSSL has released version 1.0.2f with two security fixes
(http://www.openssl.org/news/secadv/20160128.txt) at 28-Jan-2016.
Upgrade the supported OpenSSL version in CryptoPkg/OpensslLib to catch the 
latest release 1.0.2f.
(NOTE: The patch file was just re-generated, and no new source
   changes was introduced for 1.0.2f enabling)

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Qin Long 
CC: Ting Ye 
---
 ...ssl-1.0.2e.patch => EDKII_openssl-1.0.2f.patch} | 63 +++---
 CryptoPkg/Library/OpensslLib/Install.cmd   |  2 +-
 CryptoPkg/Library/OpensslLib/Install.sh|  2 +-
 CryptoPkg/Library/OpensslLib/OpensslLib.inf|  4 +-
 CryptoPkg/Library/OpensslLib/Patch-HOWTO.txt   | 26 -
 5 files changed, 48 insertions(+), 49 deletions(-)  rename 
CryptoPkg/Library/OpensslLib/{EDKII_openssl-1.0.2e.patch => 
EDKII_openssl-1.0.2f.patch} (89%)

diff --git a/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2e.patch 
b/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2f.patch
similarity index 89%
rename from CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2e.patch
rename to CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2f.patch
index e4eaff6..c42b776 100644
--- a/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2e.patch
+++ b/CryptoPkg/Library/OpensslLib/EDKII_openssl-1.0.2f.patch
@@ -1,7 +1,7 @@
 diff U3 crypto/bio/bio.h crypto/bio/bio.h
 crypto/bio/bio.h   Thu Jun 11 21:50:12 2015
-+++ crypto/bio/bio.h   Fri Jun 12 11:00:52 2015
-@@ -646,10 +646,10 @@
+--- crypto/bio/bio.h   Thu Jan 28 21:56:08 2016
 crypto/bio/bio.h   Wed Feb 17 16:43:40 2016
+@@ -650,10 +650,10 @@
  int BIO_asn1_get_suffix(BIO *b, asn1_ps_func **psuffix,
  asn1_ps_func **psuffix_free);
  
@@ -14,8 +14,8 @@ diff U3 crypto/bio/bio.h crypto/bio/bio.h
  # endif
  BIO *BIO_new(BIO_METHOD *type);
 diff U3 crypto/bio/bss_file.c crypto/bio/bss_file.c
 crypto/bio/bss_file.c  Thu Jun 11 21:01:06 2015
-+++ crypto/bio/bss_file.c  Fri Jun 12 11:01:28 2015
+--- crypto/bio/bss_file.c  Thu Jan 28 21:38:30 2016
 crypto/bio/bss_file.c  Wed Feb 17 16:01:02 2016
 @@ -467,6 +467,23 @@
  return (ret);
  }
@@ -41,8 +41,8 @@ diff U3 crypto/bio/bss_file.c crypto/bio/bss_file.c
  
  #endif  /* HEADER_BSS_FILE_C */
 diff U3 crypto/dh/dh_pmeth.c crypto/dh/dh_pmeth.c
 crypto/dh/dh_pmeth.c   Thu Jun 11 21:50:12 2015
-+++ crypto/dh/dh_pmeth.c   Fri Jun 12 11:08:48 2015
+--- crypto/dh/dh_pmeth.c   Thu Jan 28 21:56:08 2016
 crypto/dh/dh_pmeth.c   Wed Feb 17 16:15:58 2016
 @@ -449,6 +449,9 @@
  *keylen = ret;
  return 1;
@@ -62,8 +62,8 @@ diff U3 crypto/dh/dh_pmeth.c crypto/dh/dh_pmeth.c
  return 1;
  }
 diff U3 crypto/pem/pem.h crypto/pem/pem.h
 crypto/pem/pem.h   Thu Jun 11 21:50:12 2015
-+++ crypto/pem/pem.h   Fri Jun 12 10:58:18 2015
+--- crypto/pem/pem.h   Thu Jan 28 21:56:08 2016
 crypto/pem/pem.h   Wed Feb 17 15:56:26 2016
 @@ -324,6 +324,7 @@
  
  #  define DECLARE_PEM_read_fp(name, type) /**/ @@ -73,8 +73,8 @@ diff U3 
crypto/pem/pem.h crypto/pem/pem.h
  # else
  
 diff U3 crypto/pkcs7/pk7_smime.c crypto/pkcs7/pk7_smime.c
 crypto/pkcs7/pk7_smime.c   Thu Jun 11 21:01:06 2015
-+++ crypto/pkcs7/pk7_smime.c   Fri Jun 12 11:23:38 2015
+--- crypto/pkcs7/pk7_smime.c   Thu Jan 28 21:56:08 2016
 crypto/pkcs7/pk7_smime.c   Wed Feb 17 16:22:45 2016
 @@ -254,7 +254,8 @@
  STACK_OF(PKCS7_SIGNER_INFO) *sinfos;
  PKCS7_SIGNER_INFO *si;
@@ -114,20 +114,19 @@ diff U3 crypto/pkcs7/pk7_smime.c crypto/pkcs7/pk7_smime.c
  if (i <= 0)
  break;
  if (tmpout)
-@@ -394,6 +394,10 @@
+@@ -394,6 +394,9 @@
  }
  BIO_free_all(p7bio);
  sk_X509_free(signers);
-+
 +if (buf != NULL) {
-+  OPENSSL_free(buf);
++OPENSSL_free(buf);
 +}
  return ret;
  }
  
 diff U3 crypto/rand/rand_unix.c crypto/rand/rand_unix.c
 crypto/rand/rand_unix.cThu Jun 11 21:01:06 2015
-+++ crypto/rand/rand_unix.cFri Jun 12 10:51:21 2015
+--- crypto/rand/rand_unix.cThu Jan 28 21:38:32 2016
 crypto/rand/rand_unix.cWed Feb 17 15:40:02 2016
 @@ -116,7 +116,7 @@
  #include 
  #include "rand_lcl.h"
@@ -147,8 +146,8 @@ diff U3 crypto/rand/rand_unix.c crypto/rand/rand_unix.c
  {
  return 0;
 diff U3 crypto/rsa/rsa_ameth.c crypto/rsa/rsa_ameth.c
 crypto/rsa/rsa_ameth.c Thu Jun 11 21:50:12 2015
-+++ crypto/rsa/rsa_ameth.c Fri Jun 12 10:45:38 2015
+--- crypto/rsa/rsa_ameth.c Thu Jan 28 21:56:08 2016
 crypto/rsa/rsa_ameth.c Wed Feb 17 15:09:46 2016
 @@ -68,10 +68,12 @@
  #endif
  #include "asn1_locl.h"
@@ -221,8 +220,8