From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id AEECEC433F5 for ; Wed, 22 Dec 2021 15:56:01 +0000 (UTC) Received: from localhost ([::1]:34488 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1n03yC-0000mI-Fz for qemu-devel@archiver.kernel.org; Wed, 22 Dec 2021 10:56:00 -0500 Received: from eggs.gnu.org ([209.51.188.92]:50140) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1n03wZ-0008Gi-BZ for qemu-devel@nongnu.org; Wed, 22 Dec 2021 10:54:23 -0500 Received: from mx0a-00082601.pphosted.com ([67.231.145.42]:59716) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1n03wW-0007ii-Jg for qemu-devel@nongnu.org; Wed, 22 Dec 2021 10:54:18 -0500 Received: from pps.filterd (m0148461.ppops.net [127.0.0.1]) by mx0a-00082601.pphosted.com (8.16.1.2/8.16.1.2) with ESMTP id 1BMAq1bt028725; Wed, 22 Dec 2021 07:54:11 -0800 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fb.com; h=from : to : cc : subject : date : message-id : references : in-reply-to : content-type : mime-version; s=facebook; bh=K6JCiKBlYbvIGrBkAPTuw8vCG3jeCftrWR04sSbu6CY=; b=f65tcHdDbLI/AK8G9PWflkN71pvEHSWP2cW3L34mBloTNyuFhUOtfSqU6yS4Ea6JDjC2 IinH9r7OkQ0pCdO6g35y4T0wK4NP3cpYH2XyDPB8g2rfW2CuaO2WpNBMVryMWES0P8g1 y5/DGV289bCz4iR6YA9mF9t2Fi8h0SzdjtI= Received: from mail.thefacebook.com ([163.114.132.120]) by mx0a-00082601.pphosted.com (PPS) with ESMTPS id 3d42mxhuyy-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NOT); Wed, 22 Dec 2021 07:54:10 -0800 Received: from NAM12-MW2-obe.outbound.protection.outlook.com (100.104.98.9) by o365-in.thefacebook.com (100.104.94.230) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2308.20; Wed, 22 Dec 2021 07:54:09 -0800 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=liD2VH/BeXD4Ck9HhiQuuNWmqUNW21biUBYrxXqa4V1CVhVtf5A9u6IlDA0UseaEB7RiPgRD9DyGHnX8npcDp4L2M6AiOJ1of0/2dT5lO7kMGmNQ2zsgl/94YtX4Hk/t6nOqdAPldbyT0cgAsUz4FwJLOUOLGAhbHqD3e8+obXe32QQfxcJvgp1tvBeP95b6M5ChtsWwrDEa2AWEFAgaPgQD1kCIkPzVCBqR6ZCHM7R1pj5eGFOS/5+FImocu4MJ+PIY4lpxmeKijbDGkZCNNJ1tgZjle2HZ5kzh5bkik/X3UfEEKSyznb0NSENv5CGxSnbr3UfSiLUyWR+tY5O/pw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=K6JCiKBlYbvIGrBkAPTuw8vCG3jeCftrWR04sSbu6CY=; b=Mpc3X4KchetdQb18ab5pYQf9poJSiHGIxh5fWIECmpqO9dzS101wFSSTUiAZmG06wnUXHpH82oEwJMGhd4ZP3x7NolNq8HgU49r6RVcvwFMjZ5iBKxD7u23BaDkhsilWJ9yVXCMoQoBYRd6gA2dPV0yFjusH6y5kRzZtSDNP7jQ7EyigecSnRurnIYUVghlLQd1a0ci8SS/2gSW67TME9NxXlOJuA4BLYOAOrjTYqwQRaGxnKuitDC4kHmToQNVXclwdMemcj9v6BkP3RLsfZ1I1Bis/lGuLS+uFFpazH1ERcoRcBWTL0Vg6//98pl3vqbHMhMo+Cn3IcUMmEtXIIA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=fb.com; dmarc=pass action=none header.from=fb.com; dkim=pass header.d=fb.com; arc=none Received: from BY5PR15MB3572.namprd15.prod.outlook.com (2603:10b6:a03:1b2::24) by BYAPR15MB3365.namprd15.prod.outlook.com (2603:10b6:a03:111::24) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4801.17; Wed, 22 Dec 2021 15:54:08 +0000 Received: from BY5PR15MB3572.namprd15.prod.outlook.com ([fe80::84c5:3255:12c:bd58]) by BY5PR15MB3572.namprd15.prod.outlook.com ([fe80::84c5:3255:12c:bd58%6]) with mapi id 15.20.4801.020; Wed, 22 Dec 2021 15:54:08 +0000 From: Henry Kleynhans To: Henry Kleynhans , "qemu-devel@nongnu.org" CC: "berrange@redhat.com" , "henry.kleynhans@fb.com" Subject: Re: [PATCH 2/2] [crypto] Only verify CA certs in chain of trust Thread-Topic: [PATCH 2/2] [crypto] Only verify CA certs in chain of trust Thread-Index: AQHX90V9z35174OBTEaBiwicj7+5waw+pwHE Date: Wed, 22 Dec 2021 15:54:08 +0000 Message-ID: References: <20211222150600.37677-1-henry.kleynhans@gmail.com> <20211222150600.37677-2-henry.kleynhans@gmail.com> In-Reply-To: <20211222150600.37677-2-henry.kleynhans@gmail.com> Accept-Language: en-GB, en-US Content-Language: en-GB X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 270cfbe2-34d2-4f36-2ce6-08d9c5634a27 x-ms-traffictypediagnostic: BYAPR15MB3365:EE_ x-microsoft-antispam-prvs: x-fb-source: Internal x-ms-oob-tlc-oobclassifiers: OLM:6430; x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: T+BwnNrDySLf0TzEs30XfIUuXv32GVsf1GJd8rw9yUS0Q1kUGjJFdYXD4BXzrOjfYmKk0HWsZKoZueLbqa3VNnMDY35r9Zu5qntoAiUmtLe8sp8N7jBbWIhvdOqRnc5nDAtd7hXB/Rfhh8k+9Eaj7m+TK+e5KmHAvR1yHq34TetGtNUoAsT3eQFheOf8XRGnGpPJt6xOWPM1d3jfoGq34WaS8sMSJC7ui4VZu5+FBtx48ioziIe0M98d3xSh5XbFtHEdC412FO08wRhh/PY3DJoM3JRHRS9OJV5iWH9lPLf/yUzQBcbLwtOWbyD9aI6QL/eaoJZljqA4BxqQJ8ZDky9HDL6lcZOPxcrowlVG6c+Kc90a6cvXsk61jPMuDa5OfqrirwRLH+vGX26iPV0QzniGxVcGdQzQht4szaTHO1nUzRB1DBPwu2sDSd3t4ScCLJ5MOCOvglE/tjZLB2Rnj28rTCHxWWSmQOP0vwIGmPrjWvoW8ivDz5aZvmPyL1QCgR5eCXWgpMh9HdPg3DFLZlhMfwqi0e7B19xrHnCNhfxl6TDT3A5XO/XuWaR3YMzSCh3P5srrUBr6kCXh721Vnc1RVeYBt0Y9gOKqNYHAeRkGA3yFCLhGAdo8Fa8OYZf3g5R3hvV7cCAnvPnaokhBLR6SbgGWPgQFybtoJpiYvBY+d2CUk6XdFjxNzhC2s70+2v5pY/9ccgNydlrMmp1vPg== x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:BY5PR15MB3572.namprd15.prod.outlook.com; PTR:; CAT:NONE; SFS:(4636009)(366004)(6506007)(53546011)(7696005)(8936002)(316002)(54906003)(110136005)(186003)(55016003)(9326002)(508600001)(122000001)(38070700005)(86362001)(5660300002)(38100700002)(33656002)(4326008)(9686003)(83380400001)(66446008)(64756008)(66556008)(66476007)(15650500001)(8676002)(52536014)(66946007)(91956017)(2906002)(76116006)(71200400001); DIR:OUT; SFP:1102; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?te3MKHCWBSF7qdU87hp+6iT3c60WnEqEjOnBljHeVjADddD3FpN4liTEJ7Or?= =?us-ascii?Q?4q4JOESbVpnQStfANnOnovkHyJxkoV4oIHCLOBLdiQCZPerSFcw/c/g2rXfF?= =?us-ascii?Q?R01MC4zpRlJy01AF3cQxiQlQlMev3M64F8iY9hmr9nn0JossMgVsyruB3HVq?= =?us-ascii?Q?1lCoVP6kSxLmt3tpuJhkACEoyzGcTe1bevjrAQr9r4OkGFkpmFKyI9+w0/b3?= =?us-ascii?Q?DHbuz8XQ0NuOtQGHGfi/4PXHSkkr7LXfvOHDy/77CQHLCgwobBvCL75ulubj?= =?us-ascii?Q?citpg0ZfXDF0oRGjLHLxlBUtUs+4aCUYeXbKXauwmVdfIUoDMVtI/jadOIlq?= =?us-ascii?Q?NjKLscAKEi8iadozTU3FqIhGf1IkgpbaeUoYCmGNNdqK1ck1+5w4dpkXhZLo?= =?us-ascii?Q?uNkJz1+9DzugAiS/mridjGJX/gdd3Oo74ejzq+2dUIWbP6qbZoY4Fc2hidmz?= =?us-ascii?Q?t9Qr3qcSsuzmtqh4glxHUR+ysA3ACpSS6FmNjdDe4T4fhrTocmkMqGAMcDji?= =?us-ascii?Q?VhVuvnoJCOB/k1/3VW4qBHocDDMZiWIak+7NWDksHVnpLjEvvBDABpWWjUsT?= =?us-ascii?Q?uQ6ckQXAWjsM1YstS6OY91HrGqzTZLDm8rI1ulFeKln6zdYxiDY0sQxn0U0q?= =?us-ascii?Q?wk4jKCMeD/QrqkguFpNCrchpQ2vvTiiB7r5NJPHV9JX4LomJI4xubf90AajS?= =?us-ascii?Q?jJO9lirLTv0NRReaqFlnbI3dyx6Q6zBK1HHCjTywVnPSVN0tkgoQ9mb88vIj?= =?us-ascii?Q?ZuchZ3i3QnbImYccrQbjoXtPhs7taIXpWNYMRYLok8Azh+TrrhSQkyl8+Lhz?= =?us-ascii?Q?r0NtUqJo8bpO9uZYE/CXqcWvN+/jg9gchli93p9RkR9+fWSl1YE4XyawfVUc?= =?us-ascii?Q?hKQ0+O1GbrqFm3GTiBnUeFOpVjxUJchoq0G/38Sauub54AkXgEvHnbKLachR?= =?us-ascii?Q?LXanAdn86z2SvwKHqmGcGX0DYOqzh/Pix7SdiDal70EeZZQWrHgaTb5YVAw3?= =?us-ascii?Q?ocVTUB2cQpf5GSxP9IHwy9E0dTVgX36XNvHsIPz/PJWSJV7AKEanxHzNx/hM?= =?us-ascii?Q?Gzo77UwPLmL8jIkO2Qx6+O0xuDNHP+QYJPFJIT/olvJ2Wnf9N9TbOv1pHUQD?= =?us-ascii?Q?iK9MsTWRfU066SeI9T6jgOY/Ex0+8MJ66xEAo6pNms4FLtdZz3h2a+IibKdJ?= =?us-ascii?Q?ir5OLZxkbc8B/CbELOPUy6iJ8sIIvxTgWsxFDvAIRZiSwWvoA7AxoQ1JPoiH?= =?us-ascii?Q?mlob7lyNdoXU8v8EQ6VKpDRcwrCSgb2TLDB4k3TXxshI8LZfcwoK+oV51Jer?= =?us-ascii?Q?lsmDDPC5SgxJQDj/uxatyyj0nFv8Xd8f+PMZvG4ee5RHSup6coBpsjS6U7SA?= =?us-ascii?Q?WRpKszE8kMri1WUwJ6vEwm4qKPdBQ9CANyPLDLVCVoJ+oks0ocZcIpdsIREZ?= =?us-ascii?Q?w05P1LQ9QjD5COowTjm2roFG6u7EpohYdurOgqvd7nXMRP8rdhk/w4Ze43BY?= =?us-ascii?Q?5SXBSEDu7QRjF6tdCK4v062c1QC4mPtv8imTUcoOSy149QSkmiSmovThSDJ9?= =?us-ascii?Q?uVGlwKUB0O0WKwYC+3RUeJTu/Kj/ANTAJ62GmUyUZn6EO1a+CVj9j88lotWm?= =?us-ascii?Q?S080k0SS9ojHfYfUaic75B2j6Q1STS2cCR05M3dfz5ofYl8MAw/FIlajoFxz?= =?us-ascii?Q?R+N3ww=3D=3D?= Content-Type: multipart/alternative; boundary="_000_BY5PR15MB3572B35B9EEFE823B9F1270FB87D9BY5PR15MB3572namp_" MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: BY5PR15MB3572.namprd15.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 270cfbe2-34d2-4f36-2ce6-08d9c5634a27 X-MS-Exchange-CrossTenant-originalarrivaltime: 22 Dec 2021 15:54:08.5213 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: hFEllqbtFoBQZcv/4jTeXapGHizj8MX13nZjxJlcrW7kZLZmO8pHaI9oi8MALQVqosu8/iKax3uDM8ZvSaHa5Q== X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR15MB3365 X-OriginatorOrg: fb.com X-Proofpoint-ORIG-GUID: F7FFh83L4YyriGmYnhg1WQfPo23C4TwY X-Proofpoint-GUID: F7FFh83L4YyriGmYnhg1WQfPo23C4TwY X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.205,Aquarius:18.0.790,Hydra:6.0.425,FMLib:17.11.62.513 definitions=2021-12-22_07,2021-12-22_01,2021-12-02_01 X-Proofpoint-Spam-Details: rule=fb_outbound_notspam policy=fb_outbound score=0 adultscore=0 clxscore=1015 mlxlogscore=999 lowpriorityscore=0 priorityscore=1501 suspectscore=0 spamscore=0 impostorscore=0 malwarescore=0 mlxscore=0 bulkscore=0 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000 definitions=main-2112220090 X-FB-Internal: deliver Received-SPF: pass client-ip=67.231.145.42; envelope-from=prvs=19906fbbc0=hkleynhans@fb.com; helo=mx0a-00082601.pphosted.com X-Spam_score_int: -29 X-Spam_score: -3.0 X-Spam_bar: --- X-Spam_report: (-3.0 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.203, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H3=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --_000_BY5PR15MB3572B35B9EEFE823B9F1270FB87D9BY5PR15MB3572namp_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Daniel, This patch tightens the CA verification code to only check the issuer chain= of the client cert. I think this will still not catch expired/invalid cer= ts if the client and server certs have different issuer chains; so maybe th= is too is not quite the correct fix. Let me know what you think. Kind regards, Henry From: Henry Kleynhans Date: Wednesday, 22 December 2021 at 15:06 To: qemu-devel@nongnu.org Cc: berrange@redhat.com , henry.kleynhans@fb.com , Henry Kleynhans Subject: [PATCH 2/2] [crypto] Only verify CA certs in chain of trust From: Henry Kleynhans The CA file provided to qemu may contain CA certificates which do not form part of the chain of trust for the specific certificate we are sanity checking. This patch changes the sanity checking from validating every CA certificate to only checking the CA certificates which are part of the chain of trust (issuer chain). Other certificates are ignored. Signed-off-by: Henry Kleynhans --- crypto/tlscredsx509.c | 50 +++++++++++++++++++++++---- tests/unit/test-crypto-tlscredsx509.c | 25 +++++++++++++- 2 files changed, 68 insertions(+), 7 deletions(-) diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c index d061c68253..841f80aac6 100644 --- a/crypto/tlscredsx509.c +++ b/crypto/tlscredsx509.c @@ -315,6 +315,44 @@ qcrypto_tls_creds_check_cert(QCryptoTLSCredsX509 *cred= s, return 0; } +static int +qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds, + gnutls_x509_crt_t cert, + gnutls_x509_crt_t *cacerts, + unsigned int ncacerts, + const char *cacertFile, + bool isServer, + bool isCA, + Error **errp) +{ + gnutls_x509_crt_t *cert_to_check =3D &cert; + int checking_issuer =3D 1; + int retval =3D 0; + + while (checking_issuer) { + checking_issuer =3D 0; + + if (gnutls_x509_crt_check_issuer(*cert_to_check, *cert_to_check)) = { + /* The cert is self-signed indicating we have reached the root= of trust. */ + return qcrypto_tls_creds_check_cert(creds, *cert_to_check, cac= ertFile, + isServer, isCA, errp); + } + for (int i =3D 0; i < ncacerts; i++) { + if (gnutls_x509_crt_check_issuer(*cert_to_check, cacerts[i])) = { + retval =3D qcrypto_tls_creds_check_cert(creds, cacerts[i],= cacertFile, + isServer, isCA, errp= ); + if (retval < 0) { + return retval; + } + cert_to_check =3D &cacerts[i]; + checking_issuer =3D 1; + break; + } + } + } + + return -1; +} static int qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t cert, @@ -500,12 +538,12 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX5= 09 *creds, goto cleanup; } - for (i =3D 0; i < ncacerts; i++) { - if (qcrypto_tls_creds_check_cert(creds, - cacerts[i], cacertFile, - isServer, true, errp) < 0) { - goto cleanup; - } + if (cert && + qcrypto_tls_creds_check_authority_chain(creds, cert, + cacerts, ncacerts, + cacertFile, isServer, + true, errp) < 0) { + goto cleanup; } if (cert && ncacerts && diff --git a/tests/unit/test-crypto-tlscredsx509.c b/tests/unit/test-crypto= -tlscredsx509.c index aab4149b56..e4d657ba61 100644 --- a/tests/unit/test-crypto-tlscredsx509.c +++ b/tests/unit/test-crypto-tlscredsx509.c @@ -589,6 +589,12 @@ int main(int argc, char **argv) true, true, GNUTLS_KEY_KEY_CERT_SIGN, false, false, NULL, NULL, 0, 0); + TLS_CERT_REQ(cacertlevel1creq_invalid, cacertrootreq, + "UK", "qemu level 1c invalid", NULL, NULL, NULL, NULL, + true, true, true, + true, true, GNUTLS_KEY_KEY_CERT_SIGN, + false, false, NULL, NULL, + 360, 400); TLS_CERT_REQ(cacertlevel2areq, cacertlevel1areq, "UK", "qemu level 2a", NULL, NULL, NULL, NULL, true, true, true, @@ -617,16 +623,32 @@ int main(int argc, char **argv) cacertlevel2areq.crt, }; + test_tls_write_cert_chain(WORKDIR "cacertchain-ctx.pem", certchain, G_N_ELEMENTS(certchain)); + gnutls_x509_crt_t certchain_with_invalid[] =3D { + cacertrootreq.crt, + cacertlevel1areq.crt, + cacertlevel1breq.crt, + cacertlevel1creq_invalid.crt, + cacertlevel2areq.crt, + }; + + test_tls_write_cert_chain(WORKDIR "cacertchain-with-invalid-ctx.pem", + certchain_with_invalid, + G_N_ELEMENTS(certchain_with_invalid)); + TLS_TEST_REG(chain1, true, WORKDIR "cacertchain-ctx.pem", servercertlevel3areq.filename, false); TLS_TEST_REG(chain2, false, WORKDIR "cacertchain-ctx.pem", clientcertlevel2breq.filename, false); + TLS_TEST_REG(certchainwithexpiredcert, false, + WORKDIR "cacertchain-with-invalid-ctx.pem", + clientcertlevel2breq.filename, false); /* Some missing certs - first two are fatal, the last * is ok @@ -640,7 +662,6 @@ int main(int argc, char **argv) TLS_TEST_REG(missingclient, false, cacert1req.filename, "clientcertdoesnotexist.pem", false); - ret =3D g_test_run(); test_tls_discard_cert(&cacertreq); @@ -694,10 +715,12 @@ int main(int argc, char **argv) test_tls_discard_cert(&cacertrootreq); test_tls_discard_cert(&cacertlevel1areq); test_tls_discard_cert(&cacertlevel1breq); + test_tls_discard_cert(&cacertlevel1creq_invalid); test_tls_discard_cert(&cacertlevel2areq); test_tls_discard_cert(&servercertlevel3areq); test_tls_discard_cert(&clientcertlevel2breq); unlink(WORKDIR "cacertchain-ctx.pem"); + unlink(WORKDIR "cacertchain-with-invalid-ctx.pem"); test_tls_cleanup(KEYFILE); rmdir(WORKDIR); -- 2.34.1 --_000_BY5PR15MB3572B35B9EEFE823B9F1270FB87D9BY5PR15MB3572namp_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

