[Patch] NetworkPkg/HttpDxe: Fix the potential NULL dereference

* [Patch] NetworkPkg/HttpDxe: Fix the potential NULL dereference
@ 2016-12-23  3:14 Jiaxin Wu
  2016-12-26  8:00 ` Ye, Ting
  2016-12-26  9:44 ` Fu, Siyuan
  0 siblings, 2 replies; 3+ messages in thread
From: Jiaxin Wu @ 2016-12-23  3:14 UTC (permalink / raw)
  To: edk2-devel; +Cc: Ye Ting, Fu Siyuan, Wu Hao A, Wu Jiaxin

Cc: Ye Ting <ting.ye@intel.com>
Cc: Fu Siyuan <siyuan.fu@intel.com>
Cc: Wu Hao A <hao.a.wu@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
---
 NetworkPkg/HttpDxe/HttpImpl.c     |  4 ++-
 NetworkPkg/HttpDxe/HttpProto.c    |  6 +++-
 NetworkPkg/HttpDxe/HttpsSupport.c | 74 +++++++++++++++++++++++++++------------
 3 files changed, 60 insertions(+), 24 deletions(-)

diff --git a/NetworkPkg/HttpDxe/HttpImpl.c b/NetworkPkg/HttpDxe/HttpImpl.c
index 77aa64a..d19f733 100644
--- a/NetworkPkg/HttpDxe/HttpImpl.c
+++ b/NetworkPkg/HttpDxe/HttpImpl.c
@@ -589,14 +589,16 @@ EfiHttpRequest (
     }
   }
 
   Status = HttpGenRequestMessage (HttpMsg, FileUrl, &RequestMsg, &RequestMsgSize);
 
