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

Michael Kubacki posted 12 patches 3 years, 1 month ago
There is a newer version of this series
[edk2-devel] [PATCH v1 03/12] BaseTools/VfrCompile: Fix potential buffer overwrites
Posted by Michael Kubacki 3 years, 1 month ago
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 03/12] BaseTools/VfrCompile: Fix potential buffer overwrites
Posted by Michael D Kinney 3 years ago
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]
> -=-=-=-=-=-=
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#96594): https://edk2.groups.io/g/devel/message/96594
Mute This Topic: https://groups.io/mt/94918087/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-