[PATCH] NetworkPkg/HttpDxe: Clear TlsChildHandle during cleanup

[PATCH] NetworkPkg/HttpDxe: Clear TlsChildHandle during cleanup

[D Scott Phillips]

From: D Scott Phillips <d.scott.phillips@amperecomputing.com>

Leaving TlsChildHandle with the stale handle causes later use of https
with the http instance to incorrectly skip tls reconfiguration, use
the stale handle, and eventually call a garbage function pointer.

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1917
Signed-off-by: D Scott Phillips <d.scott.phillips@amperecomputing.com>
---
 NetworkPkg/HttpDxe/HttpProto.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/NetworkPkg/HttpDxe/HttpProto.c b/NetworkPkg/HttpDxe/HttpProto.c
index 3c7c6ff9f0..afc7db5a72 100644
--- a/NetworkPkg/HttpDxe/HttpProto.c
+++ b/NetworkPkg/HttpDxe/HttpProto.c
@@ -873,6 +873,7 @@ HttpCleanProtocol (
     // Destroy the TLS instance.
     //
     HttpInstance->TlsSb->DestroyChild (HttpInstance->TlsSb, HttpInstance->TlsChildHandle);
+    HttpInstance->TlsChildHandle = NULL;
   }
 
   if (HttpInstance->Tcp4ChildHandle != NULL) {
-- 
2.26.2

Re: [edk2-devel] [PATCH] NetworkPkg/HttpDxe: Clear TlsChildHandle during cleanup

[Laszlo Ersek]

Hi Scott,

(+Rebecca)

On 09/05/20 03:15, D Scott Phillips wrote:
> From: D Scott Phillips <d.scott.phillips@amperecomputing.com>
> 
> Leaving TlsChildHandle with the stale handle causes later use of https
> with the http instance to incorrectly skip tls reconfiguration, use
> the stale handle, and eventually call a garbage function pointer.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1917
> Signed-off-by: D Scott Phillips <d.scott.phillips@amperecomputing.com>
> ---
>  NetworkPkg/HttpDxe/HttpProto.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/NetworkPkg/HttpDxe/HttpProto.c b/NetworkPkg/HttpDxe/HttpProto.c
> index 3c7c6ff9f0..afc7db5a72 100644
> --- a/NetworkPkg/HttpDxe/HttpProto.c
> +++ b/NetworkPkg/HttpDxe/HttpProto.c
> @@ -873,6 +873,7 @@ HttpCleanProtocol (
>      // Destroy the TLS instance.
>      //
>      HttpInstance->TlsSb->DestroyChild (HttpInstance->TlsSb, HttpInstance->TlsChildHandle);
> +    HttpInstance->TlsChildHandle = NULL;
>    }
>  
>    if (HttpInstance->Tcp4ChildHandle != NULL) {
> 

thanks a lot for tracking this down!

I've reopened BZ#1917, and linked your patch email in a new comment.

But, I'd also like to assign the BZ to you, if that's OK with you. Can
you please register in the TianoCore bugzilla instance for that?

Thanks!
Laszlo

Re: [edk2-devel] [PATCH] NetworkPkg/HttpDxe: Clear TlsChildHandle during cleanup

[D Scott Phillips]

On Monday, September 7, 2020 4:33 AM, Laszlo Ersek <lersek@redhat.com> wrote:

> Hi Scott,
>
> (+Rebecca)
>
> On 09/05/20 03:15, D Scott Phillips wrote:
>
> > From: D Scott Phillips d.scott.phillips@amperecomputing.com
> > Leaving TlsChildHandle with the stale handle causes later use of https
> > with the http instance to incorrectly skip tls reconfiguration, use
> > the stale handle, and eventually call a garbage function pointer.
> >
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1917
> > Signed-off-by: D Scott Phillips d.scott.phillips@amperecomputing.com
> >
> > ------------------------------------------------------------------------------------------------------------------------------
> >
> > NetworkPkg/HttpDxe/HttpProto.c | 1 +
> > 1 file changed, 1 insertion(+)
> > diff --git a/NetworkPkg/HttpDxe/HttpProto.c b/NetworkPkg/HttpDxe/HttpProto.c
> > index 3c7c6ff9f0..afc7db5a72 100644
> > --- a/NetworkPkg/HttpDxe/HttpProto.c
> > +++ b/NetworkPkg/HttpDxe/HttpProto.c
> > @@ -873,6 +873,7 @@ HttpCleanProtocol (
> > // Destroy the TLS instance.
> > //
> > HttpInstance->TlsSb->DestroyChild (HttpInstance->TlsSb, HttpInstance->TlsChildHandle);
> >
> > -   HttpInstance->TlsChildHandle = NULL;
> >     }
> >
> >
> > if (HttpInstance->Tcp4ChildHandle != NULL) {
>
> thanks a lot for tracking this down!
>
> I've reopened BZ#1917, and linked your patch email in a new comment.
>
> But, I'd also like to assign the BZ to you, if that's OK with you. Can
> you please register in the TianoCore bugzilla instance for that?

Certainly, account created and assignment taken. Thanks Laszlo.

Scott

Re: [edk2-devel] [PATCH] NetworkPkg/HttpDxe: Clear TlsChildHandle during cleanup

[Maciej Rabeda]

[-- Attachment #1: Type: text/plain, Size: 2189 bytes --]

Hi Scott,

Thanks for submitting the patch - I am about to approve the patch, 
however - a couple of small remarks to the commit message.
1. Please remove the "From:" line
2. Please add "Cc:" lines before "Signed-off-by". Cc people are added 
based on appropriate EDK2 package maintainer & reviewer list: 
https://github.com/tianocore/edk2/blob/master/Maintainers.txt
Example of a patch from NetworkPkg: 
https://github.com/tianocore/edk2/commit/0716b2390f005e84961cb98af28bd16cdcc5db42

Thanks,
Maciej

On 08-Sep-20 06:50, D Scott Phillips wrote:
> On Monday, September 7, 2020 4:33 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>
>> Hi Scott,
>>
>> (+Rebecca)
>>
>> On 09/05/20 03:15, D Scott Phillips wrote:
>>
>>> *From: D Scott Phillips d.scott.phillips@amperecomputing.com*
>>> Leaving TlsChildHandle with the stale handle causes later use of https
>>> with the http instance to incorrectly skip tls reconfiguration, use
>>> the stale handle, and eventually call a garbage function pointer.
>>>
>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1917
>>> Signed-off-by: D Scott Phillips d.scott.phillips@amperecomputing.com
>>>
>>> ------------------------------------------------------------------------------------------------------------------------------
>>>
>>> NetworkPkg/HttpDxe/HttpProto.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>> diff --git a/NetworkPkg/HttpDxe/HttpProto.c b/NetworkPkg/HttpDxe/HttpProto.c
>>> index 3c7c6ff9f0..afc7db5a72 100644
>>> --- a/NetworkPkg/HttpDxe/HttpProto.c
>>> +++ b/NetworkPkg/HttpDxe/HttpProto.c
>>> @@ -873,6 +873,7 @@ HttpCleanProtocol (
>>> // Destroy the TLS instance.
>>> //
>>> HttpInstance->TlsSb->DestroyChild (HttpInstance->TlsSb, HttpInstance->TlsChildHandle);
>>>
>>> -   HttpInstance->TlsChildHandle = NULL;
>>>      }
>>>
>>>
>>> if (HttpInstance->Tcp4ChildHandle != NULL) {
>> thanks a lot for tracking this down!
>>
>> I've reopened BZ#1917, and linked your patch email in a new comment.
>>
>> But, I'd also like to assign the BZ to you, if that's OK with you. Can
>> you please register in the TianoCore bugzilla instance for that?
> Certainly, account created and assignment taken. Thanks Laszlo.
>
> Scott
>
>
> 
>


[-- Attachment #2: Type: text/html, Size: 3494 bytes --]