From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (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 3998F225FA065 for ; Tue, 10 Apr 2018 02:47:38 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C83CD406E8BD; Tue, 10 Apr 2018 09:47:37 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-125.rdu2.redhat.com [10.10.120.125]) by smtp.corp.redhat.com (Postfix) with ESMTP id 18DD010EE6D8; Tue, 10 Apr 2018 09:47:35 +0000 (UTC) To: "Wu, Jiaxin" , edk2-devel-01 Cc: Ard Biesheuvel , Gary Ching-Pang Lin , "Justen, Jordan L" , "Gao, Liming" , "Kinney, Michael D" , "Long, Qin" , "Fu, Siyuan" , "Ye, Ting" References: <20180403145149.8925-1-lersek@redhat.com> <895558F6EA4E3B41AC93A00D163B7274163B458F@SHSMSX103.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: Date: Tue, 10 Apr 2018 11:47:35 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <895558F6EA4E3B41AC93A00D163B7274163B458F@SHSMSX103.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Tue, 10 Apr 2018 09:47:37 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Tue, 10 Apr 2018 09:47:37 +0000 (UTC) for IP:'10.11.54.3' DOMAIN:'int-mx03.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH 00/13] {Ovmf, Mde, Network, Crypto}Pkg: fixes+features for setting HTTPS cipher suites X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 10 Apr 2018 09:47:39 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 04/10/18 06:09, Wu, Jiaxin wrote: > Hi Laszlo > > Appreciate your contribution. I have reviewed the series patches you attached here. First, I assume you have verified the patches on OVMF and the functionality works well, That's right; I tested cipher suite negotiation failures and successes. For example, I configured apache to "Disable All SSL and TLS Protocols Except TLS 1 and Up" , and then I verified that HTTPS boot would succeed vs. fail dependent on whether I passed strong ciphers too, or only weak ones, to OVMF. > then, below are my comments: > > 1. The patches for OvmfPkg/NetworkPkg (0001-0004) are good to me. Thanks. For patch #2, "MdePkg/Include/Protocol/Tls.h: pack structures from the TLS RFC", we exchanged some points with Liming earlier: http://mid.mail-archive.com/4A89E2EF3DFEDB4C8BFDE51014F606A14E1F1B10@SHSMSX104.ccr.corp.intel.com (Please see also my response to that.) I see that both you and Siyuan are OK with patch #2, i.e. with the separate #pragma directives. I'd also like Liming to confirm that he accepts the patch as-is. > > 2. For CryptoPkg, the major viewpoint is also related to the TlsCipherMappingTable. For this table, only include the supported ciphersuites looks more reasonable. OK. I think this means that I should preserve patches #5 through #7, and drop patches #8 ('CryptoPkg/TlsLib: add the "TlsMappingTable.sh" POSIX shell script') and #9 ('CryptoPkg/TlsLib: extend "TlsCipherMappingTable"'). Is that correct? > After talked with Qin, I know the unsupported cipher suites won't be rejected/filtrated by the OpenSSL cipher list setting, if so, the cipher suite list that passed from the client to the server in the ClientHello message might also include such unsupported cipher suites. In such a case, the failure will happen once the server select the unsupported cipher suite. From the handshake process view, it's unreasonable since the client sent the desired cipher suites, then the server selected one of them but still met the error. Oh! You are totally right. I apologize for missing this -- I didn't realize this from Qin's comments on TianoCore #915. In other words, it is actually *important* that "TlsCipherMappingTable" match the cipher suites that we build into edk2. I understand now. Thanks! > Anyway, filtrating the unsupported cipher suites as early as possible is a wise choice. So, TlsCipherMappingTable should only include the supported cipher suites by reference the security requirement of CryptoPkg. Yes. > > 3. For patch 0006, it's good to me to optimize the searching algorithm since the table is larger than before. > > 4. Can we combined some patches together to make the things simple? e.g. Patches 0005/0007/0010/0011/0012/0013. Those patches are the same purpose to fix the issues in 0013. I'm not against squashing these patches together, but separating patch #6 (the binary search) out of the middle is not possible without a rewrite of that patch, because it has context dependencies on patch #5. Do you want me to do that? I.e., first implement the binary search for TlsGetCipherString() -- without changing the interface --, and *then* switch it over to TlsGetCipherMapping(), as part of the large squashed patch? > > 5. For patch 0008, I think it's unnecessary to provide such script. I prefer to maintain the TlsCipherMappingTable more statical since it's the internal mapping table. How about we keep it as internal assistant tool? Sure, given that TlsCipherMappingTable *must* match the ciphers that we build into OpensslLib, updating that table semi-automatically is out of question (it can only be done manually, in synch with OpensslLib.inf changes). Therefore the shell script is useless. I'll just drop the patch. > > 6. For patch 0009 to extend the TlsCipherMappingTable, I think Qin can help us to provide the supported cipher suites by reference the security requirement of CryptoPkg. Right -- I think I'll simply drop patch #9 from the v2 series, and I'll let Qin post a separate patch later, so that TlsCipherMappingTable matches the ciphers we build in 100%. Qin, do you prefer that I open a separate TianoCore BZ for this? Also, would you like to post the patch for TlsCipherMappingTable before or after my v2? Thanks! Laszlo