[PATCH v1] NetworkPkg/TlsDxe: Fix failure to process multiple TLS records.

[PATCH v1] NetworkPkg/TlsDxe: Fix failure to process multiple TLS records.

[Jiaxin Wu]

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1290.

Current implementation failed to parse the multiple TLS record
messages due to the incorrect pointer of TLS record header. This
patch is to resolve that problem.

Cc: Ye Ting <ting.ye@intel.com>
Cc: Fu Siyuan <siyuan.fu@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
---
 NetworkPkg/TlsDxe/TlsImpl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/NetworkPkg/TlsDxe/TlsImpl.c b/NetworkPkg/TlsDxe/TlsImpl.c
index ea83dbd04f..2d4169b0a5 100644
--- a/NetworkPkg/TlsDxe/TlsImpl.c
+++ b/NetworkPkg/TlsDxe/TlsImpl.c
@@ -142,11 +142,11 @@ TlsEncryptPacket (
     }
 
     BufferOutSize += ThisMessageSize;
 
     BufferInPtr += TLS_RECORD_HEADER_LENGTH + ThisPlainMessageSize;
-    TempRecordHeader += ThisMessageSize;
+    TempRecordHeader = (TLS_RECORD_HEADER *)((UINT8 *)TempRecordHeader + ThisMessageSize);
   }
 
   FreePool (BufferIn);
   BufferIn = NULL;
 
@@ -315,11 +315,11 @@ TlsDecryptPacket (
     CopyMem (TempRecordHeader, RecordHeaderIn, TLS_RECORD_HEADER_LENGTH);
     TempRecordHeader->Length = ThisPlainMessageSize;
     BufferOutSize += TLS_RECORD_HEADER_LENGTH + ThisPlainMessageSize;
 
     BufferInPtr += TLS_RECORD_HEADER_LENGTH + ThisCipherMessageSize;
-    TempRecordHeader += TLS_RECORD_HEADER_LENGTH + ThisPlainMessageSize;
+    TempRecordHeader = (TLS_RECORD_HEADER *)((UINT8 *)TempRecordHeader + TLS_RECORD_HEADER_LENGTH + ThisPlainMessageSize);
   }
 
   FreePool (BufferIn);
   BufferIn = NULL;
 
-- 
2.17.1.windows.2

Re: [PATCH v1] NetworkPkg/TlsDxe: Fix failure to process multiple TLS records.

[Fu, Siyuan]

Reviewed-by: Fu Siyuan <siyuan.fu@intel.com>


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Jiaxin Wu
> Sent: Wednesday, October 31, 2018 1:42 PM
> To: edk2-devel@lists.01.org
> Cc: Ye, Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Wu,
> Jiaxin <jiaxin.wu@intel.com>
> Subject: [edk2] [PATCH v1] NetworkPkg/TlsDxe: Fix failure to process
> multiple TLS records.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1290.
> 
> Current implementation failed to parse the multiple TLS record
> messages due to the incorrect pointer of TLS record header. This
> patch is to resolve that problem.
> 
> Cc: Ye Ting <ting.ye@intel.com>
> Cc: Fu Siyuan <siyuan.fu@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
> ---
>  NetworkPkg/TlsDxe/TlsImpl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/NetworkPkg/TlsDxe/TlsImpl.c b/NetworkPkg/TlsDxe/TlsImpl.c
> index ea83dbd04f..2d4169b0a5 100644
> --- a/NetworkPkg/TlsDxe/TlsImpl.c
> +++ b/NetworkPkg/TlsDxe/TlsImpl.c
> @@ -142,11 +142,11 @@ TlsEncryptPacket (
>      }
> 
>      BufferOutSize += ThisMessageSize;
> 
>      BufferInPtr += TLS_RECORD_HEADER_LENGTH + ThisPlainMessageSize;
> -    TempRecordHeader += ThisMessageSize;
> +    TempRecordHeader = (TLS_RECORD_HEADER *)((UINT8 *)TempRecordHeader +
> ThisMessageSize);
>    }
> 
>    FreePool (BufferIn);
>    BufferIn = NULL;
> 
> @@ -315,11 +315,11 @@ TlsDecryptPacket (
>      CopyMem (TempRecordHeader, RecordHeaderIn, TLS_RECORD_HEADER_LENGTH);
>      TempRecordHeader->Length = ThisPlainMessageSize;
>      BufferOutSize += TLS_RECORD_HEADER_LENGTH + ThisPlainMessageSize;
> 
>      BufferInPtr += TLS_RECORD_HEADER_LENGTH + ThisCipherMessageSize;
> -    TempRecordHeader += TLS_RECORD_HEADER_LENGTH + ThisPlainMessageSize;
> +    TempRecordHeader = (TLS_RECORD_HEADER *)((UINT8 *)TempRecordHeader +
> TLS_RECORD_HEADER_LENGTH + ThisPlainMessageSize);
>    }
> 
>    FreePool (BufferIn);
>    BufferIn = NULL;
> 
> --
> 2.17.1.windows.2
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH v1] NetworkPkg/TlsDxe: Fix failure to process multiple TLS records.

