From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by mx.groups.io with SMTP id smtpd.web11.5663.1571237027807369856 for ; Wed, 16 Oct 2019 07:43:48 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 522FE30014B9; Wed, 16 Oct 2019 14:43:47 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-232.ams2.redhat.com [10.36.116.232]) by smtp.corp.redhat.com (Postfix) with ESMTP id BC7C960852; Wed, 16 Oct 2019 14:43:45 +0000 (UTC) Subject: Re: [edk2-devel] [RFC v1 5/4] CryptoPkg/TlsLib: accept peer certs via both DNS names and IP addresses To: David Woodhouse , "Wu, Jiaxin" , "devel@edk2.groups.io" Cc: Bret Barkelew , "Wang, Jian J" , Richard Levitte , Sivaraman Nainar References: <20190927034441.3096-1-Jiaxin.wu@intel.com> <20191015230839.27708-1-lersek@redhat.com> <895558F6EA4E3B41AC93A00D163B727416F81251@SHSMSX107.ccr.corp.intel.com> <56d17f5f-8433-2ec5-924c-bade642ac5a7@redhat.com> <139da0c5a4684b76809fa19acc007f4699e3eb28.camel@infradead.org> From: "Laszlo Ersek" Message-ID: <81cf523b-1cc0-9df1-cbb3-c16a78e26a55@redhat.com> Date: Wed, 16 Oct 2019 16:43:44 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.40]); Wed, 16 Oct 2019 14:43:47 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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