Re: [PATCH v3] CryptoPkg: Add new API to retrieve commonName of X.509 certificate

["Ye, Ting" <ting.ye@intel.com>]

Hi Qin,

I think we might add OPTIONAL attribute to CommonName, as NULL is an valid input for this API.
In function description, I think we need update below statement to "if *** and *CommonNameSize is 0."

 "If CommonName is not NULL and CommonNameSize is 0."

Others are good to me.

Reviewed-by: Ye Ting <ting.ye@intel.com>

Best Regards,
Ting

-----Original Message-----
From: Long, Qin 
Sent: Thursday, September 21, 2017 10:48 AM
To: lersek@redhat.com; Ye, Ting <ting.ye@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>
Cc: edk2-devel@lists.01.org
Subject: [PATCH v3] CryptoPkg: Add new API to retrieve commonName of X.509 certificate

v3: Add extra CommonNameSize check since OpenSSL didn't check this
    input parameter. (One openssl issue was filed to address this risk:
    https://github.com/openssl/openssl/issues/4392)
v2: Update function interface to return RETURN_STATUS to represent
    different error cases.

Add one new API (X509GetCommonName()) to retrieve the subject commonName string from one X.509 certificate.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ting Ye <ting.ye@intel.com>
Cc: Chao Zhang <chao.b.zhang@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Qin Long <qin.long@intel.com>
---
 CryptoPkg/Application/Cryptest/RsaVerify2.c        |  32 ++++--
 CryptoPkg/Include/Library/BaseCryptLib.h           |  35 +++++++
 CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c      | 109 +++++++++++++++++++++
 CryptoPkg/Library/BaseCryptLib/Pk/CryptX509Null.c  |  32 ++++++
 .../Pk/CryptX509Null.c                             |  34 ++++++-
 5 files changed, 234 insertions(+), 8 deletions(-)

diff --git a/CryptoPkg/Application/Cryptest/RsaVerify2.c b/CryptoPkg/Application/Cryptest/RsaVerify2.c
index 98b5aad900..9db43d6eef 100644
--- a/CryptoPkg/Application/Cryptest/RsaVerify2.c
+++ b/CryptoPkg/Application/Cryptest/RsaVerify2.c
@@ -204,13 +204,17 @@ ValidateCryptRsa2 (
   VOID
   )
 {
-  BOOLEAN  Status;
-  VOID     *RsaPrivKey;
-  VOID     *RsaPubKey;
-  UINT8    *Signature;
-  UINTN    SigSize;
-  UINT8    *Subject;
-  UINTN    SubjectSize;
+  BOOLEAN        Status;
+  VOID           *RsaPrivKey;
+  VOID           *RsaPubKey;
+  UINT8          *Signature;
+  UINTN          SigSize;
+  UINT8          *Subject;
+  UINTN          SubjectSize;
+  RETURN_STATUS  ReturnStatus;
+  CHAR8          CommonName[64];
+  CHAR16         CommonNameUnicode[64];
+  UINTN          CommonNameSize;
 
   Print (L"\nUEFI-OpenSSL RSA Key Retrieving Testing: ");
 
@@ -286,6 +290,20 @@ ValidateCryptRsa2 (
     Print (L"[Pass]");
   }
 
+  //
+  // Get CommonName from X509 Certificate Subject  //  CommonNameSize = 
+ 64;  ZeroMem (CommonName, CommonNameSize);  ReturnStatus = 
+ X509GetCommonName (TestCert, sizeof (TestCert), CommonName, 
+ &CommonNameSize);  if (RETURN_ERROR (ReturnStatus)) {
+    Print (L"\n  - Retrieving Common Name - [Fail]");
+    return EFI_ABORTED;
+  } else {
+    AsciiStrToUnicodeStrS (CommonName, CommonNameUnicode, CommonNameSize);
+    Print (L"\n  - Retrieving Common Name = \"%s\" (Size = %d)", 
+ CommonNameUnicode, CommonNameSize);  }
+
   //
   // X509 Certificate Verification.
   //
diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h b/CryptoPkg/Include/Library/BaseCryptLib.h
index 9c5ffcd9cf..2366a0218d 100644
--- a/CryptoPkg/Include/Library/BaseCryptLib.h
+++ b/CryptoPkg/Include/Library/BaseCryptLib.h
@@ -2171,6 +2171,41 @@ X509GetSubjectName (
   IN OUT  UINTN        *SubjectSize
   );
 
+/**
+  Retrieve the common name (CN) string from one X.509 certificate.
+
+  @param[in]      Cert             Pointer to the DER-encoded X509 certificate.
+  @param[in]      CertSize         Size of the X509 certificate in bytes.
+  @param[out]     CommonName       Buffer to contain the retrieved certificate common
+                                   name string. At most CommonNameSize bytes will be
+                                   written and the string will be null terminated. May be
+                                   NULL in order to determine the size buffer needed.
+  @param[in,out]  CommonNameSize   The size in bytes of the CommonName buffer on input,
+                                   and the size of buffer returned CommonName on output.
+                                   If CommonName is NULL then the amount of space needed
+                                   in buffer (including the final null) is returned.
+
+  @retval RETURN_SUCCESS           The certificate CommonName retrieved successfully.
+  @retval RETURN_INVALID_PARAMETER If Cert is NULL.
+                                   If CommonNameSize is NULL.
+                                   If CommonName is not NULL and CommonNameSize is 0.
+                                   If Certificate is invalid.
+  @retval RETURN_NOT_FOUND         If no CommonName entry exists.
+  @retval RETURN_BUFFER_TOO_SMALL  If the CommonName is NULL. The required buffer size
+                                   (including the final null) is returned in the 
+                                   CommonNameSize parameter.
+  @retval RETURN_UNSUPPORTED       The operation is not supported.
+
+**/
+RETURN_STATUS
+EFIAPI
+X509GetCommonName (
+  IN      CONST UINT8  *Cert,
+  IN      UINTN        CertSize,
+  OUT     CHAR8        *CommonName,
+  IN OUT  UINTN        *CommonNameSize
+  );
+
 /**
   Verify one X509 certificate was issued by the trusted CA.
 
diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c b/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c
index 7d275977c5..bcd5ff49fd 100644
--- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c
+++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c
@@ -297,6 +297,115 @@ _Exit:
   return Status;
 }
 
+/**
+  Retrieve the common name (CN) string from one X.509 certificate.
+
+  @param[in]      Cert             Pointer to the DER-encoded X509 certificate.
+  @param[in]      CertSize         Size of the X509 certificate in bytes.
+  @param[out]     CommonName       Buffer to contain the retrieved certificate common
+                                   name string. At most CommonNameSize bytes will be
+                                   written and the string will be null terminated. May be
+                                   NULL in order to determine the size buffer needed.
+  @param[in,out]  CommonNameSize   The size in bytes of the CommonName buffer on input,
+                                   and the size of buffer returned CommonName on output.
+                                   If CommonName is NULL then the amount of space needed
+                                   in buffer (including the final null) is returned.
+
+  @retval RETURN_SUCCESS           The certificate CommonName retrieved successfully.
+  @retval RETURN_INVALID_PARAMETER If Cert is NULL.
+                                   If CommonNameSize is NULL.
+                                   If CommonName is not NULL and CommonNameSize is 0.
+                                   If Certificate is invalid.
+  @retval RETURN_NOT_FOUND         If no CommonName entry exists.
+  @retval RETURN_BUFFER_TOO_SMALL  If the CommonName is NULL. The required buffer size
+                                   (including the final null) is returned in the 
+                                   CommonNameSize parameter.
+  @retval RETURN_UNSUPPORTED       The operation is not supported.
+
+**/
+RETURN_STATUS
+EFIAPI
+X509GetCommonName (
+  IN      CONST UINT8  *Cert,
+  IN      UINTN        CertSize,
+  OUT     CHAR8        *CommonName,
+  IN OUT  UINTN        *CommonNameSize
+  )
+{
+  RETURN_STATUS  ReturnStatus;
+  BOOLEAN        Status;
+  X509           *X509Cert;
+  X509_NAME      *X509Name;
+  INTN           Length;
+
+  ReturnStatus = RETURN_INVALID_PARAMETER;
+
+  //
+  // Check input parameters.
+  //
+  if ((Cert == NULL) || (CertSize > INT_MAX) || (CommonNameSize == NULL)) {
+    return ReturnStatus;
+  }
+  if ((CommonName != NULL) && (*CommonNameSize == 0)) {
+    return ReturnStatus;
+  }
+
+  X509Cert = NULL;
+  //
+  // Read DER-encoded X509 Certificate and Construct X509 object.
+  //
+  Status = X509ConstructCertificate (Cert, CertSize, (UINT8 **) 
+ &X509Cert);  if ((X509Cert == NULL) || (!Status)) {
+    //
+    // Invalid X.509 Certificate
+    //
+    goto _Exit;
+  }
+
+  Status = FALSE;
+
+  //
+  // Retrieve subject name from certificate object.
+  //
+  X509Name = X509_get_subject_name (X509Cert);  if (X509Name == NULL) {
+    //
+    // Fail to retrieve subject name content
+    //
+    goto _Exit;
+  }
+
+  //
+  // Retrieve the CommonName information from X.509 Subject  //  Length 
+ = (INTN) X509_NAME_get_text_by_NID (X509Name, NID_commonName, 
+ CommonName, (int)(*CommonNameSize));  if (Length < 0) {
+    //
+    // No CommonName entry exists in X509_NAME object
+    //
+    *CommonNameSize = 0;
+    ReturnStatus    = RETURN_NOT_FOUND;
+    goto _Exit;
+  }
+
+  *CommonNameSize = (UINTN)(Length + 1);  if (CommonName == NULL) {
+    ReturnStatus = RETURN_BUFFER_TOO_SMALL;  } else {
+    ReturnStatus = RETURN_SUCCESS;
+  }
+
+_Exit:
+  //
+  // Release Resources.
+  //
+  if (X509Cert != NULL) {
+    X509_free (X509Cert);
+  }
+
+  return ReturnStatus;
+}
+
 /**
   Retrieve the RSA Public Key from one DER-encoded X509 certificate.
 
diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509Null.c b/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509Null.c
index 51aa0633a8..25879d3578 100644
--- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509Null.c
+++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptX509Null.c
@@ -127,6 +127,38 @@ X509GetSubjectName (
   return FALSE;
 }
 
+/**
+  Retrieve the common name (CN) string from one X.509 certificate.
+
+  Return RETURN_UNSUPPORTED to indicate this interface is not supported.
+
+  @param[in]      Cert             Pointer to the DER-encoded X509 certificate.
+  @param[in]      CertSize         Size of the X509 certificate in bytes.
+  @param[out]     CommonName       Buffer to contain the retrieved certificate common
+                                   name string. At most CommonNameSize bytes will be
+                                   written and the string will be null terminated. May be
+                                   NULL in order to determine the size buffer needed.
+  @param[in,out]  CommonNameSize   The size in bytes of the CommonName buffer on input,
+                                   and the size of buffer returned CommonName on output.
+                                   If CommonName is NULL then the amount of space needed
+                                   in buffer (including the final null) is returned.
+
+  @retval RETURN_UNSUPPORTED       The operation is not supported.
+
+**/
+RETURN_STATUS
+EFIAPI
+X509GetCommonName (
+  IN      CONST UINT8  *Cert,
+  IN      UINTN        CertSize,
+  OUT     CHAR8        *CommonName,
+  IN OUT  UINTN        *CommonNameSize
+  )
+{
+  ASSERT (FALSE);
+  return RETURN_UNSUPPORTED;
+}
+
 /**
   Retrieve the RSA Public Key from one DER-encoded X509 certificate.
 
diff --git a/CryptoPkg/Library/BaseCryptLibRuntimeCryptProtocol/Pk/CryptX509Null.c b/CryptoPkg/Library/BaseCryptLibRuntimeCryptProtocol/Pk/CryptX509Null.c
index f5d9aa1076..25879d3578 100644
--- a/CryptoPkg/Library/BaseCryptLibRuntimeCryptProtocol/Pk/CryptX509Null.c
+++ b/CryptoPkg/Library/BaseCryptLibRuntimeCryptProtocol/Pk/CryptX509Nul
+++ l.c
@@ -127,6 +127,38 @@ X509GetSubjectName (
   return FALSE;
 }
 
+/**
+  Retrieve the common name (CN) string from one X.509 certificate.
+
+  Return RETURN_UNSUPPORTED to indicate this interface is not supported.
+
+  @param[in]      Cert             Pointer to the DER-encoded X509 certificate.
+  @param[in]      CertSize         Size of the X509 certificate in bytes.
+  @param[out]     CommonName       Buffer to contain the retrieved certificate common
+                                   name string. At most CommonNameSize bytes will be
+                                   written and the string will be null terminated. May be
+                                   NULL in order to determine the size buffer needed.
+  @param[in,out]  CommonNameSize   The size in bytes of the CommonName buffer on input,
+                                   and the size of buffer returned CommonName on output.
+                                   If CommonName is NULL then the amount of space needed
+                                   in buffer (including the final null) is returned.
+
+  @retval RETURN_UNSUPPORTED       The operation is not supported.
+
+**/
+RETURN_STATUS
+EFIAPI
+X509GetCommonName (
+  IN      CONST UINT8  *Cert,
+  IN      UINTN        CertSize,
+  OUT     CHAR8        *CommonName,
+  IN OUT  UINTN        *CommonNameSize
+  )
+{
+  ASSERT (FALSE);
+  return RETURN_UNSUPPORTED;
+}
+
 /**
   Retrieve the RSA Public Key from one DER-encoded X509 certificate.
 
@@ -203,4 +235,4 @@ X509GetTBSCert (
 {
   ASSERT (FALSE);
   return FALSE;
-}
\ No newline at end of file
+}
--
2.14.1.windows.1