[Laszlo Ersek]

On 10/31/18 06:42, Jiaxin Wu wrote:
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1290.
> 
> Current implementation failed to parse the multiple TLS record
> messages due to the incorrect pointer of TLS record header. This
> patch is to resolve that problem.
> 
> Cc: Ye Ting <ting.ye@intel.com>
> Cc: Fu Siyuan <siyuan.fu@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
> ---
>  NetworkPkg/TlsDxe/TlsImpl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/NetworkPkg/TlsDxe/TlsImpl.c b/NetworkPkg/TlsDxe/TlsImpl.c
> index ea83dbd04f..2d4169b0a5 100644
> --- a/NetworkPkg/TlsDxe/TlsImpl.c
> +++ b/NetworkPkg/TlsDxe/TlsImpl.c
> @@ -142,11 +142,11 @@ TlsEncryptPacket (
>      }
>  
>      BufferOutSize += ThisMessageSize;
>  
>      BufferInPtr += TLS_RECORD_HEADER_LENGTH + ThisPlainMessageSize;
> -    TempRecordHeader += ThisMessageSize;
> +    TempRecordHeader = (TLS_RECORD_HEADER *)((UINT8 *)TempRecordHeader + ThisMessageSize);
>    }
>  
>    FreePool (BufferIn);
>    BufferIn = NULL;
>  
> @@ -315,11 +315,11 @@ TlsDecryptPacket (
>      CopyMem (TempRecordHeader, RecordHeaderIn, TLS_RECORD_HEADER_LENGTH);
>      TempRecordHeader->Length = ThisPlainMessageSize;
>      BufferOutSize += TLS_RECORD_HEADER_LENGTH + ThisPlainMessageSize;
>  
>      BufferInPtr += TLS_RECORD_HEADER_LENGTH + ThisCipherMessageSize;
> -    TempRecordHeader += TLS_RECORD_HEADER_LENGTH + ThisPlainMessageSize;
> +    TempRecordHeader = (TLS_RECORD_HEADER *)((UINT8 *)TempRecordHeader + TLS_RECORD_HEADER_LENGTH + ThisPlainMessageSize);
>    }
>  
>    FreePool (BufferIn);
>    BufferIn = NULL;
>  
> 

What is the practical impact of this issue? In what scenario was it
encountered? What were the symptoms?

I realize the patch may have been pushed by now -- please consider
adding the information to the BZ. (Or, please answer here, and then add
the message URL to the BZ.)

Thanks
Laszlo

Re: [PATCH v1] NetworkPkg/TlsDxe: Fix failure to process multiple TLS records.

[Wu, Jiaxin]

Already share the info @ https://bugzilla.tianocore.org/show_bug.cgi?id=1290

Thanks,
Jiaxin

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, November 6, 2018 2:33 AM
> To: Wu, Jiaxin <jiaxin.wu@intel.com>; edk2-devel@lists.01.org
> Cc: Ye, Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> Subject: Re: [edk2] [PATCH v1] NetworkPkg/TlsDxe: Fix failure to process
> multiple TLS records.
> 
> On 10/31/18 06:42, Jiaxin Wu wrote:
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1290.
> >
> > Current implementation failed to parse the multiple TLS record
> > messages due to the incorrect pointer of TLS record header. This
> > patch is to resolve that problem.
> >
> > Cc: Ye Ting <ting.ye@intel.com>
> > Cc: Fu Siyuan <siyuan.fu@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
> > ---
> >  NetworkPkg/TlsDxe/TlsImpl.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/NetworkPkg/TlsDxe/TlsImpl.c b/NetworkPkg/TlsDxe/TlsImpl.c
> > index ea83dbd04f..2d4169b0a5 100644
> > --- a/NetworkPkg/TlsDxe/TlsImpl.c
> > +++ b/NetworkPkg/TlsDxe/TlsImpl.c
> > @@ -142,11 +142,11 @@ TlsEncryptPacket (
> >      }
> >
> >      BufferOutSize += ThisMessageSize;
> >
> >      BufferInPtr += TLS_RECORD_HEADER_LENGTH + ThisPlainMessageSize;
> > -    TempRecordHeader += ThisMessageSize;
> > +    TempRecordHeader = (TLS_RECORD_HEADER *)((UINT8
> *)TempRecordHeader + ThisMessageSize);
> >    }
> >
> >    FreePool (BufferIn);
> >    BufferIn = NULL;
> >
> > @@ -315,11 +315,11 @@ TlsDecryptPacket (
> >      CopyMem (TempRecordHeader, RecordHeaderIn,
> TLS_RECORD_HEADER_LENGTH);
> >      TempRecordHeader->Length = ThisPlainMessageSize;
> >      BufferOutSize += TLS_RECORD_HEADER_LENGTH +
> ThisPlainMessageSize;
> >
> >      BufferInPtr += TLS_RECORD_HEADER_LENGTH + ThisCipherMessageSize;
> > -    TempRecordHeader += TLS_RECORD_HEADER_LENGTH +
> ThisPlainMessageSize;
> > +    TempRecordHeader = (TLS_RECORD_HEADER *)((UINT8
> *)TempRecordHeader + TLS_RECORD_HEADER_LENGTH +
> ThisPlainMessageSize);
> >    }
> >
> >    FreePool (BufferIn);
> >    BufferIn = NULL;
> >
> >
> 
> What is the practical impact of this issue? In what scenario was it
> encountered? What were the symptoms?
> 
> I realize the patch may have been pushed by now -- please consider
> adding the information to the BZ. (Or, please answer here, and then add
> the message URL to the BZ.)
> 
> Thanks
> Laszlo

