Re: [edk2] [PATCH v2 1/1] MdeModulePkg:DxeHttpLib: Add checks in HttpGenRequestMessage API

2016-05-12 Thread Fu, Siyuan 
Reviewed-by: Fu Siyuan <siyuan...@intel.com>


> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Nagaraj Hegde
> Sent: Thursday, May 5, 2016 2:59 PM
> To: edk2-devel@lists.01.org
> Cc: Ye, Ting <ting...@intel.com>; Fu, Siyuan <siyuan...@intel.com>; Wu,
> Jiaxin <jiaxin...@intel.com>
> Subject: [edk2] [PATCH v2 1/1] MdeModulePkg:DxeHttpLib: Add checks in
> HttpGenRequestMessage API
> 
> v2:
> More input validation based on Request and HeaderCount.
> 
> HttpGenRequestMessage assumes that HTTP message would always
> contain a request-line, headers and an optional message body.
> However, subsequent to a HTTP PUT/POST request, HTTP requests
> would contain just the message body. This patch supports
> creation of such request messages with additional checks.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Hegde, Nagaraj P nagaraj-p.he...@hpe.com
> ---
>  MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c | 225 +++-
>  1 file changed, 130 insertions(+), 95 deletions(-)
> 
> diff --git a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> index 8d61d0b..1b0858c 100644
> --- a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> +++ b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> @@ -1681,61 +1681,92 @@ HttpGenRequestMessage (
>HttpUtilitiesProtocol = NULL;
> 
>//
> -  // Locate the HTTP_UTILITIES protocol.
> +  // 1. If we have a Request, we cannot have a NULL Url
> +  // 2. If we have a Request, HeaderCount can not be non-zero
> +  // 3. If we do not have a Request, HeaderCount should be zero
> +  // 4. If we do not have Request and Headers, we need at least a message-
> body
>//
> -  Status = gBS->LocateProtocol (
> -  ,
> -  NULL,
> -  (VOID **)
> -  );
> -
> -  if (EFI_ERROR (Status)) {
> -DEBUG ((DEBUG_ERROR,"Failed to locate Http Utilities protocol. Status
> = %r.\n", Status));
> -return Status;
> +  if ((Message->Data.Request != NULL && Url == NULL) ||
> +  (Message->Data.Request != NULL && Message->HeaderCount == 0) ||
> +  (Message->Data.Request == NULL && Message->HeaderCount != 0) ||
> +  (Message->Data.Request == NULL && Message->HeaderCount == 0 &&
> Message->BodyLength == 0)) {
> +return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (Message->HeaderCount != 0) {
> +//
> +// Locate the HTTP_UTILITIES protocol.
> +//
> +Status = gBS->LocateProtocol (
> +,
> +NULL,
> +(VOID **)
> +);
> +
> +if (EFI_ERROR (Status)) {
> +  DEBUG ((DEBUG_ERROR,"Failed to locate Http Utilities protocol. Status
> = %r.\n", Status));
> +  return Status;
> +}
> +
> +//
> +// Build AppendList to send into HttpUtilitiesBuild
> +//
> +AppendList = AllocateZeroPool (sizeof (EFI_HTTP_HEADER *) * (Message-
> >HeaderCount));
> +if (AppendList == NULL) {
> +  return EFI_OUT_OF_RESOURCES;
> +}
> +
> +for(Index = 0; Index < Message->HeaderCount; Index++){
> +  AppendList[Index] = >Headers[Index];
> +}
> +
> +//
> +// Build raw HTTP Headers
> +//
> +Status = HttpUtilitiesProtocol->Build (
> +HttpUtilitiesProtocol,
> +0,
> +NULL,
> +0,
> +NULL,
> +Message->HeaderCount,
> +AppendList,
> +,
> +
> +);
> +
> +if (AppendList != NULL) {
> +  FreePool (AppendList);
> +}
> +
> +if (EFI_ERROR (Status) || HttpHdr == NULL){
> +  return Status;
> +}
>}
> 
>//
> -  // Build AppendList to send into HttpUtilitiesBuild
> +  // If we have headers to be sent, account for it.
>//
> -  AppendList = AllocateZeroPool (sizeof (EFI_HTTP_HEADER *) * (Message-
> >HeaderCount));
> -  if (AppendList == NULL) {
> -return EFI_OUT_OF_RESOURCES;
> -  }
> -
> -  for(Index = 0; Index < Message->HeaderCount; Index++){
> -AppendList[Index] = >Headers[Index];
> +  if (Message->HeaderCount != 0) {
> +MsgSize = HttpHdrSize;
>}
> 
>//
> -  // Build raw HTTP Headers
> +  // If we have a request line, account for the fields.
>//
> -  Status = HttpUtilitiesProtocol->Build (
> -  HttpUtilitiesProtocol,
> -

Re: [edk2] [PATCH v2 1/1] MdeModulePkg:DxeHttpLib: Add checks in HttpGenRequestMessage API

2016-05-12 Thread Hegde, Nagaraj P 
Hi Siyuan,

Do you have any additional feedback on the patch v2?

Regards,
Nagaraj.

-Original Message-
From: Wu, Jiaxin [mailto:jiaxin...@intel.com] 
Sent: Monday, May 09, 2016 7:29 AM
To: Hegde, Nagaraj P <nagaraj-p.he...@hpe.com>; edk2-devel@lists.01.org
Cc: Ye, Ting <ting...@intel.com>; Fu, Siyuan <siyuan...@intel.com>
Subject: RE: [edk2] [PATCH v2 1/1] MdeModulePkg:DxeHttpLib: Add checks in 
HttpGenRequestMessage API

It's good to me!

Reviewed-By: Wu Jiaxin <jiaxin...@intel.com>

Best Regards!
Jiaxin

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
> Nagaraj Hegde
> Sent: Thursday, May 5, 2016 2:59 PM
> To: edk2-devel@lists.01.org
> Cc: Ye, Ting <ting...@intel.com>; Fu, Siyuan <siyuan...@intel.com>; 
> Wu, Jiaxin <jiaxin...@intel.com>
> Subject: [edk2] [PATCH v2 1/1] MdeModulePkg:DxeHttpLib: Add checks in 
> HttpGenRequestMessage API
> 
> v2:
> More input validation based on Request and HeaderCount.
> 
> HttpGenRequestMessage assumes that HTTP message would always contain a 
> request-line, headers and an optional message body.
> However, subsequent to a HTTP PUT/POST request, HTTP requests would 
> contain just the message body. This patch supports creation of such 
> request messages with additional checks.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Hegde, Nagaraj P nagaraj-p.he...@hpe.com
> ---
>  MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c | 225 +++---
> --
>  1 file changed, 130 insertions(+), 95 deletions(-)
> 
> diff --git a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> index 8d61d0b..1b0858c 100644
> --- a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> +++ b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> @@ -1681,61 +1681,92 @@ HttpGenRequestMessage (
>HttpUtilitiesProtocol = NULL;
> 
>//
> -  // Locate the HTTP_UTILITIES protocol.
> +  // 1. If we have a Request, we cannot have a NULL Url  // 2. If we 
> + have a Request, HeaderCount can not be non-zero  // 3. If we do not 
> + have a Request, HeaderCount should be zero  // 4. If we do not have 
> + Request and Headers, we need at least a message-body
>//
> -  Status = gBS->LocateProtocol (
> -  ,
> -  NULL,
> -  (VOID **)
> -  );
> -
> -  if (EFI_ERROR (Status)) {
> -DEBUG ((DEBUG_ERROR,"Failed to locate Http Utilities protocol. Status
> = %r.\n", Status));
> -return Status;
> +  if ((Message->Data.Request != NULL && Url == NULL) ||
> +  (Message->Data.Request != NULL && Message->HeaderCount == 0) ||
> +  (Message->Data.Request == NULL && Message->HeaderCount != 0) ||
> +  (Message->Data.Request == NULL && Message->HeaderCount == 0 &&
> Message->BodyLength == 0)) {
> +return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (Message->HeaderCount != 0) {
> +//
> +// Locate the HTTP_UTILITIES protocol.
> +//
> +Status = gBS->LocateProtocol (
> +,
> +NULL,
> +(VOID **)
> +);
> +
> +if (EFI_ERROR (Status)) {
> +  DEBUG ((DEBUG_ERROR,"Failed to locate Http Utilities protocol. 
> + Status
> = %r.\n", Status));
> +  return Status;
> +}
> +
> +//
> +// Build AppendList to send into HttpUtilitiesBuild
> +//
> +AppendList = AllocateZeroPool (sizeof (EFI_HTTP_HEADER *) * 
> + (Message-
> >HeaderCount));
> +if (AppendList == NULL) {
> +  return EFI_OUT_OF_RESOURCES;
> +}
> +
> +for(Index = 0; Index < Message->HeaderCount; Index++){
> +  AppendList[Index] = >Headers[Index];
> +}
> +
> +//
> +// Build raw HTTP Headers
> +//
> +Status = HttpUtilitiesProtocol->Build (
> +HttpUtilitiesProtocol,
> +0,
> +NULL,
> +0,
> +NULL,
> +Message->HeaderCount,
> +AppendList,
> +,
> +
> +);
> +
> +if (AppendList != NULL) {
> +  FreePool (AppendList);
> +}
> +
> +if (EFI_ERROR (Status) || HttpHdr == NULL){
> +  return Status;
> +}
>}
> 
>//
> -  // Build AppendList to send into HttpUtilitiesBuild
> +  // If we have headers to be sent, account for it.
>//
> -  AppendList = AllocateZeroPool (sizeof (EFI_HTTP_HEADER *) * 
> (Message-
> >HeaderCo

Re: [edk2] [PATCH v2 1/1] MdeModulePkg:DxeHttpLib: Add checks in HttpGenRequestMessage API

2016-05-08 Thread Wu, Jiaxin 
It's good to me!

Reviewed-By: Wu Jiaxin <jiaxin...@intel.com>

Best Regards!
Jiaxin

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Nagaraj Hegde
> Sent: Thursday, May 5, 2016 2:59 PM
> To: edk2-devel@lists.01.org
> Cc: Ye, Ting <ting...@intel.com>; Fu, Siyuan <siyuan...@intel.com>; Wu,
> Jiaxin <jiaxin...@intel.com>
> Subject: [edk2] [PATCH v2 1/1] MdeModulePkg:DxeHttpLib: Add checks in
> HttpGenRequestMessage API
> 
> v2:
> More input validation based on Request and HeaderCount.
> 
> HttpGenRequestMessage assumes that HTTP message would always contain
> a request-line, headers and an optional message body.
> However, subsequent to a HTTP PUT/POST request, HTTP requests would
> contain just the message body. This patch supports creation of such request
> messages with additional checks.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Hegde, Nagaraj P nagaraj-p.he...@hpe.com
> ---
>  MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c | 225 +++---
> --
>  1 file changed, 130 insertions(+), 95 deletions(-)
> 
> diff --git a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> index 8d61d0b..1b0858c 100644
> --- a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> +++ b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
> @@ -1681,61 +1681,92 @@ HttpGenRequestMessage (
>HttpUtilitiesProtocol = NULL;
> 
>//
> -  // Locate the HTTP_UTILITIES protocol.
> +  // 1. If we have a Request, we cannot have a NULL Url  // 2. If we
> + have a Request, HeaderCount can not be non-zero  // 3. If we do not
> + have a Request, HeaderCount should be zero  // 4. If we do not have
> + Request and Headers, we need at least a message-body
>//
> -  Status = gBS->LocateProtocol (
> -  ,
> -  NULL,
> -  (VOID **)
> -  );
> -
> -  if (EFI_ERROR (Status)) {
> -DEBUG ((DEBUG_ERROR,"Failed to locate Http Utilities protocol. Status
> = %r.\n", Status));
> -return Status;
> +  if ((Message->Data.Request != NULL && Url == NULL) ||
> +  (Message->Data.Request != NULL && Message->HeaderCount == 0) ||
> +  (Message->Data.Request == NULL && Message->HeaderCount != 0) ||
> +  (Message->Data.Request == NULL && Message->HeaderCount == 0 &&
> Message->BodyLength == 0)) {
> +return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (Message->HeaderCount != 0) {
> +//
> +// Locate the HTTP_UTILITIES protocol.
> +//
> +Status = gBS->LocateProtocol (
> +,
> +NULL,
> +(VOID **)
> +);
> +
> +if (EFI_ERROR (Status)) {
> +  DEBUG ((DEBUG_ERROR,"Failed to locate Http Utilities protocol. Status
> = %r.\n", Status));
> +  return Status;
> +}
> +
> +//
> +// Build AppendList to send into HttpUtilitiesBuild
> +//
> +AppendList = AllocateZeroPool (sizeof (EFI_HTTP_HEADER *) * (Message-
> >HeaderCount));
> +if (AppendList == NULL) {
> +  return EFI_OUT_OF_RESOURCES;
> +}
> +
> +for(Index = 0; Index < Message->HeaderCount; Index++){
> +  AppendList[Index] = >Headers[Index];
> +}
> +
> +//
> +// Build raw HTTP Headers
> +//
> +Status = HttpUtilitiesProtocol->Build (
> +HttpUtilitiesProtocol,
> +0,
> +NULL,
> +0,
> +NULL,
> +Message->HeaderCount,
> +AppendList,
> +,
> +
> +);
> +
> +if (AppendList != NULL) {
> +  FreePool (AppendList);
> +}
> +
> +if (EFI_ERROR (Status) || HttpHdr == NULL){
> +  return Status;
> +}
>}
> 
>//
> -  // Build AppendList to send into HttpUtilitiesBuild
> +  // If we have headers to be sent, account for it.
>//
> -  AppendList = AllocateZeroPool (sizeof (EFI_HTTP_HEADER *) * (Message-
> >HeaderCount));
> -  if (AppendList == NULL) {
> -return EFI_OUT_OF_RESOURCES;
> -  }
> -
> -  for(Index = 0; Index < Message->HeaderCount; Index++){
> -AppendList[Index] = >Headers[Index];
> +  if (Message->HeaderCount != 0) {
> +MsgSize = HttpHdrSize;
>}
> 
>//
> -  // Build raw HTTP Headers
> +  // If we have a request line, account for the fields.
>//
> -  Status = HttpUtilitiesProtocol-&g

[edk2] [PATCH v2 1/1] MdeModulePkg:DxeHttpLib: Add checks in HttpGenRequestMessage API

2016-05-05 Thread Nagaraj Hegde 
v2:
More input validation based on Request and HeaderCount.

HttpGenRequestMessage assumes that HTTP message would always
contain a request-line, headers and an optional message body.
However, subsequent to a HTTP PUT/POST request, HTTP requests
would contain just the message body. This patch supports
creation of such request messages with additional checks.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Hegde, Nagaraj P nagaraj-p.he...@hpe.com
---
 MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c | 225 +++-
 1 file changed, 130 insertions(+), 95 deletions(-)

diff --git a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c 
b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
index 8d61d0b..1b0858c 100644
--- a/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
+++ b/MdeModulePkg/Library/DxeHttpLib/DxeHttpLib.c
@@ -1681,61 +1681,92 @@ HttpGenRequestMessage (
   HttpUtilitiesProtocol = NULL;
 
   //
-  // Locate the HTTP_UTILITIES protocol.
+  // 1. If we have a Request, we cannot have a NULL Url
+  // 2. If we have a Request, HeaderCount can not be non-zero
+  // 3. If we do not have a Request, HeaderCount should be zero
+  // 4. If we do not have Request and Headers, we need at least a message-body
   //
-  Status = gBS->LocateProtocol (
-  ,
-  NULL,
-  (VOID **)
-  );
-
-  if (EFI_ERROR (Status)) {
-DEBUG ((DEBUG_ERROR,"Failed to locate Http Utilities protocol. Status = 
%r.\n", Status));
-return Status;
+  if ((Message->Data.Request != NULL && Url == NULL) ||
+  (Message->Data.Request != NULL && Message->HeaderCount == 0) ||
+  (Message->Data.Request == NULL && Message->HeaderCount != 0) ||
+  (Message->Data.Request == NULL && Message->HeaderCount == 0 && 
Message->BodyLength == 0)) {
+return EFI_INVALID_PARAMETER;
+  }
+
+  if (Message->HeaderCount != 0) {
+//
+// Locate the HTTP_UTILITIES protocol.
+//
+Status = gBS->LocateProtocol (
+,
+NULL,
+(VOID **)
+);
+
+if (EFI_ERROR (Status)) {
+  DEBUG ((DEBUG_ERROR,"Failed to locate Http Utilities protocol. Status = 
%r.\n", Status));
+  return Status;
+}
+
+//
+// Build AppendList to send into HttpUtilitiesBuild
+//
+AppendList = AllocateZeroPool (sizeof (EFI_HTTP_HEADER *) * 
(Message->HeaderCount));
+if (AppendList == NULL) {
+  return EFI_OUT_OF_RESOURCES;
+}
+
+for(Index = 0; Index < Message->HeaderCount; Index++){
+  AppendList[Index] = >Headers[Index];
+}
+
+//
+// Build raw HTTP Headers
+//
+Status = HttpUtilitiesProtocol->Build (
+HttpUtilitiesProtocol,
+0,
+NULL,
+0,
+NULL,
+Message->HeaderCount,
+AppendList,
+,
+
+);
+
+if (AppendList != NULL) {
+  FreePool (AppendList);
+}
+
+if (EFI_ERROR (Status) || HttpHdr == NULL){
+  return Status;
+}
   }
 
   //
-  // Build AppendList to send into HttpUtilitiesBuild
+  // If we have headers to be sent, account for it.
   //
-  AppendList = AllocateZeroPool (sizeof (EFI_HTTP_HEADER *) * 
(Message->HeaderCount));
-  if (AppendList == NULL) {
-return EFI_OUT_OF_RESOURCES;
-  }
-
-  for(Index = 0; Index < Message->HeaderCount; Index++){
-AppendList[Index] = >Headers[Index];
+  if (Message->HeaderCount != 0) {
+MsgSize = HttpHdrSize;
   }
 
   //
-  // Build raw HTTP Headers
+  // If we have a request line, account for the fields.
   //
-  Status = HttpUtilitiesProtocol->Build (
-  HttpUtilitiesProtocol,
-  0,
-  NULL,
-  0,
-  NULL,
-  Message->HeaderCount,
-  AppendList,
-  ,
-  
-  );
-
-  if (AppendList != NULL) {
-FreePool (AppendList);
-  }
-
-  if (EFI_ERROR (Status) || HttpHdr == NULL){
-return Status;
+  if (Message->Data.Request != NULL) {
+MsgSize += HTTP_METHOD_MAXIMUM_LEN + AsciiStrLen (HTTP_VERSION_CRLF_STR) + 
AsciiStrLen (Url);
   }
 
+
   //
-  // Calculate HTTP message length.
+  // If we have a message body to be sent, account for it.
   //
-  MsgSize = Message->BodyLength + HTTP_METHOD_MAXIMUM_LEN + AsciiStrLen (Url) +
-AsciiStrLen (HTTP_VERSION_CRLF_STR) + HttpHdrSize;
-
+  MsgSize += Message->BodyLength;
 
+  //
+  // memory for the string that needs to be sent to TCP
+  //
   *RequestMsg = AllocateZeroPool (MsgSize);
   if (*RequestMsg == NULL) {
 Status = EFI_OUT_OF_RESOURCES;
@@ -1746,60 +1777,64 @@ HttpGenRequestMessage (
   //
   // Construct header request
   //
-  switch (Message->Data.Request->Method) {
-  case HttpMethodGet:
-StrLength = sizeof (HTTP_METHOD_GET) - 1;
-CopyMem (RequestPtr, HTTP_METHOD_GET, StrLength);
-RequestPtr += StrLength;
-break;
-