[v1] ArmVirt- Nt32- Ovmf- CryptoPkg: conditionalize libssl presence in OpensslLib

[edk2] [URGENT-ish PATCH 0/5] ArmVirt- Nt32- Ovmf- CryptoPkg: conditionalize libssl presence in OpensslLib

Posted by Laszlo Ersek 7 years, 1 month ago

In commit 32387e0081db ("CryptoPkg: Enable ssl build in OpensslLib
directly", 2016-12-14), we enabled libssl functionality in
CryptoPkg/OpensslLib unconditionally.

While that's real convenient, it is also overkill for platforms (or
platform builds) that don't want TLS. The impact (beyond wasted build
time) is that when the next vulnerability comes out that affects the
libssl subset of OpenSSL, security teams all around will look at build
logs and INF files, see the libssl files being built, and get nervous --
without a good reason for such builds that don't actually *use* TLS.

Let's make this easier on them (and thereby on ourselves!), and
introduce an OpensslLibNoSsl instance, which excludes libssl.

The edk2 integration script "process_files.sh" is updated to process
both INF files in the same invocation.

If noone disagrees with the concept, I'd appreciate if we could review &
merge this series real fast. (Sorry about that, but a downstream
deadline looms close, and I consider this sort of a blocker for the next
rebase.)

I updated the following platform packages:
- ArmVirtPkg, because I know it never uses TLS (or HTTP boot for that
  matter),
- Nt32Pkg, because it exposes the TLS_ENABLE build flag,
- OvmfPkg, because it exposes the TLS_ENABLE build flag.

I didn't touch other packages because they don't expose TLS_ENABLE, and
I don't have time to figure out if they want TLS built-in.

I tested the new OpensslLibNoSsl instance with Secure Boot under OVMF.

The series was formatted with "--find-copies-harder", which makes a real
difference for patch #2.

Tomas: if you would like to comment on this series, please subscribe to
the edk2-devel list at
<https://lists.01.org/mailman/listinfo/edk2-devel>, and also wait for
your subscription request to complete, *before* responding.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Gary Lin <glin@suse.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Qin Long <qin.long@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Ting Ye <ting.ye@intel.com>
Cc: Tomas Hoger <thoger@redhat.com>

Thanks!
Laszlo

Laszlo Ersek (5):
  CryptoPkg/OpensslLib: refresh OpensslLib.inf, opensslconf.h after
    32387e00
  CryptoPkg/OpensslLib: introduce OpensslLibNoSsl instance
  ArmVirtPkg: resolve OpensslLib to OpensslLibNoSsl
  Nt32Pkg: exclude libssl functionality from OpensslLib if
    TLS_ENABLE=FALSE
  OvmfPkg: exclude libssl functionality from OpensslLib if
    TLS_ENABLE=FALSE

 ArmVirtPkg/ArmVirt.dsc.inc                                           |  2 +-
 Nt32Pkg/Nt32Pkg.dsc                                                  |  4 ++
 OvmfPkg/OvmfPkgIa32.dsc                                              |  4 ++
 OvmfPkg/OvmfPkgIa32X64.dsc                                           |  4 ++
 OvmfPkg/OvmfPkgX64.dsc                                               |  4 ++
 CryptoPkg/Library/OpensslLib/OpensslLib.inf                          |  1 +
 CryptoPkg/Library/OpensslLib/{OpensslLib.inf => OpensslLibNoSsl.inf} | 55 ++------------------
 CryptoPkg/Library/OpensslLib/opensslconf.h                           |  6 ---
 CryptoPkg/Library/OpensslLib/{OpensslLib.uni => OpensslLibNoSsl.uni} |  8 +--
 CryptoPkg/Library/OpensslLib/process_files.sh                        | 27 +++++++---
 10 files changed, 46 insertions(+), 69 deletions(-)
 copy CryptoPkg/Library/OpensslLib/{OpensslLib.inf => OpensslLibNoSsl.inf} (90%)
 copy CryptoPkg/Library/OpensslLib/{OpensslLib.uni => OpensslLibNoSsl.uni} (71%)

