From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.136; helo=mga12.intel.com; envelope-from=jaben.carsey@intel.com; receiver=edk2-devel@lists.01.org Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) (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 DB9DB222A334B for ; Thu, 8 Feb 2018 07:16:33 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 08 Feb 2018 07:22:18 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,479,1511856000"; d="scan'208";a="18306519" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga002.fm.intel.com with ESMTP; 08 Feb 2018 07:22:18 -0800 Received: from fmsmsx158.amr.corp.intel.com (10.18.116.75) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 8 Feb 2018 07:22:18 -0800 Received: from fmsmsx103.amr.corp.intel.com ([169.254.2.47]) by fmsmsx158.amr.corp.intel.com ([169.254.15.248]) with mapi id 14.03.0319.002; Thu, 8 Feb 2018 07:22:18 -0800 From: "Carsey, Jaben" To: "Ni, Ruiyu" , "edk2-devel@lists.01.org" Thread-Topic: [PATCH] ShellPkg/hexedit: Fix a read-after-free bug Thread-Index: AQHToJeVUR50G3D2YU+QHjgsix9FQqOan//g Date: Thu, 8 Feb 2018 15:22:17 +0000 Message-ID: References: <20180208044454.91652-1-ruiyu.ni@intel.com> In-Reply-To: <20180208044454.91652-1-ruiyu.ni@intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiOThhZWMyZTAtMjUzYi00M2JkLWI3NTYtMmVjM2U2MDY1NDdmIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6IlpiZ3BFUlBVTWJnUUlvMTFXM01BZmRyXC9NOEd6dE13RWxreUxGejlDM05rPSJ9 x-ctpclassification: CTP_NT x-originating-ip: [10.1.200.107] MIME-Version: 1.0 Subject: Re: [PATCH] ShellPkg/hexedit: Fix a read-after-free bug X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 08 Feb 2018 15:16:34 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Jaben Carsey > -----Original Message----- > From: Ni, Ruiyu > Sent: Wednesday, February 07, 2018 8:45 PM > To: edk2-devel@lists.01.org > Cc: Carsey, Jaben > Subject: [PATCH] ShellPkg/hexedit: Fix a read-after-free bug > Importance: High >=20 > HDiskImageSetDiskNameOffsetSize() and HFileImageSetFileName() > may be called using the current disk name or file name. > When this happens, today's implementation firstly frees the memory > and then accesses the just-freed memory. > The patch fixes this issue by doing nothing when the disk or file > name is the current one. >=20 > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ruiyu Ni > Cc: Jaben Carsey > --- > .../UefiShellDebug1CommandsLib/HexEdit/DiskImage.c | 22 +++++++++----- > ------- > .../UefiShellDebug1CommandsLib/HexEdit/FileImage.c | 23 +++++++++------ > ------- > 2 files changed, 18 insertions(+), 27 deletions(-) >=20 > diff --git > a/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/DiskImage.c > b/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/DiskImage.c > index 846b102975..8deb643f07 100644 > --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/DiskImage.c > +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/DiskImage.c > @@ -1,7 +1,7 @@ > /** @file > Functions to deal with Disk buffer. >=20 > - Copyright (c) 2005 - 2016, Intel Corporation. All rights reserved. > + Copyright (c) 2005 - 2018, Intel Corporation. All rights reserved. > This program and the accompanying materials > are licensed and made available under the terms and conditions of the = BSD > License > which accompanies this distribution. The full text of the license may= be > found at > @@ -120,27 +120,23 @@ HDiskImageSetDiskNameOffsetSize ( > IN UINTN Size > ) > { > - UINTN Len; > - UINTN Index; > + if (Str =3D=3D HDiskImage.Name) { > + // > + // This function might be called using HDiskImage.FileName as Str. > + // Directly return without updating HDiskImage.FileName. > + // > + return EFI_SUCCESS; > + } >=20 > // > // free the old file name > // > SHELL_FREE_NON_NULL (HDiskImage.Name); > - > - Len =3D StrLen (Str); > - > - HDiskImage.Name =3D AllocateZeroPool (2 * (Len + 1)); > + HDiskImage.Name =3D AllocateCopyPool (StrSize (Str), Str); > if (HDiskImage.Name =3D=3D NULL) { > return EFI_OUT_OF_RESOURCES; > } >=20 > - for (Index =3D 0; Index < Len; Index++) { > - HDiskImage.Name[Index] =3D Str[Index]; > - } > - > - HDiskImage.Name[Len] =3D L'\0'; > - > HDiskImage.Offset =3D Offset; > HDiskImage.Size =3D Size; >=20 > diff --git > a/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/FileImage.c > b/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/FileImage.c > index 2517a57f59..d9fd72cdd2 100644 > --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/FileImage.c > +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/HexEdit/FileImage.c > @@ -1,7 +1,7 @@ > /** @file > Functions to deal with file buffer. >=20 > - Copyright (c) 2005 - 2015, Intel Corporation. All rights reserved. > + Copyright (c) 2005 - 2018, Intel Corporation. All rights reserved. > This program and the accompanying materials > are licensed and made available under the terms and conditions of the = BSD > License > which accompanies this distribution. The full text of the license may= be > found at > @@ -110,27 +110,22 @@ HFileImageSetFileName ( > IN CONST CHAR16 *Str > ) > { > - UINTN Size; > - UINTN Index; > - > + if (Str =3D=3D HFileImage.FileName) { > + // > + // This function might be called using HFileImage.FileName as Str. > + // Directly return without updating HFileImage.FileName. > + // > + return EFI_SUCCESS; > + } > // > // free the old file name > // > SHELL_FREE_NON_NULL (HFileImage.FileName); > - > - Size =3D StrLen (Str); > - > - HFileImage.FileName =3D AllocateZeroPool (2 * (Size + 1)); > + HFileImage.FileName =3D AllocateCopyPool (StrSize (Str), Str); > if (HFileImage.FileName =3D=3D NULL) { > return EFI_OUT_OF_RESOURCES; > } >=20 > - for (Index =3D 0; Index < Size; Index++) { > - HFileImage.FileName[Index] =3D Str[Index]; > - } > - > - HFileImage.FileName[Size] =3D L'\0'; > - > return EFI_SUCCESS; > } >=20 > -- > 2.16.1.windows.1