From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 96F1F21ECCB0F for ; Wed, 20 Sep 2017 10:09:12 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 21A30C0828B5; Wed, 20 Sep 2017 17:12:18 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 21A30C0828B5 Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-23.rdu2.redhat.com [10.10.120.23]) by smtp.corp.redhat.com (Postfix) with ESMTP id E21FD6031F; Wed, 20 Sep 2017 17:12:16 +0000 (UTC) To: "Long, Qin" , "Ye, Ting" , "Zhang, Chao B" Cc: "edk2-devel@lists.01.org" References: <20170920160515.6792-1-qin.long@intel.com> From: Laszlo Ersek Message-ID: <0b3f49fc-a3b3-d0ac-1e8f-85fed954014b@redhat.com> Date: Wed, 20 Sep 2017 19:12:15 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Wed, 20 Sep 2017 17:12:18 +0000 (UTC) Subject: Re: [PATCH v2] CryptoPkg: Add new API to retrieve commonName of X.509 certificate X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 20 Sep 2017 17:09:12 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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 ; Ye, Ting ; Zhang, Chao B > 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 > >> Cc: Ting Ye > >> Cc: Chao Zhang > >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Qin Long > >> --- >> 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