-- 
2.9.3

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

Re: [edk2] [URGENT-ish PATCH 0/5] ArmVirt- Nt32- Ovmf- CryptoPkg: conditionalize libssl presence in OpensslLib

Posted by Laszlo Ersek 7 years, 1 month ago

On 02/23/17 22:57, Laszlo Ersek wrote:
> In commit 32387e0081db ("CryptoPkg: Enable ssl build in OpensslLib
> directly", 2016-12-14), we enabled libssl functionality in
> CryptoPkg/OpensslLib unconditionally.
> 
> While that's real convenient, it is also overkill for platforms (or
> platform builds) that don't want TLS. The impact (beyond wasted build
> time) is that when the next vulnerability comes out that affects the
> libssl subset of OpenSSL, security teams all around will look at build
> logs and INF files, see the libssl files being built, and get nervous --
> without a good reason for such builds that don't actually *use* TLS.
> 
> Let's make this easier on them (and thereby on ourselves!), and
> introduce an OpensslLibNoSsl instance, which excludes libssl.

Public repo and branch:
https://github.com/lersek/edk2.git conditionalize-ssl

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

Re: [edk2] [URGENT-ish PATCH 0/5] ArmVirt- Nt32- Ovmf- CryptoPkg: conditionalize libssl presence in OpensslLib

Posted by Ard Biesheuvel 7 years, 1 month ago

On 23 February 2017 at 21:57, Laszlo Ersek <lersek@redhat.com> wrote:
> In commit 32387e0081db ("CryptoPkg: Enable ssl build in OpensslLib
> directly", 2016-12-14), we enabled libssl functionality in
> CryptoPkg/OpensslLib unconditionally.
>
> While that's real convenient, it is also overkill for platforms (or
> platform builds) that don't want TLS. The impact (beyond wasted build
> time) is that when the next vulnerability comes out that affects the
> libssl subset of OpenSSL, security teams all around will look at build
> logs and INF files, see the libssl files being built, and get nervous --
> without a good reason for such builds that don't actually *use* TLS.
>
> Let's make this easier on them (and thereby on ourselves!), and
> introduce an OpensslLibNoSsl instance, which excludes libssl.
>

I think it would be nicer to align with OpenSSL more closely, and
split the functionality into a libcrypto and a libssl library, and
include the latter only if TLS functionality is needed. However, I am
not volunteering to do the work, and this approach comes down to the
same thing, given that libssl depends on libcrypto, and so libcrypto
and libcrypto+libssl are the only combinations that make any sense.

