> -----Original Message----- > From: Palmer, Thomas [mailto:thomas.pal...@hpe.com] > Sent: Saturday, June 25, 2016 12:51 AM > To: Wu, Jiaxin <jiaxin...@intel.com>; edk2-devel@lists.01.org > Cc: El-Haj-Mahmoud, Samer <samer.el-haj-mahm...@hpe.com>; Zimmer, > Vincent <vincent.zim...@intel.com>; Li, Ruth <ruth...@intel.com>; Fu, > Siyuan <siyuan...@intel.com>; Ye, Ting <ting...@intel.com>; Hsiung, Harry L > <harry.l.hsi...@intel.com> > Subject: RE: [PATCH] [edk2-staging/HTTPS-TLS][PATCH]: Centralize TLS var > cert name and guid > > Jiaxin, et al ~ > > I noticed while using the TLS feature that the GUID and Variable Name define > were being re-defined in multiple spots. Currently, if someone were to write > a UEFI application, there is no single include file that would provide the > variable name in a define. As a matter of sheer better programming practices, > the Variable Name define and GUID should be put into central locations and > not copied all over the codebase.
Yes, I agree to put it into a single file. I can create another patch to refine it. If you would like to provide it, I'm fine:). > > With regards to GlobalVariable.h: I realize now this is not the place to put > it. > Because our variable has a unique GUID, we would have to create a new > MdePkg/Include/Guid/ header file to hold the define, much like > ImageAuthentication.h which is also used by VarCheckUefiLibNullClass.c. > > I put the GUID definition into CryptoPkg.dec because the TlsLib and OpenSSL > library are there. I can be persuaded to have it in NetworkPkg.dec as it's > modules are the ultimate consumers of the variable. CryptoPkg is simply > providing libraries. > > With regards to "[TlsCaCertificate is] only a private variable": This > variable is > super critical to secure TLS communication. It is so critical that we are > even > discussing how to protect it in runtime from malicious/careless modifications. > We understand that if this variable were compromised that there could be > severe security implications that follow. This variable must be respected > properly. Actually, I don't have the strong opinion about the security of this variable. TlsCaCertificate is used to store the public certificate that means everyone can get and use it. Take windows OS as an example, any login user can/should have the ability to modify this kind of certificate, we can think it's not protected except for the system-level access control. This is the reason why I put it into plaintext currently. But I also agree that protecting this variable is also meaningful because we no such access control. As for how to protect it, it is another question we discussed before. > > For that reason, I'd argue that we should put the TlsCaCertificate into the > UEFI Spec. When it gets put into the spec I do not know, but we should be > aiming for that. It is too important to security to not be in the spec. In my opinion, TlsCaCertificate variable is just a temporary scenario to hold the certificate, not the finally or general UEFI solution for the certificate management. So, it's pointless to standardize it, keeping it as a private variable is fine currently. > > Not only that, but once this is in the spec it will enable 3rd party > applications > to re-use this variable too. I've personally talked with one such developer > who is eagerly awaiting a variable that is a secure UEFI standard for > Certificate Authority storage. > > Thomas > > -----Original Message----- > From: Wu, Jiaxin [mailto:jiaxin...@intel.com] > Sent: Thursday, June 23, 2016 9:56 PM > To: Palmer, Thomas <thomas.pal...@hpe.com>; edk2-devel@lists.01.org > Cc: El-Haj-Mahmoud, Samer <samer.el-haj-mahm...@hpe.com>; Zimmer, > Vincent <vincent.zim...@intel.com>; Li, Ruth <ruth...@intel.com>; Fu, > Siyuan <siyuan...@intel.com>; Ye, Ting <ting...@intel.com>; Hsiung, Harry L > <harry.l.hsi...@intel.com> > Subject: RE: [PATCH] [edk2-staging/HTTPS-TLS][PATCH]: Centralize TLS var > cert name and guid > > Hi Thomas, > One point we should know that TLS cert variable is not defined in UEFI Spec, > it's only private variable used to configure the CA certificate. So, we can't > add > this variable check into VarCheckUefiLib. VarCheckUefiLib only contain the > variables defined in UEFI spec, private variable is not allowed. > EDKII_VAR_CHECK_PROTOCOL could be located directly If we truly want to > check one private variable. > > In addition, I think defining TlsCaCertificate in GlobalVariable.h is also > unreasonable. This file should only contain globally defined variables with > gEfiGlobalVariableGuid. What do you think? > > Moreover, Why put the GUID definition in CryptoPkg.dec? It looks so strange. > > Thanks. > Jiaxin > > > > -----Original Message----- > > From: Thomas Palmer [mailto:thomas.pal...@hpe.com] > > Sent: Friday, June 24, 2016 2:14 AM > > To: edk2-devel@lists.01.org > > Cc: samer.el-haj-mahm...@hpe.com; Wu, Jiaxin <jiaxin...@intel.com>; > > Zimmer, Vincent <vincent.zim...@intel.com>; Li, Ruth > > <ruth...@intel.com>; Fu, Siyuan <siyuan...@intel.com>; Ye, Ting > > <ting...@intel.com>; Hsiung, Harry L <harry.l.hsi...@intel.com>; > > Thomas Palmer <thomas.pal...@hpe.com> > > Subject: [PATCH] [edk2-staging/HTTPS-TLS][PATCH]: Centralize TLS var > > cert name and guid > > > > Put the TLS cert variable name define into GlobalVariable.h and create > > a GUID for it in CryptoPkg.dec. Describe the minimum size and expected > > variable attributes in VarCheckUefiLib. > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Thomas Palmer <thomas.pal...@hpe.com> > > --- > > CryptoPkg/CryptoPkg.dec | 5 ++++ > > .../Library/VarCheckUefiLib/VarCheckUefiLib.inf | 3 +++ > > .../VarCheckUefiLib/VarCheckUefiLibNullClass.c | 28 > > +++++++++++++++++++++- > > MdePkg/Include/Guid/GlobalVariable.h | 7 ++++++ > > NetworkPkg/HttpDxe/HttpDxe.inf | 7 +++++- > > NetworkPkg/HttpDxe/HttpsSupport.c | 7 +++--- > > NetworkPkg/HttpDxe/HttpsSupport.h | 11 +-------- > > NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf | 3 +++ > > NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c | 11 ++++----- > > NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.h | 11 +-------- > > 10 files changed, 61 insertions(+), 32 deletions(-) > > > > diff --git a/CryptoPkg/CryptoPkg.dec b/CryptoPkg/CryptoPkg.dec index > > ea02ad7..fe04b7d 100644 > > --- a/CryptoPkg/CryptoPkg.dec > > +++ b/CryptoPkg/CryptoPkg.dec > > @@ -5,6 +5,7 @@ > > # It also provides a test application to test libraries. > > # > > # Copyright (c) 2009 - 2016, Intel Corporation. All rights > > reserved.<BR> > > +# (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR> > > # This program and the accompanying materials # are licensed and > > made available under the terms and conditions of the BSD License # > > which accompanies this distribution. The full text of the license may > > be found at @@ -35,6 +36,10 @@ > > ## > > TlsLib|Include/Library/TlsLib.h > > > > +[Guids] > > + ## GUID used for TLS Certificate verification > > + gEfiTlsCaCertificateGuid = {0xfd2340D0, 0x3dab, 0x4349, {0xa6, > > +0xc7, 0x3b, 0x4f, 0x12, 0xb4, 0x8e, 0xae}} > > + > > [Protocols] > > ## Include/Protocol/RuntimeCrypt.h > > gEfiRuntimeCryptProtocolGuid = { 0xe1475e0c, 0x1746, 0x4802, {0x86, > > 0x2e, 0x1, 0x1c, 0x2c, 0x2d, 0x9d, 0x86 }} diff --git > > a/MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf > > b/MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf > > index 128c44d..945397a 100644 > > --- a/MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf > > +++ b/MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf > > @@ -36,6 +36,7 @@ > > [Packages] > > MdePkg/MdePkg.dec > > MdeModulePkg/MdeModulePkg.dec > > + CryptoPkg/CryptoPkg.dec > > > > [LibraryClasses] > > BaseLib > > @@ -81,6 +82,8 @@ > > ## SOMETIMES_CONSUMES ## Variable:L"SysPrep####" > > ## SOMETIMES_CONSUMES ## Variable:L"Key####" > > gEfiGlobalVariableGuid > > + ## SOMETIMES_CONSUMES ## Variable:L"TlsCaCertificate" > > + gEfiTlsCaCertificateGuid > > ## SOMETIMES_CONSUMES ## Variable:L"DB" > > ## SOMETIMES_CONSUMES ## Variable:L"DBX" > > ## SOMETIMES_CONSUMES ## Variable:L"DBT" > > diff --git > > a/MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLibNullClass.c > > b/MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLibNullClass.c > > index 8f7126e..b820659 100644 > > --- a/MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLibNullClass.c > > +++ > b/MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLibNullClass.c > > @@ -2,6 +2,7 @@ > > Implementation functions and structures for var check uefi library. > > > > Copyright (c) 2015 - 2016, Intel Corporation. All rights > > reserved.<BR> > > +(C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR> > > This program and the accompanying materials are licensed and made > > available under the terms and conditions of the BSD License which > > accompanies this distribution. The full text of the license may be > > found at @@ -671,10 +672,26 @@ UEFI_DEFINED_VARIABLE_ENTRY > > mHwErrRecVariable = { > > NULL > > }; > > > > +// > > +// EFI_TLS_CA_CERTIFICATE_VARIABLE > > +// > > +UEFI_DEFINED_VARIABLE_ENTRY mTlsCaCertificateVariable = { > > + EFI_TLS_CA_CERTIFICATE_VARIABLE, > > + { > > + VAR_CHECK_VARIABLE_PROPERTY_REVISION, > > + 0, > > + VARIABLE_ATTRIBUTE_NV_BS_RT, > > + sizeof (EFI_SIGNATURE_LIST), > > + MAX_UINTN > > + }, > > + NULL > > +}; > > + > > EFI_GUID *mUefiDefinedGuid[] = { > > &gEfiGlobalVariableGuid, > > &gEfiImageSecurityDatabaseGuid, > > - &gEfiHardwareErrorVariableGuid > > + &gEfiHardwareErrorVariableGuid, > > + &gEfiTlsCaCertificateGuid, > > }; > > > > /** > > @@ -915,6 +932,15 @@ VariablePropertySetUefiDefined ( > > &gEfiHardwareErrorVariableGuid, > > &mHwErrRecVariable.VariableProperty > > ); > > + > > + // > > + // EFI_TLS_CA_CERTIFICATE_VARIABLE > > + // > > + VarCheckLibVariablePropertySet ( > > + mTlsCaCertificateVariable.Name, > > + &gEfiTlsCaCertificateGuid, > > + &mTlsCaCertificateVariable.VariableProperty > > + ); > > } > > > > /** > > diff --git a/MdePkg/Include/Guid/GlobalVariable.h > > b/MdePkg/Include/Guid/GlobalVariable.h > > index 0804236..aebf56d 100644 > > --- a/MdePkg/Include/Guid/GlobalVariable.h > > +++ b/MdePkg/Include/Guid/GlobalVariable.h > > @@ -2,6 +2,7 @@ > > GUID for EFI (NVRAM) Variables. > > > > Copyright (c) 2006 - 2016, Intel Corporation. All rights > > reserved.<BR> > > + (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR> > > This program and the accompanying materials > > are licensed and made available under the terms and conditions of > > the BSD License > > which accompanies this distribution. The full text of the license > > may be found at @@ -189,4 +190,10 @@ extern EFI_GUID > > gEfiGlobalVariableGuid; /// > > #define EFI_VENDOR_KEYS_VARIABLE_NAME L"VendorKeys" > > > > +/// > > +/// List of trusted certificates for TLS communication /// Its > > +attribute is NV+BS+RT. > > +/// > > +#define EFI_TLS_CA_CERTIFICATE_VARIABLE L"TlsCaCertificate" > > + > > #endif > > diff --git a/NetworkPkg/HttpDxe/HttpDxe.inf > > b/NetworkPkg/HttpDxe/HttpDxe.inf index a228c3d..3942ce8 100644 > > --- a/NetworkPkg/HttpDxe/HttpDxe.inf > > +++ b/NetworkPkg/HttpDxe/HttpDxe.inf > > @@ -2,6 +2,7 @@ > > # Implementation of EFI HTTP protocol interfaces. > > # > > # Copyright (c) 2015 - 2016, Intel Corporation. All rights > > reserved.<BR> > > +# (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR> > > # > > # This program and the accompanying materials # are licensed and > > made available under the terms and conditions of the BSD License @@ > > -25,6 +26,7 @@ > > > > [Packages] > > MdePkg/MdePkg.dec > > + CryptoPkg/CryptoPkg.dec > > MdeModulePkg/MdeModulePkg.dec > > > > [Sources] > > @@ -53,6 +55,9 @@ > > HttpLib > > DpcLib > > > > +[Guids] > > + gEfiTlsCaCertificateGuid > > + > > [Protocols] > > gEfiHttpServiceBindingProtocolGuid ## BY_START > > gEfiHttpProtocolGuid ## BY_START > > @@ -72,4 +77,4 @@ > > gEfiTlsConfigurationProtocolGuid ## SOMETIMES_CONSUMES > > > > [UserExtensions.TianoCore."ExtraFiles"] > > - HttpDxeExtra.uni > > \ No newline at end of file > > + HttpDxeExtra.uni > > diff --git a/NetworkPkg/HttpDxe/HttpsSupport.c > > b/NetworkPkg/HttpDxe/HttpsSupport.c > > index 09aaa46..b69b157 100644 > > --- a/NetworkPkg/HttpDxe/HttpsSupport.c > > +++ b/NetworkPkg/HttpDxe/HttpsSupport.c > > @@ -2,6 +2,7 @@ > > Miscellaneous routines specific to Https for HttpDxe driver. > > > > Copyright (c) 2016, Intel Corporation. All rights reserved.<BR> > > +(C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR> > > This program and the accompanying materials are licensed and made > > available under the terms and conditions of the BSD License which > > accompanies this distribution. The full text of the license may be > > found at @@ -14,8 +15,6 @@ WITHOUT WARRANTIES OR > REPRESENTATIONS OF > > ANY KIND, EITHER EXPRESS OR IMPLIED. > > > > #include "HttpDriver.h" > > > > -EFI_GUID mEfiTlsCaCertificateGuid = EFI_TLS_CA_CERTIFICATE_GUID; > > - > > /** > > Returns the first occurrence of a Null-terminated ASCII sub-string > > in a Null- terminated > > ASCII string and ignore case during the search process. > > @@ -397,7 +396,7 @@ TlsConfigCertificate ( > > CACertSize = 0; > > Status = gRT->GetVariable ( > > EFI_TLS_CA_CERTIFICATE_VARIABLE, > > - &mEfiTlsCaCertificateGuid, > > + &gEfiTlsCaCertificateGuid, > > NULL, > > &CACertSize, > > NULL > > @@ -414,7 +413,7 @@ TlsConfigCertificate ( > > > > Status = gRT->GetVariable ( > > EFI_TLS_CA_CERTIFICATE_VARIABLE, > > - &mEfiTlsCaCertificateGuid, > > + &gEfiTlsCaCertificateGuid, > > NULL, > > &CACertSize, > > CACert > > diff --git a/NetworkPkg/HttpDxe/HttpsSupport.h > > b/NetworkPkg/HttpDxe/HttpsSupport.h > > index 682a6b6..f6bc5bf 100644 > > --- a/NetworkPkg/HttpDxe/HttpsSupport.h > > +++ b/NetworkPkg/HttpDxe/HttpsSupport.h > > @@ -2,6 +2,7 @@ > > The header files of miscellaneous routines specific to Https for > > HttpDxe driver. > > > > Copyright (c) 2016, Intel Corporation. All rights reserved.<BR> > > +(C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR> > > This program and the accompanying materials are licensed and made > > available under the terms and conditions of the BSD License which > > accompanies this distribution. The full text of the license may be > > found at @@ -22,16 +23,6 @@ WITHOUT WARRANTIES OR > REPRESENTATIONS OF > > ANY KIND, EITHER EXPRESS OR IMPLIED. > > #define HTTPS_FLAG "https" > > > > // > > -// Private variable for CA Certificate configuration -// -#define > > EFI_TLS_CA_CERTIFICATE_GUID \ > > - { \ > > - 0xfd2340D0, 0x3dab, 0x4349, { 0xa6, 0xc7, 0x3b, 0x4f, 0x12, 0xb4, 0x8e, > > 0xae } \ > > - } > > - > > -#define EFI_TLS_CA_CERTIFICATE_VARIABLE L"TlsCaCertificate" > > - > > -// > > // TLS Version > > // > > #define TLS10_PROTOCOL_VERSION_MAJOR 0x03 diff --git > > a/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf > > b/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf > > index dd480a4..7824d3d 100644 > > --- a/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf > > +++ b/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf > > @@ -3,6 +3,7 @@ > > # By this module, user may change the content of TlsCaCertificate. > > # > > # Copyright (c) 2016, Intel Corporation. All rights reserved.<BR> > > +# (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR> > > # This program and the accompanying materials # are licensed and > > made available under the terms and conditions of the BSD License # > > which accompanies this distribution. The full text of the license may > > be found at @@ -30,6 +31,7 @@ > > MdePkg/MdePkg.dec > > MdeModulePkg/MdeModulePkg.dec > > NetworkPkg/NetworkPkg.dec > > + CryptoPkg/CryptoPkg.dec > > > > [Sources] > > TlsAuthConfigImpl.c > > @@ -63,6 +65,7 @@ > > gTlsAuthConfigGuid ## PRODUCES ## GUID > > gEfiCertX509Guid ## CONSUMES ## GUID # > > Indicate the > > cert type > > gEfiIfrTianoGuid ## CONSUMES ## HII > > + gEfiTlsCaCertificateGuid ## CONSUMES ## GUID > > > > [Depex] > > gEfiHiiConfigRoutingProtocolGuid AND diff --git > > a/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c > > b/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c > > index bdf7963..ae9ece8 100644 > > --- a/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c > > +++ b/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c > > @@ -2,6 +2,7 @@ > > The Miscellaneous Routines for TlsAuthConfigDxe driver. > > > > Copyright (c) 2016, Intel Corporation. All rights reserved.<BR> > > +(C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR> > > > > This program and the accompanying materials are licensed and made > > available under the terms and conditions of the BSD License > > @@ -20,8 +21,6 @@ VOID *mEndOpCodeHandle = NULL; > > EFI_IFR_GUID_LABEL *mStartLabel = NULL; > > EFI_IFR_GUID_LABEL *mEndLabel = NULL; > > > > -EFI_GUID mEfiTlsCaCertificateGuid = > > EFI_TLS_CA_CERTIFICATE_GUID; > > - > > CHAR16 mTlsAuthConfigStorageName[] = > > L"TLS_AUTH_CONFIG_IFR_NVDATA"; > > > > TLS_AUTH_CONFIG_PRIVATE_DATA *mTlsAuthPrivateData = NULL; > > @@ -1006,7 +1005,7 @@ EnrollX509toVariable ( > > > > Status = gRT->GetVariable( > > VariableName, > > - &mEfiTlsCaCertificateGuid, > > + &gEfiTlsCaCertificateGuid, > > NULL, > > &DataSize, > > NULL > > @@ -1019,7 +1018,7 @@ EnrollX509toVariable ( > > > > Status = gRT->SetVariable( > > VariableName, > > - &mEfiTlsCaCertificateGuid, > > + &gEfiTlsCaCertificateGuid, > > Attr, > > SigDataSize, > > Data > > @@ -1782,7 +1781,7 @@ TlsAuthConfigAccessCallback ( > > UpdateDeletePage ( > > Private, > > EFI_TLS_CA_CERTIFICATE_VARIABLE, > > - &mEfiTlsCaCertificateGuid, > > + &gEfiTlsCaCertificateGuid, > > LABEL_CA_DELETE, > > TLS_AUTH_CONFIG_FORMID5_FORM, > > OPTION_DEL_CA_ESTION_ID > > @@ -1795,7 +1794,7 @@ TlsAuthConfigAccessCallback ( > > DeleteCert ( > > Private, > > EFI_TLS_CA_CERTIFICATE_VARIABLE, > > - &mEfiTlsCaCertificateGuid, > > + &gEfiTlsCaCertificateGuid, > > LABEL_CA_DELETE, > > TLS_AUTH_CONFIG_FORMID5_FORM, > > OPTION_DEL_CA_ESTION_ID, > > diff --git a/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.h > > b/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.h > > index d08eb16..f73fd61 100644 > > --- a/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.h > > +++ b/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.h > > @@ -2,6 +2,7 @@ > > Header file of Miscellaneous Routines for TlsAuthConfigDxe driver. > > > > Copyright (c) 2016, Intel Corporation. All rights reserved.<BR> > > +(C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR> > > > > This program and the accompanying materials are licensed and made > > available under the terms and conditions of the BSD License @@ -80,16 > > +81,6 @@ struct _TLS_AUTH_CONFIG_PRIVATE_DATA { > > EFI_GUID *CertGuid; > > }; > > > > -// > > -// Private variable for CA Certificate configuration -// -#define > > EFI_TLS_CA_CERTIFICATE_GUID \ > > - { \ > > - 0xfd2340D0, 0x3dab, 0x4349, { 0xa6, 0xc7, 0x3b, 0x4f, 0x12, 0xb4, 0x8e, > > 0xae } \ > > - } > > - > > -#define EFI_TLS_CA_CERTIFICATE_VARIABLE L"TlsCaCertificate" > > - > > /** > > Unload the configuration form, this includes: delete all the > > configuration > > entries, uninstall the form callback protocol, and free the resources > > used. > > -- > > 1.9.1 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
