On 09/20/17 19:04, Long, Qin wrote: > > > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Thursday, September 21, 2017 12:38 AM > To: Long, Qin <qin.l...@intel.com>; Ye, Ting <ting...@intel.com>; Zhang, Chao > B <chao.b.zh...@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 <ler...@redhat.com<mailto:ler...@redhat.com>> >> Cc: Ting Ye <ting...@intel.com<mailto:ting...@intel.com>> >> Cc: Chao Zhang <chao.b.zh...@intel.com<mailto:chao.b.zh...@intel.com>> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Qin Long <qin.l...@intel.com<mailto:qin.l...@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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