> The edk2 integration script "process_files.sh" is updated to process
> both INF files in the same invocation.
>
> If noone disagrees with the concept, I'd appreciate if we could review &
> merge this series real fast. (Sorry about that, but a downstream
> deadline looms close, and I consider this sort of a blocker for the next
> rebase.)
>
> I updated the following platform packages:
> - ArmVirtPkg, because I know it never uses TLS (or HTTP boot for that
>   matter),
> - Nt32Pkg, because it exposes the TLS_ENABLE build flag,
> - OvmfPkg, because it exposes the TLS_ENABLE build flag.
>
> I didn't touch other packages because they don't expose TLS_ENABLE, and
> I don't have time to figure out if they want TLS built-in.
>
> I tested the new OpensslLibNoSsl instance with Secure Boot under OVMF.
>
> The series was formatted with "--find-copies-harder", which makes a real
> difference for patch #2.
>
> Tomas: if you would like to comment on this series, please subscribe to
> the edk2-devel list at
> <https://lists.01.org/mailman/listinfo/edk2-devel>, and also wait for
> your subscription request to complete, *before* responding.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gary Lin <glin@suse.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Qin Long <qin.long@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Ting Ye <ting.ye@intel.com>
> Cc: Tomas Hoger <thoger@redhat.com>
>
> Thanks!
> Laszlo
>
> Laszlo Ersek (5):
>   CryptoPkg/OpensslLib: refresh OpensslLib.inf, opensslconf.h after
>     32387e00
>   CryptoPkg/OpensslLib: introduce OpensslLibNoSsl instance
>   ArmVirtPkg: resolve OpensslLib to OpensslLibNoSsl
>   Nt32Pkg: exclude libssl functionality from OpensslLib if
>     TLS_ENABLE=FALSE
>   OvmfPkg: exclude libssl functionality from OpensslLib if
>     TLS_ENABLE=FALSE
>
>  ArmVirtPkg/ArmVirt.dsc.inc                                           |  2 +-
>  Nt32Pkg/Nt32Pkg.dsc                                                  |  4 ++
>  OvmfPkg/OvmfPkgIa32.dsc                                              |  4 ++
>  OvmfPkg/OvmfPkgIa32X64.dsc                                           |  4 ++
>  OvmfPkg/OvmfPkgX64.dsc                                               |  4 ++
>  CryptoPkg/Library/OpensslLib/OpensslLib.inf                          |  1 +
>  CryptoPkg/Library/OpensslLib/{OpensslLib.inf => OpensslLibNoSsl.inf} | 55 ++------------------
>  CryptoPkg/Library/OpensslLib/opensslconf.h                           |  6 ---
>  CryptoPkg/Library/OpensslLib/{OpensslLib.uni => OpensslLibNoSsl.uni} |  8 +--
>  CryptoPkg/Library/OpensslLib/process_files.sh                        | 27 +++++++---
>  10 files changed, 46 insertions(+), 69 deletions(-)
>  copy CryptoPkg/Library/OpensslLib/{OpensslLib.inf => OpensslLibNoSsl.inf} (90%)
>  copy CryptoPkg/Library/OpensslLib/{OpensslLib.uni => OpensslLibNoSsl.uni} (71%)
>
> --
> 2.9.3
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [URGENT-ish PATCH 0/5] ArmVirt- Nt32- Ovmf- CryptoPkg: conditionalize libssl presence in OpensslLib

Posted by Long, Qin 7 years, 1 month ago

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Friday, February 24, 2017 6:25 AM
> To: Laszlo Ersek
> Cc: edk2-devel-01; Gary Lin; Wu, Jiaxin; Justen, Jordan L; Long, Qin; Ni, Ruiyu;
> Ye, Ting; Tomas Hoger
> Subject: Re: [URGENT-ish PATCH 0/5] ArmVirt- Nt32- Ovmf- CryptoPkg:
> conditionalize libssl presence in OpensslLib
> 
> On 23 February 2017 at 21:57, Laszlo Ersek <lersek@redhat.com> wrote:
> > In commit 32387e0081db ("CryptoPkg: Enable ssl build in OpensslLib
> > directly", 2016-12-14), we enabled libssl functionality in
> > CryptoPkg/OpensslLib unconditionally.
> >
> > While that's real convenient, it is also overkill for platforms (or
> > platform builds) that don't want TLS. The impact (beyond wasted build
> > time) is that when the next vulnerability comes out that affects the
> > libssl subset of OpenSSL, security teams all around will look at build
> > logs and INF files, see the libssl files being built, and get nervous
> > -- without a good reason for such builds that don't actually *use* TLS.
> >
> > Let's make this easier on them (and thereby on ourselves!), and
> > introduce an OpensslLibNoSsl instance, which excludes libssl.
> >
> 
> I think it would be nicer to align with OpenSSL more closely, and split the
> functionality into a libcrypto and a libssl library, and include the latter only if
> TLS functionality is needed. However, I am not volunteering to do the work,
> and this approach comes down to the same thing, given that libssl depends
> on libcrypto, and so libcrypto and libcrypto+libssl are the only combinations
> that make any sense.

