Re: [edk2-devel] [RFC v1 5/4] CryptoPkg/TlsLib: accept peer certs via both DNS names and IP addresses

["Laszlo Ersek" <lersek@redhat.com>]

On 10/16/19 15:35, David Woodhouse wrote:
> On Wed, 2019-10-16 at 13:41 +0200, Laszlo Ersek wrote:

>> Anyway: we still have the issue that X509_VERIFY_PARAM_set_ip_asc()
>> appears to reject IPv4 address literals. Could you check that please?
>>
>> (Using a hosted (Linux userspace) program like "sconnect", it must be
>> easier to debug. I tried connecting gdb to QEMU, running OVMF, but it
>> crashed gdb. :)
> 
> Ah, but if you were using a hosted Linux userspace program like
> sconnect, then your sscanf() implementation wouldn't look like this:
> 
> $ grep -B1 -A4 sscanf CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
> /* Read formatted data from a string */
> int sscanf (const char *buffer, const char *format, ...)
> {
>   //
>   // Null sscanf() function implementation to satisfy the linker, since
>   // no direct functionality logic dependency in present UEFI cases.
>   //
>   return 0;
> }

Hahaha ROTFL :)

I have no clue why I didn't realize this :) I looked at the sscanf()
call, and it never occurred to me that sscanf() is a standard C function! :)

> I told you to stare hard at that, didn't I :)

You did!

> I'm sure that OpenSSL upstream would welcome a patch to ditch that use
> of the non-recommended sscanf() function and use inet_ntoa() where it's
> available instead (although that might sensibly be guarded on
> OPENSSL_NO_SOCK, which you set for the EDK2 build).

Hrmpf. Too many functions here, for OpenSSL proper:

- inet_addr() is in POSIX:

https://pubs.opengroup.org/onlinepubs/9699919799/functions/inet_addr.html

but its failure mode is not nice (the error value aliases 255.255.255.255).

- inet_aton() is good, but it's not in POSIX. (BSD extension)

- inet_pton() is good and in POSIX. Best choice?

(Not volunteering for the OpenSSL patch at the moment -- I have my hands
full, and I'd have to go through the CLA thingy first.)


Regarding the current edk2 patch set, I think we should do the following:

- use X509_VERIFY_PARAM_set1_ip() rather than
X509_VERIFY_PARAM_set1_ip_asc()

- incorporate "StdLib/BsdSocketLib/inet_pton.c" from the edk2-libc
project (which used to be part of edk2 itself) into TlsLib, and call
inet_pton() for parsing the address as both IPv4 and IPv6.

The source file mentioned above seems to depend only on the strchr() and
memcpy() functions, and "CryptoPkg/Library/Include/CrtLibSupport.h"
already provides macros for those.

Jiaxin, what's your opinion?

Thanks
Laszlo