Hi Daniel,

This patch tightens the CA verification code to only check the issuer chain= of the client cert.  I think this will still not catch expired/invali= d certs if the client and server certs have different issuer chains; so may= be this too is not quite the correct fix.  Let me know what you think.

Kind regards, Henry

 

From: Henry Kleynhans <= ;henry.kleynhans@gmail.com>
Date: Wednesday, 22 December 2021 at 15:06
To: qemu-devel@nongnu.org <qemu-devel@nongnu.org>
Cc: berrange@redhat.com <berrange@redhat.com>, henry.kleynhans= @fb.com <henry.kleynhans@fb.com>, Henry Kleynhans <hkleynhans@fb.c= om>
Subject: [PATCH 2/2] [crypto] Only verify CA certs in chain of trust=

From: Henry Kleynhans <hkleynhans@fb.com>

The CA file provided to qemu may contain CA certificates which do not
form part of the chain of trust for the specific certificate we are
sanity checking.

This patch changes the sanity checking from validating every CA
certificate to only checking the CA certificates which are part of the
chain of trust (issuer chain).  Other certificates are ignored.

Signed-off-by: Henry Kleynhans <hkleynhans@fb.com>
---
 crypto/tlscredsx509.c        =          | 50 +++++++++++++++++++++= ++----
 tests/unit/test-crypto-tlscredsx509.c | 25 +++++++++++++-
 2 files changed, 68 insertions(+), 7 deletions(-)

diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index d061c68253..841f80aac6 100644
--- a/crypto/tlscredsx509.c
+++ b/crypto/tlscredsx509.c
@@ -315,6 +315,44 @@ qcrypto_tls_creds_check_cert(QCryptoTLSCredsX509 *cred= s,
     return 0;
 }
 
+static int
+qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
+            &n= bsp;            = ;            &n= bsp;  gnutls_x509_crt_t cert,
+            &n= bsp;            = ;            &n= bsp;  gnutls_x509_crt_t *cacerts,
+            &n= bsp;            = ;            &n= bsp;  unsigned int ncacerts,
+            &n= bsp;            = ;            &n= bsp;  const char *cacertFile,
+            &n= bsp;            = ;            &n= bsp;  bool isServer,
+            &n= bsp;            = ;            &n= bsp;  bool isCA,
+            &n= bsp;            = ;            &n= bsp;  Error **errp)
+{
+    gnutls_x509_crt_t *cert_to_check =3D &cert;
+    int checking_issuer =3D 1;
+    int retval =3D 0;
+
+    while (checking_issuer) {
+        checking_issuer =3D 0;
+
+        if (gnutls_x509_crt_check_issue= r(*cert_to_check, *cert_to_check)) {
+            /* The = cert is self-signed indicating we have reached the root of trust. */
+            return = qcrypto_tls_creds_check_cert(creds, *cert_to_check, cacertFile,
+            &n= bsp;            = ;            &n= bsp;          isServer, isCA, = errp);
+        }
+        for (int i =3D 0; i < ncacer= ts; i++) {
+            if (gnu= tls_x509_crt_check_issuer(*cert_to_check, cacerts[i])) {
+            &n= bsp;   retval =3D qcrypto_tls_creds_check_cert(creds, cacerts[i],= cacertFile,
+            &n= bsp;            = ;            &n= bsp;            = ;    isServer, isCA, errp);
+            &n= bsp;   if (retval < 0) {
+            &n= bsp;       return retval;
+            &n= bsp;   }
+            &n= bsp;   cert_to_check =3D &cacerts[i];
+            &n= bsp;   checking_issuer =3D 1;
+            &n= bsp;   break;
+            }
+        }
+    }
+
+    return -1;
+}
 
 static int
 qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t cert,