I agree.
In fact, we used the two separated library (crypto & ssl) aligned with OpenSSL before,
And received some comments to combine them together. :-)
Two libraries (libcryto and the combination of libcrypto and libssl) are good to me. But
I prefer to use the OpensslLibCrypto as the name, then we can have:
	OpensslLibCrypto     (libcrypto)
	OpensslLib	        (libcrypto + libssl)

OpensslLib may have more dependencies / usages in different module, we may need extra
Patches to update them.

LONG, Qin

> 
> > The edk2 integration script "process_files.sh" is updated to process
> > both INF files in the same invocation.
> >
> > If noone disagrees with the concept, I'd appreciate if we could review
> > & merge this series real fast. (Sorry about that, but a downstream
> > deadline looms close, and I consider this sort of a blocker for the
> > next
> > rebase.)
> >
> > I updated the following platform packages:
> > - ArmVirtPkg, because I know it never uses TLS (or HTTP boot for that
> >   matter),
> > - Nt32Pkg, because it exposes the TLS_ENABLE build flag,
> > - OvmfPkg, because it exposes the TLS_ENABLE build flag.
> >
> > I didn't touch other packages because they don't expose TLS_ENABLE,
> > and I don't have time to figure out if they want TLS built-in.
> >
> > I tested the new OpensslLibNoSsl instance with Secure Boot under OVMF.
> >
> > The series was formatted with "--find-copies-harder", which makes a
> > real difference for patch #2.
> >
> > Tomas: if you would like to comment on this series, please subscribe
> > to the edk2-devel list at
> > <https://lists.01.org/mailman/listinfo/edk2-devel>, and also wait for
> > your subscription request to complete, *before* responding.
> >
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Gary Lin <glin@suse.com>
> > Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > Cc: Qin Long <qin.long@intel.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Ting Ye <ting.ye@intel.com>
> > Cc: Tomas Hoger <thoger@redhat.com>
> >
> > Thanks!
> > Laszlo
> >
> > Laszlo Ersek (5):
> >   CryptoPkg/OpensslLib: refresh OpensslLib.inf, opensslconf.h after
> >     32387e00
> >   CryptoPkg/OpensslLib: introduce OpensslLibNoSsl instance
> >   ArmVirtPkg: resolve OpensslLib to OpensslLibNoSsl
> >   Nt32Pkg: exclude libssl functionality from OpensslLib if
> >     TLS_ENABLE=FALSE
> >   OvmfPkg: exclude libssl functionality from OpensslLib if
> >     TLS_ENABLE=FALSE
> >
> >  ArmVirtPkg/ArmVirt.dsc.inc                                           |  2 +-
> >  Nt32Pkg/Nt32Pkg.dsc                                                  |  4 ++
> >  OvmfPkg/OvmfPkgIa32.dsc                                              |  4 ++
> >  OvmfPkg/OvmfPkgIa32X64.dsc                                           |  4 ++
> >  OvmfPkg/OvmfPkgX64.dsc                                               |  4 ++
> >  CryptoPkg/Library/OpensslLib/OpensslLib.inf                          |  1 +
> >  CryptoPkg/Library/OpensslLib/{OpensslLib.inf => OpensslLibNoSsl.inf} | 55
> ++------------------
> >  CryptoPkg/Library/OpensslLib/opensslconf.h                           |  6 ---
> >  CryptoPkg/Library/OpensslLib/{OpensslLib.uni => OpensslLibNoSsl.uni} |  8
> +--
> >  CryptoPkg/Library/OpensslLib/process_files.sh                        | 27 +++++++---
> >  10 files changed, 46 insertions(+), 69 deletions(-)  copy
> > CryptoPkg/Library/OpensslLib/{OpensslLib.inf => OpensslLibNoSsl.inf}
> > (90%)  copy CryptoPkg/Library/OpensslLib/{OpensslLib.uni =>
> > OpensslLibNoSsl.uni} (71%)
> >
> > --
> > 2.9.3
> >
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel