diff mbox

uefi: uefirtvariable: catch a potential buffer overflow

Message ID 1425462354-2930-1-git-send-email-colin.king@canonical.com
State Rejected
Headers show

Commit Message

Colin Ian King March 4, 2015, 9:45 a.m. UTC
From: Colin Ian King <colin.king@canonical.com>

CoverityScan detected a potential buffer overflow if the returned
efi variable is larger than 10 bytes.  Silently truncate the var
size to the maximum size if it is too long - fwts has already
flagged up a failure that the variable size is wrong anyway, so
the silent truncate of an overly long variable is fine since the
user will know it is already the wrong size.  This truncation will
avoid the buffer overflow in the following byte comparison check.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 src/uefi/uefirtauthvar/uefirtauthvar.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Ivan Hu March 5, 2015, 3:34 a.m. UTC | #1
On 2015年03月04日 17:45, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> CoverityScan detected a potential buffer overflow if the returned
> efi variable is larger than 10 bytes.  Silently truncate the var
> size to the maximum size if it is too long - fwts has already
> flagged up a failure that the variable size is wrong anyway, so
> the silent truncate of an overly long variable is fine since the
> user will know it is already the wrong size.  This truncation will
> avoid the buffer overflow in the following byte comparison check.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/uefi/uefirtauthvar/uefirtauthvar.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/src/uefi/uefirtauthvar/uefirtauthvar.c b/src/uefi/uefirtauthvar/uefirtauthvar.c
> index 22c954b..e935de2 100644
> --- a/src/uefi/uefirtauthvar/uefirtauthvar.c
> +++ b/src/uefi/uefirtauthvar/uefirtauthvar.c
> @@ -415,6 +415,10 @@ static int uefirtauthvar_test5(fwts_framework *fw)
>   			"UEFIUpdateAuthVar",
>   			"Get authenticated variable data size is not the "
>   			"same as it set.");
> +
> +		/* Ensure we don't get a buffer overrun later on.. */
> +		if (getdatasize > sizeof(AuthVarUpdateData))
> +			getdatasize = sizeof(AuthVarUpdateData);
>   	}
>   
>   	for (i = 0; i < getdatasize; i++) {

Hi Colin,

Thanks for fixing this, I believe it is caused by that I forgot to 
return FWTS_ERROR.
When the test found that getting the test data is not the same as it 
set, it fails the test.
I'll send out the patch the return error to avoid the CoverityScan 
complaint.

Cheers,
Ivan
diff mbox

Patch

diff --git a/src/uefi/uefirtauthvar/uefirtauthvar.c b/src/uefi/uefirtauthvar/uefirtauthvar.c
index 22c954b..e935de2 100644
--- a/src/uefi/uefirtauthvar/uefirtauthvar.c
+++ b/src/uefi/uefirtauthvar/uefirtauthvar.c
@@ -415,6 +415,10 @@  static int uefirtauthvar_test5(fwts_framework *fw)
 			"UEFIUpdateAuthVar",
 			"Get authenticated variable data size is not the "
 			"same as it set.");
+
+		/* Ensure we don't get a buffer overrun later on.. */
+		if (getdatasize > sizeof(AuthVarUpdateData))
+			getdatasize = sizeof(AuthVarUpdateData);
 	}
 
 	for (i = 0; i < getdatasize; i++) {