Re: [PATCH 1/2] [edk2-staging/HTTPS-TLS][PATCH]: CryptoPkg/TlsLib: TLS Ver negotiate

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

Thomas,

Sorry for the delayed response. The patch is good to me, only one comments:

> +  TlsCtx = SSL_CTX_new (SSLv23_client_method ());  if (TlsCtx == NULL)
> + {
> +    ASSERT (TlsCtx != NULL);
> +    return NULL;
> +  }

I think we can remove the assert. Return NULL is fine here.

Reviewed-by: Wu Jiaxin <jiaxin.wu@intel.com>

Thanks,
Jiaxin

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Thomas Palmer
> Sent: Saturday, August 27, 2016 3:09 AM
> To: edk2-devel@lists.01.org
> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>
> Subject: [edk2] [PATCH 1/2] [edk2-staging/HTTPS-TLS][PATCH]:
> CryptoPkg/TlsLib: TLS Ver negotiate
> 
> The TLS protocol allows for clients and servers to negotiate which version of
> TLS to use.  Newer versions are deemed safer, so when they are available the
> client and server should opt to use them.
> 
> The EDK2 TLS code today only allows TLSv1.0 for TLS communication,
> regardless of the target server's capabilities. In order to use the newer
> protocols, we'll update the EDK2 TlsLib.c code to allow for TLS version
> negotiation when a new TLS object is created. The TLS version specified in
> TlsCtxNew will be the minimum version accepted.
> 
> Because EDK2 is not yet using OpenSSL 1.1, we use SSL_set_options to
> simulate SSL_CTX_set_min_proto_version.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Thomas Palmer <thomas.palmer@hpe.com>
> ---
>  CryptoPkg/Library/TlsLib/TlsLib.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/CryptoPkg/Library/TlsLib/TlsLib.c
> b/CryptoPkg/Library/TlsLib/TlsLib.c
> index aa08595..0ff699b 100644
> --- a/CryptoPkg/Library/TlsLib/TlsLib.c
> +++ b/CryptoPkg/Library/TlsLib/TlsLib.c
> @@ -195,26 +195,39 @@ TlsCtxNew (
> 
>    ProtoVersion = (MajorVer << 8) | MinorVer;
> 
> -  TlsCtx = NULL;
> +  TlsCtx = SSL_CTX_new (SSLv23_client_method ());  if (TlsCtx == NULL)
> + {
> +    ASSERT (TlsCtx != NULL);
> +    return NULL;
> +  }
> +
> +  //
> +  // Ensure SSLv3 is disabled
> +  //
> +  SSL_CTX_set_options (TlsCtx, SSL_OP_NO_SSLv3);
> 
> +  //
> +  // Treat as minimum accepted versions.  Client can use higher  // TLS
> + version if server supports it  //
>    switch (ProtoVersion) {
>    case TLS1_VERSION:
>      //
>      // TLS 1.0
>      //
> -    TlsCtx = SSL_CTX_new (TLSv1_method ());
>      break;
>    case TLS1_1_VERSION:
>      //
>      // TLS 1.1
>      //
> -    TlsCtx = SSL_CTX_new (TLSv1_1_method ());
> +    SSL_CTX_set_options (TlsCtx, SSL_OP_NO_TLSv1);
>      break;
>    case TLS1_2_VERSION:
>      //
>      // TLS 1.2
>      //
> -    TlsCtx = SSL_CTX_new (TLSv1_2_method ());
> +    SSL_CTX_set_options (TlsCtx, SSL_OP_NO_TLSv1);
> +    SSL_CTX_set_options (TlsCtx, SSL_OP_NO_TLSv1_1);
>      break;
>    default:
>      //
> --
> 2.7.4
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel