[Patch] SecurityPkg OpalPasswordDxe: Fix buffer overflow issue.

[Patch] SecurityPkg OpalPasswordDxe: Fix buffer overflow issue.

[Eric Dong]

In current code, PSID is processed as string and the length is 0x20.
Current code only reserved 0x20 length buffer for it, no extra buffer
for the '\0'. When driver call UnicodeStrToAsciiStrS to convert PSID,
it search the '\0' for the end. So extra dirty data saved in PSID
info which caused PSID revert action failed. This patch reserved
extra 1 byte data for the '\0'.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Eric Dong <eric.dong@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
 SecurityPkg/Tcg/Opal/OpalPasswordDxe/OpalHii.c           | 5 ++++-
 SecurityPkg/Tcg/Opal/OpalPasswordDxe/OpalHiiFormValues.h | 3 ++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/SecurityPkg/Tcg/Opal/OpalPasswordDxe/OpalHii.c b/SecurityPkg/Tcg/Opal/OpalPasswordDxe/OpalHii.c
index 9a44c56..ee73697 100644
--- a/SecurityPkg/Tcg/Opal/OpalPasswordDxe/OpalHii.c
+++ b/SecurityPkg/Tcg/Opal/OpalPasswordDxe/OpalHii.c
@@ -595,12 +595,15 @@ HiiPsidRevert(
   OPAL_DISK                     *OpalDisk;
   TCG_RESULT                    Ret;
   OPAL_SESSION                  Session;
+  UINT8                         TmpBuf[PSID_CHARACTER_STRING_END_LENGTH];
 
   Ret = TcgResultFailure;
 
   OpalHiiGetBrowserData();
 
-  UnicodeStrToAsciiStrS (gHiiConfiguration.Psid, (CHAR8*)Psid.Psid, PSID_CHARACTER_LENGTH);
+  ZeroMem (TmpBuf, sizeof (TmpBuf));
+  UnicodeStrToAsciiStrS (gHiiConfiguration.Psid, (CHAR8*)TmpBuf, PSID_CHARACTER_STRING_END_LENGTH);
+  CopyMem (Psid.Psid, TmpBuf, PSID_CHARACTER_LENGTH);
 
   OpalDisk = HiiGetOpalDiskCB (gHiiConfiguration.SelectedDiskIndex);
   if (OpalDisk != NULL) {
diff --git a/SecurityPkg/Tcg/Opal/OpalPasswordDxe/OpalHiiFormValues.h b/SecurityPkg/Tcg/Opal/OpalPasswordDxe/OpalHiiFormValues.h
index 138bcb8..88cf9f5 100644
--- a/SecurityPkg/Tcg/Opal/OpalPasswordDxe/OpalHiiFormValues.h
+++ b/SecurityPkg/Tcg/Opal/OpalPasswordDxe/OpalHiiFormValues.h
@@ -21,6 +21,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 
 // PSID Length
 #define PSID_CHARACTER_LENGTH                              0x20
+#define PSID_CHARACTER_STRING_END_LENGTH                   0x21
 
 // ID's for various forms that will be used by HII
 #define FORMID_VALUE_MAIN_MENU                             0x01
@@ -38,7 +39,7 @@ typedef struct {
     UINT8   KeepUserData;
     UINT16  AvailableFields;
     UINT16  Password[MAX_PASSWORD_CHARACTER_LENGTH];
-    UINT16  Psid[PSID_CHARACTER_LENGTH];
+    UINT16  Psid[PSID_CHARACTER_STRING_END_LENGTH];
     UINT8   EnableBlockSid;
 } OPAL_HII_CONFIGURATION;
 #pragma pack()
-- 
2.6.4.windows.1

Re: [Patch] SecurityPkg OpalPasswordDxe: Fix buffer overflow issue.

[Zeng, Star]

Reviewed-by: Star Zeng <star.zeng@intel.com>

-----Original Message-----
From: Dong, Eric 
Sent: Tuesday, August 2, 2016 7:33 PM
To: edk2-devel@lists.01.org
Cc: Zeng, Star <star.zeng@intel.com>
Subject: [Patch] SecurityPkg OpalPasswordDxe: Fix buffer overflow issue.

In current code, PSID is processed as string and the length is 0x20.
Current code only reserved 0x20 length buffer for it, no extra buffer for the '\0'. When driver call UnicodeStrToAsciiStrS to convert PSID, it search the '\0' for the end. So extra dirty data saved in PSID info which caused PSID revert action failed. This patch reserved extra 1 byte data for the '\0'.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Eric Dong <eric.dong@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
 SecurityPkg/Tcg/Opal/OpalPasswordDxe/OpalHii.c           | 5 ++++-
 SecurityPkg/Tcg/Opal/OpalPasswordDxe/OpalHiiFormValues.h | 3 ++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/SecurityPkg/Tcg/Opal/OpalPasswordDxe/OpalHii.c b/SecurityPkg/Tcg/Opal/OpalPasswordDxe/OpalHii.c
index 9a44c56..ee73697 100644
--- a/SecurityPkg/Tcg/Opal/OpalPasswordDxe/OpalHii.c
+++ b/SecurityPkg/Tcg/Opal/OpalPasswordDxe/OpalHii.c
@@ -595,12 +595,15 @@ HiiPsidRevert(
   OPAL_DISK                     *OpalDisk;
   TCG_RESULT                    Ret;
   OPAL_SESSION                  Session;
+  UINT8                         TmpBuf[PSID_CHARACTER_STRING_END_LENGTH];
 
   Ret = TcgResultFailure;
 
   OpalHiiGetBrowserData();
 
-  UnicodeStrToAsciiStrS (gHiiConfiguration.Psid, (CHAR8*)Psid.Psid, PSID_CHARACTER_LENGTH);
+  ZeroMem (TmpBuf, sizeof (TmpBuf));
+  UnicodeStrToAsciiStrS (gHiiConfiguration.Psid, (CHAR8*)TmpBuf, 
+ PSID_CHARACTER_STRING_END_LENGTH);
+  CopyMem (Psid.Psid, TmpBuf, PSID_CHARACTER_LENGTH);
 
   OpalDisk = HiiGetOpalDiskCB (gHiiConfiguration.SelectedDiskIndex);
   if (OpalDisk != NULL) {
diff --git a/SecurityPkg/Tcg/Opal/OpalPasswordDxe/OpalHiiFormValues.h b/SecurityPkg/Tcg/Opal/OpalPasswordDxe/OpalHiiFormValues.h
index 138bcb8..88cf9f5 100644
--- a/SecurityPkg/Tcg/Opal/OpalPasswordDxe/OpalHiiFormValues.h
+++ b/SecurityPkg/Tcg/Opal/OpalPasswordDxe/OpalHiiFormValues.h
@@ -21,6 +21,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 
 // PSID Length
 #define PSID_CHARACTER_LENGTH                              0x20
+#define PSID_CHARACTER_STRING_END_LENGTH                   0x21
 
 // ID's for various forms that will be used by HII
 #define FORMID_VALUE_MAIN_MENU                             0x01
@@ -38,7 +39,7 @@ typedef struct {
     UINT8   KeepUserData;
     UINT16  AvailableFields;
     UINT16  Password[MAX_PASSWORD_CHARACTER_LENGTH];
-    UINT16  Psid[PSID_CHARACTER_LENGTH];
+    UINT16  Psid[PSID_CHARACTER_STRING_END_LENGTH];
     UINT8   EnableBlockSid;
 } OPAL_HII_CONFIGURATION;
 #pragma pack()
--
2.6.4.windows.1