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

[Laszlo Ersek <lersek@redhat.com>]

On 09/20/17 19:04, Long, Qin wrote:
> 
> 
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, September 21, 2017 12:38 AM
> To: Long, Qin <qin.long@intel.com>; Ye, Ting <ting.ye@intel.com>; Zhang, Chao B <chao.b.zhang@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [PATCH v2] CryptoPkg: Add new API to retrieve commonName of X.509 certificate
> 
> 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<mailto:lersek@redhat.com>>
>> Cc: Ting Ye <ting.ye@intel.com<mailto:ting.ye@intel.com>>
>> Cc: Chao Zhang <chao.b.zhang@intel.com<mailto:chao.b.zhang@intel.com>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Qin Long <qin.long@intel.com<mailto: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.
> 
> [qlong] good catch.
> The current implementation is based on OpenSSL X509_NAME_get_text_by_OBJ
> API, and we can only get the real written data size or required size (by passing
> NULL CommonName) with this interface.
> I didn’t want to introduce additional handling (e.g. extra ASN1_STRING parsing) in
> this API. For fixed CommonNameSize buffer, it’s acceptable to receive the truncated
> string (e.g. for display purpose) depending on the API usage (and the CN string should
> be less than 64 char per the standard).

OK. If the most obvious / straight-forward implementation for the
interface does not support these elaborate use cases, then I agree it's
fine to stick with what's readily available from the underlying features.

> 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.)
> 
> [qlong] It’s one missed case. And more interesting is this may expose one OpenSSL issue.
>               Let me double-check this. ☺

Heh, great :) Thanks!
Laszlo