Re: [edk2-devel] [PATCH v1 03/12] BaseTools/VfrCompile: Fix potential buffer overwrites

["Michael D Kinney" <michael.d.kinney@intel.com>]

Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
> Sent: Wednesday, November 9, 2022 9:33 AM
> To: devel@edk2.groups.io
> Cc: Feng, Bob C <bob.c.feng@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Chen, Christine <yuwei.chen@intel.com>
> Subject: [edk2-devel] [PATCH v1 03/12] BaseTools/VfrCompile: Fix potential buffer overwrites
> 
> From: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> While more portable methods exist to handle these cases, this change
> does not attempt to do more than fix the immediate problem and
> follow the conventions already established in this code.
> 
> `snprintf()` is introduced as the minimum improvement apart from
> making the buffers larger.
> 
> Fixes the following CodeQL alerts:
> 
> 1. Failure on line 2339 in
>    BaseTools/Source/C/VfrCompile/Pccts/antlr/gen.c
> 
>    - Type: Potentially overrunning write
>    - Severity: Critical
>    - Problem: This 'call to sprintf' operation requires 17 bytes but
>      the destination is only 16 bytes.
> 
> 2. Failure on line 2341 in
>    BaseTools/Source/C/VfrCompile/Pccts/antlr/gen.c
> 
>    - Type: Potentially overrunning write
>    - Severity: Critical
>    - Problem: This 'call to sprintf' operation requires 17 bytes but
>      the destination is only 16 bytes.
> 
> 3. Failure on line 1309 in
>    BaseTools/Source/C/VfrCompile/Pccts/antlr/main.c
> 
>    - Type: Potentially overrunning write
>    - Severity: Critical
>    - Problem: This 'call to sprintf' operation requires 25 bytes but
>      the destination is only 20 bytes.
> 
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Yuwei Chen <yuwei.chen@intel.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
>  BaseTools/Source/C/VfrCompile/Pccts/antlr/gen.c  | 10 +++++-----
>  BaseTools/Source/C/VfrCompile/Pccts/antlr/main.c |  4 ++--
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/BaseTools/Source/C/VfrCompile/Pccts/antlr/gen.c b/BaseTools/Source/C/VfrCompile/Pccts/antlr/gen.c
> index 8e41239f4751..33d9cac4c7de 100644
> --- a/BaseTools/Source/C/VfrCompile/Pccts/antlr/gen.c
> +++ b/BaseTools/Source/C/VfrCompile/Pccts/antlr/gen.c
> @@ -2331,14 +2331,14 @@ TokNode *p;
>  			set_nameErrSet = bufErrSet;								/* MR23 */
>  		}
>  		else {					/* wild card */
> -			static char buf[sizeof("zzerr")+10];
> -			static char bufErrSet[sizeof("zzerr")+10];
> +			static char buf[sizeof("zzerr")+11];
> +			static char bufErrSet[sizeof("zzerr")+11];
>  			int n = DefErrSet( &b, 0, NULL );
>  			int nErrSet = DefErrSetWithSuffix(0, &bErrSet, 1, NULL, "_set");
> -			if ( GenCC ) sprintf(buf, "err%d", n);
> -			else sprintf(buf, "zzerr%d", n);
> +			if ( GenCC ) snprintf(buf, 11, "err%d", n);
> +			else snprintf(buf, 11, "zzerr%d", n);
>  			if ( GenCC ) sprintf(bufErrSet, "err%d", nErrSet);
> -			else sprintf(bufErrSet, "zzerr%d", nErrSet);
> +			else snprintf(bufErrSet, 11, "zzerr%d", nErrSet);
>  			set_name = buf;
>  			set_nameErrSet = bufErrSet;
>  		}
> diff --git a/BaseTools/Source/C/VfrCompile/Pccts/antlr/main.c b/BaseTools/Source/C/VfrCompile/Pccts/antlr/main.c
> index 051ee4ec5d28..488b4b90461c 100644
> --- a/BaseTools/Source/C/VfrCompile/Pccts/antlr/main.c
> +++ b/BaseTools/Source/C/VfrCompile/Pccts/antlr/main.c
> @@ -1295,7 +1295,7 @@ int token;
>  #endif
>  {
>  	int     j;
> -        static    char    imag_name[20];
> +        static    char    imag_name[25];
> 
>  	/* look in all lexclasses for the token */
>  	if ( TokenString(token) != NULL ) return TokenString(token);
> @@ -1306,7 +1306,7 @@ int token;
>  	}
> 
>      if (1) {
> -      sprintf(imag_name,"UnknownToken#%d",token);           /* MR13 */
> +      snprintf(imag_name, 25, "UnknownToken#%d", token);    /* MR13 */
>        return imag_name;                                     /* MR13 */
>      }
> 
> --
> 2.28.0.windows.1
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#96149): https://edk2.groups.io/g/devel/message/96149
> Mute This Topic: https://groups.io/mt/94918087/1643496
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney@intel.com]
> -=-=-=-=-=-=
>