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

[Laszlo Ersek <lersek@redhat.com>]

Hello Qin,

On 09/20/17 18:05, Qin Long wrote:
> 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           |  34 +++++++
>  CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c      | 106 +++++++++++++++++++++
>  CryptoPkg/Library/BaseCryptLib/Pk/CryptX509Null.c  |  32 +++++++
>  .../Pk/CryptX509Null.c                             |  34 ++++++-
>  5 files changed, 230 insertions(+), 8 deletions(-)
> 
> diff --git a/CryptoPkg/Include/Library/BaseCryptLib.h b/CryptoPkg/Include/Library/BaseCryptLib.h
> index 9c5ffcd9cf..48e9531758 100644
> --- a/CryptoPkg/Include/Library/BaseCryptLib.h
> +++ b/CryptoPkg/Include/Library/BaseCryptLib.h
> @@ -2171,6 +2171,40 @@ 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 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.
>  

I think the RETURN_BUFFER_TOO_SMALL description is incorrect -- it
shouldn't only cover the (CommonName == NULL) case, but any other case
when *CommonNameSize is not large enough, for formatting the full CN,
plus the terminating '\0'.

Relatedly, the output value of *CommonNameSize should always be the
number of bytes required to format the NUL-terminated common name,
regardless if there is enough room or not. The return status will tell
the caller:
- if the return status is BUFFER_TOO_SMALL, then a larger buffer is
needed -- how large is explained by *CommonNameSize
- if the return status is SUCCESS, then the buffer was large enough, and
*CommonNameSize bytes have been used from it.

Additional question: does the code handle the case when *CommonNameSize
is zero, on input? (I.e., when there isn't even room for storing a '\0'
character.)

Thanks,
Laszlo