@@ -500,12 +538,12 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX5= 09 *creds,
         goto cleanup;
     }
 
-    for (i =3D 0; i < ncacerts; i++) {
-        if (qcrypto_tls_creds_check_cer= t(creds,
-            &n= bsp;            = ;            &n= bsp;   cacerts[i], cacertFile,
-            &n= bsp;            = ;            &n= bsp;   isServer, true, errp) < 0) {
-            goto cl= eanup;
-        }
+    if (cert &&
+        qcrypto_tls_creds_check_authori= ty_chain(creds, cert,
+            &n= bsp;            = ;            &n= bsp;          cacerts, ncacert= s,
+            &n= bsp;            = ;            &n= bsp;          cacertFile, isSe= rver,
+            &n= bsp;            = ;            &n= bsp;          true, errp) <= 0) {
+        goto cleanup;
     }
 
     if (cert && ncacerts &&
diff --git a/tests/unit/test-crypto-tlscredsx509.c b/tests/unit/test-crypto= -tlscredsx509.c
index aab4149b56..e4d657ba61 100644
--- a/tests/unit/test-crypto-tlscredsx509.c
+++ b/tests/unit/test-crypto-tlscredsx509.c
@@ -589,6 +589,12 @@ int main(int argc, char **argv)
            &nb= sp;     true, true, GNUTLS_KEY_KEY_CERT_SIGN,
            &nb= sp;     false, false, NULL, NULL,
            &nb= sp;     0, 0);
+    TLS_CERT_REQ(cacertlevel1creq_invalid, cacertrootreq, +            &n= bsp;    "UK", "qemu level 1c invalid", N= ULL, NULL, NULL, NULL,
+            &n= bsp;    true, true, true,
+            &n= bsp;    true, true, GNUTLS_KEY_KEY_CERT_SIGN,
+            &n= bsp;    false, false, NULL, NULL,
+            &n= bsp;    360, 400);
     TLS_CERT_REQ(cacertlevel2areq, cacertlevel1areq,             &nb= sp;     "UK", "qemu level 2a", NULL= , NULL, NULL, NULL,
            &nb= sp;     true, true, true,
@@ -617,16 +623,32 @@ int main(int argc, char **argv)
         cacertlevel2areq.crt,
     };
 
+
     test_tls_write_cert_chain(WORKDIR "cacertchai= n-ctx.pem",
            &nb= sp;            =       certchain,
            &nb= sp;            =       G_N_ELEMENTS(certchain));
 
+    gnutls_x509_crt_t certchain_with_invalid[] =3D {
+        cacertrootreq.crt,
+        cacertlevel1areq.crt,
+        cacertlevel1breq.crt,
+        cacertlevel1creq_invalid.crt, +        cacertlevel2areq.crt,
+    };
+
+    test_tls_write_cert_chain(WORKDIR "cacertchain-wit= h-invalid-ctx.pem",
+            &n= bsp;            = ;     certchain_with_invalid,
+            &n= bsp;            = ;     G_N_ELEMENTS(certchain_with_invalid));
+
     TLS_TEST_REG(chain1, true,
            &nb= sp;     WORKDIR "cacertchain-ctx.pem",
            &nb= sp;     servercertlevel3areq.filename, false);
     TLS_TEST_REG(chain2, false,
            &nb= sp;     WORKDIR "cacertchain-ctx.pem",
            &nb= sp;     clientcertlevel2breq.filename, false);
+    TLS_TEST_REG(certchainwithexpiredcert, false,
+            &n= bsp;    WORKDIR "cacertchain-with-invalid-ctx.pem"= ,
+            &n= bsp;    clientcertlevel2breq.filename, false);
 
     /* Some missing certs - first two are fatal, the l= ast
      * is ok
@@ -640,7 +662,6 @@ int main(int argc, char **argv)
     TLS_TEST_REG(missingclient, false,
            &nb= sp;     cacert1req.filename,
            &nb= sp;     "clientcertdoesnotexist.pem", false);=
-
     ret =3D g_test_run();
 
     test_tls_discard_cert(&cacertreq);
@@ -694,10 +715,12 @@ int main(int argc, char **argv)
     test_tls_discard_cert(&cacertrootreq);
     test_tls_discard_cert(&cacertlevel1areq);
     test_tls_discard_cert(&cacertlevel1breq);
+    test_tls_discard_cert(&cacertlevel1creq_invalid);      test_tls_discard_cert(&cacertlevel2areq);
     test_tls_discard_cert(&servercertlevel3areq);<= br>      test_tls_discard_cert(&clientcertlevel2breq);<= br>      unlink(WORKDIR "cacertchain-ctx.pem"); +    unlink(WORKDIR "cacertchain-with-invalid-ctx.pem&q= uot;);
 
     test_tls_cleanup(KEYFILE);
     rmdir(WORKDIR);
--
2.34.1

--_000_BY5PR15MB3572B35B9EEFE823B9F1270FB87D9BY5PR15MB3572namp_--