Reviewed-by: Siyuan Fu <siyuan...@intel.com> > -----Original Message----- > From: Wu, Jiaxin > Sent: Tuesday, February 26, 2019 4:14 PM > To: edk2-devel@lists.01.org > Cc: Ye, Ting <ting...@intel.com>; Fu, Siyuan <siyuan...@intel.com>; Wang, Fan > <fan.w...@intel.com>; Wu, Jiaxin <jiaxin...@intel.com> > Subject: [PATCH v1] NetworkPkg/DnsDxe: Check the received packet size before > parsing the message. > > Fix CVE-2018-12178 > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=809 > > The DNS driver only checks the received packet size against the > minimum DNS header size in DnsOnPacketReceived(), later it accesses > the QueryName and QuerySection beyond the header scope, which might > cause the pointer within DNS driver points to an invalid entry or > modifies the memory content beyond the header scope. > > This patch is to fix above problem. > > Cc: Ye Ting <ting...@intel.com> > Cc: Fu Siyuan <siyuan...@intel.com> > Cc: Wang Fan <fan.w...@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Wu Jiaxin <jiaxin...@intel.com> > --- > NetworkPkg/DnsDxe/DnsImpl.c | 77 ++++++++++++++++++++++++++++++++----- > NetworkPkg/DnsDxe/DnsImpl.h | 2 + > 2 files changed, 69 insertions(+), 10 deletions(-) > > diff --git a/NetworkPkg/DnsDxe/DnsImpl.c b/NetworkPkg/DnsDxe/DnsImpl.c > index 89ea755cb2..26a718987c 100644 > --- a/NetworkPkg/DnsDxe/DnsImpl.c > +++ b/NetworkPkg/DnsDxe/DnsImpl.c > @@ -1112,26 +1112,29 @@ IsValidDnsResponse ( > /** > Parse Dns Response. > > @param Instance The DNS instance > @param RxString Received buffer. > + @param Length Received buffer length. > @param Completed Flag to indicate that Dns response is valid. > > @retval EFI_SUCCESS Parse Dns Response successfully. > @retval Others Failed to parse Dns Response. > > **/ > EFI_STATUS > ParseDnsResponse ( > IN OUT DNS_INSTANCE *Instance, > IN UINT8 *RxString, > + IN UINT32 Length, > OUT BOOLEAN *Completed > ) > { > DNS_HEADER *DnsHeader; > > CHAR8 *QueryName; > + UINT32 QueryNameLen; > DNS_QUERY_SECTION *QuerySection; > > CHAR8 *AnswerName; > DNS_ANSWER_SECTION *AnswerSection; > UINT8 *AnswerData; > @@ -1153,10 +1156,11 @@ ParseDnsResponse ( > > DNS_RESOURCE_RECORD *Dns4RR; > DNS6_RESOURCE_RECORD *Dns6RR; > > EFI_STATUS Status; > + UINT32 RemainingLength; > > EFI_TPL OldTpl; > > Item = NULL; > Dns4TokenEntry = NULL; > @@ -1176,10 +1180,21 @@ ParseDnsResponse ( > Dns4RR = NULL; > Dns6RR = NULL; > > *Completed = TRUE; > Status = EFI_SUCCESS; > + RemainingLength = Length; > + > + // > + // Check whether the remaining packet length is avaiable or not. > + // > + if (RemainingLength <= sizeof (DNS_HEADER)) { > + *Completed = FALSE; > + return EFI_ABORTED; > + } else { > + RemainingLength -= sizeof (DNS_HEADER); > + } > > // > // Get header > // > DnsHeader = (DNS_HEADER *) RxString; > @@ -1189,26 +1204,42 @@ ParseDnsResponse ( > DnsHeader->QuestionsNum = NTOHS (DnsHeader->QuestionsNum); > DnsHeader->AnswersNum = NTOHS (DnsHeader->AnswersNum); > DnsHeader->AuthorityNum = NTOHS (DnsHeader->AuthorityNum); > DnsHeader->AditionalNum = NTOHS (DnsHeader->AditionalNum); > > + // > + // There is always one QuestionsNum in DNS message. The capability to > handle more > + // than one requires to redesign the message format. Currently, it's not > supported. > + // > + if (DnsHeader->QuestionsNum > 1) { > + *Completed = FALSE; > + return EFI_UNSUPPORTED; > + } > + > // > // Get Query name > // > QueryName = (CHAR8 *) (RxString + sizeof (*DnsHeader)); > > + QueryNameLen = (UINT32) AsciiStrLen (QueryName) + 1; > + > // > - // Get query section > + // Check whether the remaining packet length is avaiable or not. > // > - QuerySection = (DNS_QUERY_SECTION *) (QueryName + AsciiStrLen (QueryName) + > 1); > - QuerySection->Type = NTOHS (QuerySection->Type); > - QuerySection->Class = NTOHS (QuerySection->Class); > + if (RemainingLength <= QueryNameLen + sizeof (DNS_QUERY_SECTION)) { > + *Completed = FALSE; > + return EFI_ABORTED; > + } else { > + RemainingLength -= (QueryNameLen + sizeof (DNS_QUERY_SECTION)); > + } > > // > - // Get Answer name > + // Get query section > // > - AnswerName = (CHAR8 *) QuerySection + sizeof (*QuerySection); > + QuerySection = (DNS_QUERY_SECTION *) (QueryName + QueryNameLen); > + QuerySection->Type = NTOHS (QuerySection->Type); > + QuerySection->Class = NTOHS (QuerySection->Class); > > OldTpl = gBS->RaiseTPL (TPL_CALLBACK); > > // > // Check DnsResponse Validity, if so, also get a valid NET_MAP_ITEM. > @@ -1339,14 +1370,30 @@ ParseDnsResponse ( > } > } > > Status = EFI_NOT_FOUND; > > + // > + // Get Answer name > + // > + AnswerName = (CHAR8 *) QuerySection + sizeof (*QuerySection); > + > // > // Processing AnswerSection. > // > while (AnswerSectionNum < DnsHeader->AnswersNum) { > + // > + // Check whether the remaining packet length is avaiable or not. > + // > + if (RemainingLength <= sizeof (UINT16) + sizeof (DNS_ANSWER_SECTION)) { > + *Completed = FALSE; > + Status = EFI_ABORTED; > + goto ON_EXIT; > + } else { > + RemainingLength -= (sizeof (UINT16) + sizeof (DNS_ANSWER_SECTION)); > + } > + > // > // Answer name should be PTR, else EFI_UNSUPPORTED returned. > // > if ((*(UINT8 *) AnswerName & 0xC0) != 0xC0) { > Status = EFI_UNSUPPORTED; > @@ -1360,10 +1407,21 @@ ParseDnsResponse ( > AnswerSection->Type = NTOHS (AnswerSection->Type); > AnswerSection->Class = NTOHS (AnswerSection->Class); > AnswerSection->Ttl = NTOHL (AnswerSection->Ttl); > AnswerSection->DataLength = NTOHS (AnswerSection->DataLength); > > + // > + // Check whether the remaining packet length is avaiable or not. > + // > + if (RemainingLength < AnswerSection->DataLength) { > + *Completed = FALSE; > + Status = EFI_ABORTED; > + goto ON_EXIT; > + } else { > + RemainingLength -= AnswerSection->DataLength; > + } > + > // > // Check whether it's the GeneralLookUp querying. > // > if (Instance->Service->IpVersion == IP_VERSION_4 && Dns4TokenEntry- > >GeneralLookUp) { > Dns4RR = Dns4TokenEntry->Token->RspData.GLookupData->RRList; > @@ -1731,10 +1789,11 @@ DnsOnPacketReceived ( > ) > { > DNS_INSTANCE *Instance; > > UINT8 *RcvString; > + UINT32 Len; > > BOOLEAN Completed; > > Instance = (DNS_INSTANCE *) Context; > NET_CHECK_SIGNATURE (Instance, DNS_INSTANCE_SIGNATURE); > @@ -1746,21 +1805,19 @@ DnsOnPacketReceived ( > goto ON_EXIT; > } > > ASSERT (Packet != NULL); > > - if (Packet->TotalSize <= sizeof (DNS_HEADER)) { > - goto ON_EXIT; > - } > + Len = Packet->TotalSize; > > RcvString = NetbufGetByte (Packet, 0, NULL); > ASSERT (RcvString != NULL); > > // > // Parse Dns Response > // > - ParseDnsResponse (Instance, RcvString, &Completed); > + ParseDnsResponse (Instance, RcvString, Len, &Completed); > > ON_EXIT: > > if (Packet != NULL) { > NetbufFree (Packet); > diff --git a/NetworkPkg/DnsDxe/DnsImpl.h b/NetworkPkg/DnsDxe/DnsImpl.h > index 90dc054903..45feca2160 100644 > --- a/NetworkPkg/DnsDxe/DnsImpl.h > +++ b/NetworkPkg/DnsDxe/DnsImpl.h > @@ -581,20 +581,22 @@ IsValidDnsResponse ( > /** > Parse Dns Response. > > @param Instance The DNS instance > @param RxString Received buffer. > + @param Length Received buffer length. > @param Completed Flag to indicate that Dns response is valid. > > @retval EFI_SUCCESS Parse Dns Response successfully. > @retval Others Failed to parse Dns Response. > > **/ > EFI_STATUS > ParseDnsResponse ( > IN OUT DNS_INSTANCE *Instance, > IN UINT8 *RxString, > + IN UINT32 Length, > OUT BOOLEAN *Completed > ); > > /** > Parse response packet. > -- > 2.17.1.windows.2
_______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