-  if (EFI_ERROR (Status)) {
+  if (EFI_ERROR (Status) || NULL == RequestMsg) {
     goto Error3;
   }
 
+  ASSERT (RequestMsg != NULL);
+
   //
   // Every request we insert a TxToken and a response call would remove the TxToken.
   // In cases of PUT/POST, after an initial request-response pair, we would do a
   // continuous request without a response call. So, in such cases, where Request
   // structure is NULL, we would not insert a TxToken.
diff --git a/NetworkPkg/HttpDxe/HttpProto.c b/NetworkPkg/HttpDxe/HttpProto.c
index 36c61e2..199d575 100644
--- a/NetworkPkg/HttpDxe/HttpProto.c
+++ b/NetworkPkg/HttpDxe/HttpProto.c
@@ -1653,10 +1653,12 @@ HttpTcpTransmit (
   CHAR8                     *RequestMsg;
   CHAR8                     *Url;
   UINTN                     UrlSize;
   UINTN                     RequestMsgSize;
 
+  RequestMsg = NULL;
+
   ValueInItem = (HTTP_TOKEN_WRAP *) Item->Value;
   if (ValueInItem->TcpWrap.IsTxDone) {
     return EFI_SUCCESS;
   }
 
@@ -1680,14 +1682,16 @@ HttpTcpTransmit (
                  &RequestMsg,
                  &RequestMsgSize
                  );
   FreePool (Url);
 
-  if (EFI_ERROR (Status)){
+  if (EFI_ERROR (Status) || NULL == RequestMsg){
     return Status;
   }
 
+  ASSERT (RequestMsg != NULL);
+
   //
   // Transmit the request message.
   //
   Status = HttpTransmitTcp (
              ValueInItem->HttpInstance,
diff --git a/NetworkPkg/HttpDxe/HttpsSupport.c b/NetworkPkg/HttpDxe/HttpsSupport.c
index 478a9e0..c9e6988 100644
--- a/NetworkPkg/HttpDxe/HttpsSupport.c
+++ b/NetworkPkg/HttpDxe/HttpsSupport.c
@@ -399,37 +399,41 @@ TlsConfigCertificate (
                    NULL,
                    &CACertSize,
                    NULL
                    );
 
-  if (Status == EFI_BUFFER_TOO_SMALL) {
+  if (EFI_ERROR (Status) && Status != EFI_BUFFER_TOO_SMALL) {
+    return Status;
+  }
+
+  //
+  // Allocate buffer and read the config variable.
+  //
+  CACert = AllocatePool (CACertSize);
+  if (CACert == NULL) {
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  Status = gRT->GetVariable (
+                  EFI_TLS_CA_CERTIFICATE_VARIABLE,
+                  &gEfiTlsCaCertificateGuid,
+                  NULL,
+                  &CACertSize,
+                  CACert
+                  );
+  if (EFI_ERROR (Status)) {
     //
-    // Allocate buffer and read the config variable.
+    // GetVariable still error or the variable is corrupted.
+    // Fall back to the default value.
     //
-    CACert = AllocatePool (CACertSize);
-    if (CACert == NULL) {
-      return EFI_OUT_OF_RESOURCES;
-    }
-
-    Status = gRT->GetVariable (
-                    EFI_TLS_CA_CERTIFICATE_VARIABLE,
-                    &gEfiTlsCaCertificateGuid,
-                    NULL,
-                    &CACertSize,
-                    CACert
-                    );
-    if (EFI_ERROR (Status)) {
-      //
-      // GetVariable still error or the variable is corrupted.
-      // Fall back to the default value.
-      //
-      FreePool (CACert);
+    FreePool (CACert);
 
-      return EFI_NOT_FOUND;
-    }
+    return EFI_NOT_FOUND;
   }
 
+  ASSERT (CACert != NULL);
+
   //
   // Enumerate all data and erasing the target item.
   //
   ItemDataSize = (UINT32) CACertSize;
   CertList = (EFI_SIGNATURE_LIST *) CACert;
@@ -1035,10 +1039,15 @@ TlsConnectSession (
   //
   // Transmit ClientHello
   //
   PacketOut = NetbufAlloc ((UINT32) BufferOutSize);
   DataOut = NetbufAllocSpace (PacketOut, (UINT32) BufferOutSize, NET_BUF_TAIL);
+  if (DataOut == NULL) {
+    FreePool (BufferOut);
+    return EFI_OUT_OF_RESOURCES;
+  }
+  
   CopyMem (DataOut, BufferOut, BufferOutSize);
   Status = TlsCommonTransmit (HttpInstance, PacketOut);
 
   FreePool (BufferOut);
   NetbufFree (PacketOut);
@@ -1105,19 +1114,25 @@ TlsConnectSession (
     }
 
     FreePool (BufferIn);
 
     if (EFI_ERROR (Status)) {
+      FreePool (BufferOut);
       return Status;
     }
 
     if (BufferOutSize != 0) {
       //
       // Transmit the response packet.
       //
       PacketOut = NetbufAlloc ((UINT32) BufferOutSize);
       DataOut = NetbufAllocSpace (PacketOut, (UINT32) BufferOutSize, NET_BUF_TAIL);
+      if (DataOut == NULL) {
+        FreePool (BufferOut);
+        return EFI_OUT_OF_RESOURCES;
+      }
+      
       CopyMem (DataOut, BufferOut, BufferOutSize);
 
       Status = TlsCommonTransmit (HttpInstance, PacketOut);
 
       NetbufFree (PacketOut);
@@ -1265,10 +1280,15 @@ TlsCloseSession (
     return Status;
   }
 
   PacketOut = NetbufAlloc ((UINT32) BufferOutSize);
   DataOut = NetbufAllocSpace (PacketOut, (UINT32) BufferOutSize, NET_BUF_TAIL);
+  if (DataOut == NULL) {
+    FreePool (BufferOut);
+    return EFI_OUT_OF_RESOURCES;
+  }
+  
   CopyMem (DataOut, BufferOut, BufferOutSize);
 
   Status = TlsCommonTransmit (HttpInstance, PacketOut);
 
   FreePool (BufferOut);
@@ -1538,10 +1558,15 @@ HttpsReceive (
         }
 
         if (BufferOutSize != 0) {
           PacketOut = NetbufAlloc ((UINT32)BufferOutSize);
           DataOut = NetbufAllocSpace (PacketOut, (UINT32) BufferOutSize, NET_BUF_TAIL);
+          if (DataOut == NULL) {
+            FreePool (BufferOut);
+            return EFI_OUT_OF_RESOURCES;
+          }
+          
           CopyMem (DataOut, BufferOut, BufferOutSize);
 
           Status = TlsCommonTransmit (HttpInstance, PacketOut);
 
           NetbufFree (PacketOut);
@@ -1625,10 +1650,15 @@ HttpsReceive (
     }
 
     if (BufferOutSize != 0) {
       PacketOut = NetbufAlloc ((UINT32) BufferOutSize);
       DataOut = NetbufAllocSpace (PacketOut, (UINT32) BufferOutSize, NET_BUF_TAIL);
+      if (DataOut == NULL) {
+        FreePool (BufferOut);
+        return EFI_OUT_OF_RESOURCES;
+      }
+      
       CopyMem (DataOut, BufferOut, BufferOutSize);
 
       Status = TlsCommonTransmit (HttpInstance, PacketOut);
 
       NetbufFree (PacketOut);
-- 
1.9.5.msysgit.1



^ permalink raw reply related	[flat|nested] 3+ messages in thread