From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 9168B223DB78A for ; Fri, 9 Feb 2018 02:10:27 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 201AF62E8B; Fri, 9 Feb 2018 10:16:13 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-73.phx2.redhat.com [10.3.116.73]) by smtp.corp.redhat.com (Postfix) with ESMTP id 524156A844; Fri, 9 Feb 2018 10:16:11 +0000 (UTC) To: Jiaxin Wu , edk2-devel@lists.01.org Cc: Kinney Michael D , Zimmer Vincent , Yao Jiewen , Ye Ting , Fu Siyuan References: <1518148778-14300-1-git-send-email-jiaxin.wu@intel.com> <1518148778-14300-3-git-send-email-jiaxin.wu@intel.com> From: Laszlo Ersek Message-ID: <8ca3dab4-dd8a-0e05-a796-33203f35006b@redhat.com> Date: Fri, 9 Feb 2018 11:16:10 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <1518148778-14300-3-git-send-email-jiaxin.wu@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Fri, 09 Feb 2018 10:16:13 +0000 (UTC) Subject: Re: [Patch 2/2] NetworkPkg: Read TlsCipherList variable and configure it for HTTPS session. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 09 Feb 2018 10:10:28 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 02/09/18 04:59, Jiaxin Wu wrote: > This patch is to read the TlsCipherList variable and configure it for the > later HTTPS session. > > If the variable is not set by any platform, EFI_NOT_FOUND will be returned > from GetVariable service. In such a case, the default CipherList created in > TlsDxe driver will be used. > > Cc: Laszlo Ersek > Cc: Kinney Michael D > Cc: Zimmer Vincent > Cc: Yao Jiewen > Cc: Ye Ting > Cc: Fu Siyuan > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Wu Jiaxin > --- > NetworkPkg/HttpDxe/HttpDriver.h | 3 +- > NetworkPkg/HttpDxe/HttpDxe.inf | 3 +- > NetworkPkg/HttpDxe/HttpsSupport.c | 92 ++++++++++++++++++++++++++++++++++++++- > 3 files changed, 95 insertions(+), 3 deletions(-) > > diff --git a/NetworkPkg/HttpDxe/HttpDriver.h b/NetworkPkg/HttpDxe/HttpDriver.h > index 93a412a..eba7d32 100644 > --- a/NetworkPkg/HttpDxe/HttpDriver.h > +++ b/NetworkPkg/HttpDxe/HttpDriver.h > @@ -1,9 +1,9 @@ > /** @file > The header files of the driver binding and service binding protocol for HttpDxe driver. > > - Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.
> + Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.
> (C) Copyright 2016 Hewlett Packard Enterprise Development LP
> > 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 > @@ -59,10 +59,11 @@ > // Produced Protocols > // > #include > > #include > +#include > > #include > > // > // Driver Version > diff --git a/NetworkPkg/HttpDxe/HttpDxe.inf b/NetworkPkg/HttpDxe/HttpDxe.inf > index 20075f5..b1d7bd2 100644 > --- a/NetworkPkg/HttpDxe/HttpDxe.inf > +++ b/NetworkPkg/HttpDxe/HttpDxe.inf > @@ -1,9 +1,9 @@ > ## @file > # Implementation of EFI HTTP protocol interfaces. > # > -# Copyright (c) 2015 - 2017, Intel Corporation. All rights reserved.
> +# Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.
> # > # 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 > # http://opensource.org/licenses/bsd-license.php. > @@ -72,10 +72,11 @@ > gEfiTlsProtocolGuid ## SOMETIMES_CONSUMES > gEfiTlsConfigurationProtocolGuid ## SOMETIMES_CONSUMES > > [Guids] > gEfiTlsCaCertificateGuid ## SOMETIMES_CONSUMES ## Variable:L"TlsCaCertificate" > + gTlsCipherListGuid ## SOMETIMES_CONSUMES ## Variable:L"TlsCipherList" > > [Pcd] > gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections ## CONSUMES > gEfiNetworkPkgTokenSpaceGuid.PcdHttpsAuthenticationMode ## SOMETIMES_CONSUMES > gEfiNetworkPkgTokenSpaceGuid.PcdHttpsHostPublicCert ## SOMETIMES_CONSUMES > diff --git a/NetworkPkg/HttpDxe/HttpsSupport.c b/NetworkPkg/HttpDxe/HttpsSupport.c > index 288082a..62cb867 100644 > --- a/NetworkPkg/HttpDxe/HttpsSupport.c > +++ b/NetworkPkg/HttpDxe/HttpsSupport.c > @@ -1,9 +1,9 @@ > /** @file > Miscellaneous routines specific to Https for HttpDxe driver. > > -Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved.
> +Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.
> (C) Copyright 2016 Hewlett Packard Enterprise Development LP
> 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 > http://opensource.org/licenses/bsd-license.php > @@ -492,10 +492,91 @@ TlsConfigCertificate ( > > return Status; > } > > /** > + Read the TlsCipherList variable and configure it for HTTPS session. > + > + @param[in, out] HttpInstance The HTTP instance private data. > + > + @retval EFI_SUCCESS The prefered TLS CipherList is configured. > + @retval EFI_NOT_FOUND Fail to get 'TlsCipherList' variable. > + @retval EFI_INVALID_PARAMETER The contents of variable are invalid. > + @retval EFI_OUT_OF_RESOURCES Can't allocate memory resources. > + > + @retval Others Other error as indicated. > + > +**/ > +EFI_STATUS > +TlsConfigCipherList ( > + IN OUT HTTP_PROTOCOL *HttpInstance > + ) > +{ > + EFI_STATUS Status; > + UINT8 *CipherList; > + UINTN CipherListSize; > + > + CipherList = NULL; > + CipherListSize = 0; > + > + // > + // Try to read the TlsCipherList variable. > + // > + Status = gRT->GetVariable ( > + EDKII_TLS_CIPHER_LIST_VARIABLE, > + &gTlsCipherListGuid, > + NULL, > + &CipherListSize, > + NULL > + ); > + > + if (EFI_ERROR (Status) && Status != EFI_BUFFER_TOO_SMALL) { > + return Status; > + } I think the above GetVariable service call can never succeed. What about: ASSERT (EFI_ERROR (Status)); if (Status != EFI_BUFFER_TOO_SMALL) { return Status; } Not very important (the behavior won't change), but I think this is better for documentation purposes. The patch looks good to me otherwise. Thank you! Laszlo > + > + if (CipherListSize % sizeof (EFI_TLS_CIPHER) != 0) { > + return EFI_INVALID_PARAMETER; > + } > + > + // > + // Allocate buffer and read the config variable. > + // > + CipherList = AllocatePool (CipherListSize); > + if (CipherList == NULL) { > + return EFI_OUT_OF_RESOURCES; > + } > + > + Status = gRT->GetVariable ( > + EDKII_TLS_CIPHER_LIST_VARIABLE, > + &gTlsCipherListGuid, > + NULL, > + &CipherListSize, > + CipherList > + ); > + if (EFI_ERROR (Status)) { > + // > + // GetVariable still error or the variable is corrupted. > + // > + goto ON_EXIT; > + } > + > + ASSERT (CipherList != NULL); > + > + Status = HttpInstance->Tls->SetSessionData ( > + HttpInstance->Tls, > + EfiTlsCipherList, > + CipherList, > + CipherListSize > + ); > + > +ON_EXIT: > + FreePool (CipherList); > + > + return Status; > +} > + > +/** > Configure TLS session data. > > @param[in, out] HttpInstance The HTTP instance private data. > > @retval EFI_SUCCESS TLS session data is configured. > @@ -551,10 +632,19 @@ TlsConfigureSession ( > if (EFI_ERROR (Status)) { > return Status; > } > > // > + // Tls Cipher List > + // > + Status = TlsConfigCipherList (HttpInstance); > + if (EFI_ERROR (Status) && Status != EFI_NOT_FOUND) { > + DEBUG ((EFI_D_ERROR, "TlsConfigCipherList: return %r error.\n", Status)); > + return Status; > + } > + > + // > // Tls Config Certificate > // > Status = TlsConfigCertificate (HttpInstance); > if (EFI_ERROR (Status)) { > DEBUG ((EFI_D_ERROR, "TlsConfigCertificate: return %r error.\n", Status)); >