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

[Laszlo Ersek <lersek@redhat.com>]

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