I can create the patch if you tell me where to put everything. Or if you are like me, may be easier for you to just code it up. Either way is fine
Thomas -----Original Message----- From: Wu, Jiaxin [mailto:[email protected]] Sent: Sunday, June 26, 2016 8:38 PM To: Palmer, Thomas <[email protected]>; [email protected] Cc: El-Haj-Mahmoud, Samer <[email protected]>; Zimmer, Vincent <[email protected]>; Li, Ruth <[email protected]>; Fu, Siyuan <[email protected]>; Ye, Ting <[email protected]>; Hsiung, Harry L <[email protected]> Subject: RE: [PATCH] [edk2-staging/HTTPS-TLS][PATCH]: Centralize TLS var cert name and guid > -----Original Message----- > From: Palmer, Thomas [mailto:[email protected]] > Sent: Saturday, June 25, 2016 12:51 AM > To: Wu, Jiaxin <[email protected]>; [email protected] > Cc: El-Haj-Mahmoud, Samer <[email protected]>; Zimmer, > Vincent <[email protected]>; Li, Ruth <[email protected]>; Fu, > Siyuan <[email protected]>; Ye, Ting <[email protected]>; Hsiung, > Harry L <[email protected]> > 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:[email protected]] > Sent: Thursday, June 23, 2016 9:56 PM > To: Palmer, Thomas <[email protected]>; [email protected] > Cc: El-Haj-Mahmoud, Samer <[email protected]>; Zimmer, > Vincent <[email protected]>; Li, Ruth <[email protected]>; Fu, > Siyuan <[email protected]>; Ye, Ting <[email protected]>; Hsiung, > Harry L <[email protected]> > 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:[email protected]] > > Sent: Friday, June 24, 2016 2:14 AM > > To: [email protected] > > Cc: [email protected]; Wu, Jiaxin <[email protected]>; > > Zimmer, Vincent <[email protected]>; Li, Ruth > > <[email protected]>; Fu, Siyuan <[email protected]>; Ye, Ting > > <[email protected]>; Hsiung, Harry L <[email protected]>; > > Thomas Palmer <[email protected]> > > 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 <[email protected]> > > --- > > 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 [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
