From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1mOj74-00050f-T8 for mharc-grub-devel@gnu.org; Fri, 10 Sep 2021 12:10:50 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:34212) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mOj72-0004yH-SK for grub-devel@gnu.org; Fri, 10 Sep 2021 12:10:48 -0400 Received: from mail-qt1-x82b.google.com ([2607:f8b0:4864:20::82b]:36557) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mOj70-0004WG-Ba for grub-devel@gnu.org; Fri, 10 Sep 2021 12:10:48 -0400 Received: by mail-qt1-x82b.google.com with SMTP id d11so1888837qtw.3 for ; Fri, 10 Sep 2021 09:10:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=efficientek-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references:reply-to :mime-version:content-transfer-encoding; bh=xhX0a9CQiqOTU4o//wtmZb59f0x5lTVQpDuIqY64wNA=; b=HWGnAdEjErb+Qycs+sXL2Cub9zKZt9Gks0pIQwxDEsF0gEVE1Y9pyN/tenz/9QmFdD HNy6oxQF5T6t64C/AptEqti7IFfrGXmfictWj1vQ4VNzauCM0HQCHUne1GecuNLiH0NM wQAnM7xkWyIowgm8eQDz8LMR7LxK1A8Q4uBYK6SqjyCp0V6S1KYzGpCeF7XZXGmB9U/y m0umSyaQheBm6Uv+6Kxuh/6Z30Ma2E5LreL53b9Sxc6nN7BtougfDnORZ9yR6lLK9w5f m1GNPwP07znX49YOfY8RvapXstPT6oEjvH8sKsddAtw9e6DCo8WMTfbLuJQtUFe/TmKa kNHQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:reply-to:mime-version:content-transfer-encoding; bh=xhX0a9CQiqOTU4o//wtmZb59f0x5lTVQpDuIqY64wNA=; b=L2Rp8mW0+/qPdhfW/yaMKT8A417tQa31VYbAFDVOaOc93TyzS6De188xTJymgJD83c VSbtHzpXqWRFO9ZJOWVCcE1CDYzLaGgyvKaOJyeqfSV2mZdVMmnPLbrnp1J2+zWyqHau t3Z3MeGZXBFu3lC8GSj4ZrzakDcDPeKSwwBM8y54QydAE1Ec37MYPQb2pE/Ws7SQ0XMN R6EK+USTh+AnL+bv6XGraJXoYnUSpAJqDvZfQKPtxuFjNdx7cfAW1PIuWYZi9XKpd3DG RYHEkKKtPsPzPb898bIByYDzT5guw/ToWkcaOW06G60oc+ehB5yBim8aIyYchLLAg1Cw /gYA== X-Gm-Message-State: AOAM53043RxEagvaVmB5U1ZlGT4xwqG67PyDV+m48lxYzqRTCBuUAXOs eURp/wXe57PJcE+DhaWNtuaIrQ== X-Google-Smtp-Source: ABdhPJy0Qnhsp6CS53chbKMSYSkYU3idAOE8fuDHYpj9dloQ/yDWz2Xqxo4xW7ppwMlJP8FdSZUbbg== X-Received: by 2002:ac8:5417:: with SMTP id b23mr9123186qtq.140.1631290244424; Fri, 10 Sep 2021 09:10:44 -0700 (PDT) Received: from ubuntu ([199.58.83.9]) by smtp.gmail.com with ESMTPSA id a9sm4005136qkk.82.2021.09.10.09.10.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 10 Sep 2021 09:10:44 -0700 (PDT) Date: Fri, 10 Sep 2021 16:10:33 +0000 From: Glenn Washburn To: Daniel Kiper Cc: The development of GNU GRUB , 93sam@debian.org, alexander.burmashev@oracle.com, amakhalov@vmware.com, chris.coulson@canonical.com, cjwatson@debian.org, cperry@redhat.com, darren.kenny@oracle.com, darren.moffat@oracle.com, dave.miner@oracle.com, degranit@microsoft.com, eric.snowberg@oracle.com, ilya.okomin@oracle.com, jan.setjeeilers@oracle.com, jerecox@microsoft.com, jesse@eclypsium.com, john.haxby@oracle.com, kanth.ghatraju@oracle.com, konrad.wilk@oracle.com, mbenatto@redhat.com, mickey@eclypsium.com, msrc57813grub@microsoft.com, phcoder@gmail.com, pjones@redhat.com, sajacobu@microsoft.com, todd.vierling@oracle.com, xnox@ubuntu.com Subject: Re: [SECURITY PATCH 05/28] malloc: Use overflow checking primitives where we do complex allocations Message-ID: <20210910161033.1f79f838@ubuntu> In-Reply-To: <20200729170041.14082-6-daniel.kiper@oracle.com> References: <20200729170041.14082-1-daniel.kiper@oracle.com> <20200729170041.14082-6-daniel.kiper@oracle.com> Reply-To: development@efficientek.com X-Mailer: Claws Mail 3.17.8 (GTK+ 2.24.33; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Received-SPF: pass client-ip=2607:f8b0:4864:20::82b; envelope-from=development@efficientek.com; helo=mail-qt1-x82b.google.com X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 10 Sep 2021 16:10:49 -0000 On Wed, 29 Jul 2020 19:00:18 +0200 Daniel Kiper wrote: > From: Peter Jones > > This attempts to fix the places where we do the following where > arithmetic_expr may include unvalidated data: > > X = grub_malloc(arithmetic_expr); > > It accomplishes this by doing the arithmetic ahead of time using grub_add(), > grub_sub(), grub_mul() and testing for overflow before proceeding. ... > diff --git a/grub-core/fs/udf.c b/grub-core/fs/udf.c > index a83761674..21ac7f446 100644 > --- a/grub-core/fs/udf.c > +++ b/grub-core/fs/udf.c > @@ -28,6 +28,7 @@ > #include > #include > #include > +#include > > GRUB_MOD_LICENSE ("GPLv3+"); > > @@ -890,9 +891,19 @@ read_string (const grub_uint8_t *raw, grub_size_t sz, char *outbuf) > utf16[i] = (raw[2 * i + 1] << 8) | raw[2*i + 2]; > } > if (!outbuf) > - outbuf = grub_malloc (utf16len * GRUB_MAX_UTF8_PER_UTF16 + 1); > + { > + grub_size_t size; > + > + if (grub_mul (utf16len, GRUB_MAX_UTF8_PER_UTF16, &size) || > + grub_add (size, 1, &size)) > + goto fail; > + > + outbuf = grub_malloc (size); > + } > if (outbuf) > *grub_utf16_to_utf8 ((grub_uint8_t *) outbuf, utf16, utf16len) = '\0'; > + > + fail: > grub_free (utf16); > return outbuf; > } > @@ -1005,7 +1016,7 @@ grub_udf_read_symlink (grub_fshelp_node_t node) > grub_size_t sz = U64 (node->block.fe.file_size); > grub_uint8_t *raw; > const grub_uint8_t *ptr; > - char *out, *optr; > + char *out = NULL, *optr; > > if (sz < 4) > return NULL; > @@ -1013,14 +1024,16 @@ grub_udf_read_symlink (grub_fshelp_node_t node) > if (!raw) > return NULL; > if (grub_udf_read_file (node, NULL, NULL, 0, sz, (char *) raw) < 0) > - { > - grub_free (raw); > - return NULL; > - } > + goto fail_1; > > - out = grub_malloc (sz * 2 + 1); > + if (grub_mul (sz, 2, &sz) || > + grub_add (sz, 1, &sz)) The old code did not change the value of sz, but the new code does. This is a problem because sz is used further down. This is causing udf symlinks to not be accessible via grub and is causing the udf tests to fail on the symlink test. > + goto fail_0; > + > + out = grub_malloc (sz); > if (!out) > { > + fail_0: > grub_free (raw); > return NULL; > } > @@ -1031,17 +1044,17 @@ grub_udf_read_symlink (grub_fshelp_node_t node) > { > grub_size_t s; > if ((grub_size_t) (ptr - raw + 4) > sz) > - goto fail; > + goto fail_1; > if (!(ptr[2] == 0 && ptr[3] == 0)) > - goto fail; > + goto fail_1; > s = 4 + ptr[1]; > if ((grub_size_t) (ptr - raw + s) > sz) > - goto fail; > + goto fail_1; > switch (*ptr) > { > case 1: > if (ptr[1]) > - goto fail; > + goto fail_1; > /* Fallthrough. */ > case 2: > /* in 4 bytes. out: 1 byte. */ > @@ -1066,11 +1079,11 @@ grub_udf_read_symlink (grub_fshelp_node_t node) > if (optr != out) > *optr++ = '/'; > if (!read_string (ptr + 4, s - 4, optr)) > - goto fail; > + goto fail_1; > optr += grub_strlen (optr); > break; > default: > - goto fail; > + goto fail_1; > } > ptr += s; > } > @@ -1078,7 +1091,7 @@ grub_udf_read_symlink (grub_fshelp_node_t node) > grub_free (raw); > return out; > > - fail: > + fail_1: > grub_free (raw); > grub_free (out); > grub_error (GRUB_ERR_BAD_FS, "invalid symlink"); I actually noticed in my CI tests that the UDF tests were failing before the 2.06 release, but I assumed it was something long standing and didn't look into what was causing it. Here's another example of how my CI and testing improvements could've made a more solid release. Glenn