Re: [PATCH v1] NetworkPkg/TlsDxe: Fix failure to process multiple TLS records.

[Laszlo Ersek]

On 11/06/18 03:13, Wu, Jiaxin wrote:
> Already share the info @ https://bugzilla.tianocore.org/show_bug.cgi?id=1290

Awesome, thanks!
Laszlo

>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Tuesday, November 6, 2018 2:33 AM
>> To: Wu, Jiaxin <jiaxin.wu@intel.com>; edk2-devel@lists.01.org
>> Cc: Ye, Ting <ting.ye@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
>> Subject: Re: [edk2] [PATCH v1] NetworkPkg/TlsDxe: Fix failure to process
>> multiple TLS records.
>>
>> On 10/31/18 06:42, Jiaxin Wu wrote:
>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1290.
>>>
>>> Current implementation failed to parse the multiple TLS record
>>> messages due to the incorrect pointer of TLS record header. This
>>> patch is to resolve that problem.
>>>
>>> Cc: Ye Ting <ting.ye@intel.com>
>>> Cc: Fu Siyuan <siyuan.fu@intel.com>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
>>> ---
>>>  NetworkPkg/TlsDxe/TlsImpl.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/NetworkPkg/TlsDxe/TlsImpl.c b/NetworkPkg/TlsDxe/TlsImpl.c
>>> index ea83dbd04f..2d4169b0a5 100644
>>> --- a/NetworkPkg/TlsDxe/TlsImpl.c
>>> +++ b/NetworkPkg/TlsDxe/TlsImpl.c
>>> @@ -142,11 +142,11 @@ TlsEncryptPacket (
>>>      }
>>>
>>>      BufferOutSize += ThisMessageSize;
>>>
>>>      BufferInPtr += TLS_RECORD_HEADER_LENGTH + ThisPlainMessageSize;
>>> -    TempRecordHeader += ThisMessageSize;
>>> +    TempRecordHeader = (TLS_RECORD_HEADER *)((UINT8
>> *)TempRecordHeader + ThisMessageSize);
>>>    }
>>>
>>>    FreePool (BufferIn);
>>>    BufferIn = NULL;
>>>
>>> @@ -315,11 +315,11 @@ TlsDecryptPacket (
>>>      CopyMem (TempRecordHeader, RecordHeaderIn,
>> TLS_RECORD_HEADER_LENGTH);
>>>      TempRecordHeader->Length = ThisPlainMessageSize;
>>>      BufferOutSize += TLS_RECORD_HEADER_LENGTH +
>> ThisPlainMessageSize;
>>>
>>>      BufferInPtr += TLS_RECORD_HEADER_LENGTH + ThisCipherMessageSize;
>>> -    TempRecordHeader += TLS_RECORD_HEADER_LENGTH +
>> ThisPlainMessageSize;
>>> +    TempRecordHeader = (TLS_RECORD_HEADER *)((UINT8
>> *)TempRecordHeader + TLS_RECORD_HEADER_LENGTH +
>> ThisPlainMessageSize);
>>>    }
>>>
>>>    FreePool (BufferIn);
>>>    BufferIn = NULL;
>>>
>>>
>>
>> What is the practical impact of this issue? In what scenario was it
>> encountered? What were the symptoms?
>>
>> I realize the patch may have been pushed by now -- please consider
>> adding the information to the BZ. (Or, please answer here, and then add
>> the message URL to the BZ.)
>>
>> Thanks
>> Laszlo