Re: [edk2] [PATCH] SecurityPkg: VariableServiceSetVariable(): fix dbt <-> GUID association

Subject: Re: [edk2] [PATCH] SecurityPkg: VariableServiceSetVariable(): fix dbt <-> GUID association

From: Laszlo Ersek <lersek@redhat.com>

To: "Long, Qin" <qin.long@intel.com>, "edk2-devel@lists.sourceforge.net" <edk2-devel@lists.sourceforge.net>

Date: 2014-11-14 22:57:50

On 11/14/14 12:51, Long, Qin wrote:
>  Laszlo, thanks for catching this. This is one serious parentheses-matching issue. 
> 
> The patch is good. And I also did one quick validation. Please help to check-in this fix. 
> 
> Reviewed-by: Qin Long 

Thank you. Committed as SVN r16389.

Cheers
Laszlo

> 
> 
> Best Regards & Thanks,
> LONG, Qin
> 
> 
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com] 
> Sent: Friday, November 14, 2014 6:34 PM
> To: Long, Qin; edk2-devel@lists.sourceforge.net
> Subject: [PATCH] SecurityPkg: VariableServiceSetVariable(): fix dbt <-> GUID association
> 
> SVN r16380 ("UEFI 2.4 X509 Certificate Hash and RFC3161 Timestamp Verification support for Secure Boot") broke the "dbt" variable's association with its expected namespace GUID.
> 
> According to "MdePkg/Include/Guid/ImageAuthentication.h", *all* of the "db", "dbx", and "dbt" (== EFI_IMAGE_SECURITY_DATABASE2) variables have their special meanings in the EFI_IMAGE_SECURITY_DATABASE_GUID namespace.
> 
> However, the above commit introduced the following expression in
> VariableServiceSetVariable():
> 
> -  } else if (CompareGuid (VendorGuid, &gEfiImageSecurityDatabaseGuid) &&
> -          ((StrCmp (VariableName, EFI_IMAGE_SECURITY_DATABASE) == 0) || (StrCmp (VariableName, EFI_IMAGE_SECURITY_DATABASE1) == 0))) {
> +  } else if (CompareGuid (VendorGuid, &gEfiImageSecurityDatabaseGuid) &&
> +          ((StrCmp (VariableName, EFI_IMAGE_SECURITY_DATABASE) == 0) || (StrCmp (VariableName, EFI_IMAGE_SECURITY_DATABASE1) == 0))
> +           || (StrCmp (VariableName, EFI_IMAGE_SECURITY_DATABASE2)) == 
> + 0) {
> 
> Simply replacing the individual expressions with the predicates "GuidMatch", "DbMatch", "DbxMatch", and "DbtMatch", the above transformation becomes:
> 
> -  } else if (GuidMatch &&
> -          ((DbMatch) || (DbxMatch))) {
> +  } else if (GuidMatch &&
> +          ((DbMatch) || (DbxMatch))
> +           || DbtMatch) {
> 
> In shorter form, we change
> 
>   GuidMatch && (DbMatch || DbxMatch)
> 
> into
> 
>   GuidMatch && (DbMatch || DbxMatch) || DbtMatch
> 
> which is incorrect, because this way "dbt" will match outside of the intended namespace / GUID.
> 
> The error was caught by gcc:
> 
>> SecurityPkg/VariableAuthenticated/RuntimeDxe/Variable.c: In function
>> 'VariableServiceSetVariable':
>>
>> SecurityPkg/VariableAuthenticated/RuntimeDxe/Variable.c:3188:71: error:
>> suggest parentheses around '&&' within '||' [-Werror=parentheses]
>>
>>    } else if (CompareGuid (VendorGuid, &gEfiImageSecurityDatabaseGuid) &&
>>                                                                        
>> ^
>> cc1: all warnings being treated as errors
> 
> Fix the parentheses.
> 
> This change may have security implications.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek 
> ---
>  SecurityPkg/VariableAuthenticated/RuntimeDxe/Variable.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/SecurityPkg/VariableAuthenticated/RuntimeDxe/Variable.c b/SecurityPkg/VariableAuthenticated/RuntimeDxe/Variable.c
> index 432531f..ac043d9 100644
> --- a/SecurityPkg/VariableAuthenticated/RuntimeDxe/Variable.c
> +++ b/SecurityPkg/VariableAuthenticated/RuntimeDxe/Variable.c
> @@ -3186,8 +3186,11 @@ VariableServiceSetVariable (
>    } else if (CompareGuid (VendorGuid, &gEfiGlobalVariableGuid) && (StrCmp (VariableName, EFI_KEY_EXCHANGE_KEY_NAME) == 0)) {
>      Status = ProcessVarWithPk (VariableName, VendorGuid, Data, DataSize, &Variable, Attributes, FALSE);
>    } else if (CompareGuid (VendorGuid, &gEfiImageSecurityDatabaseGuid) &&
> -          ((StrCmp (VariableName, EFI_IMAGE_SECURITY_DATABASE) == 0) || (StrCmp (VariableName, EFI_IMAGE_SECURITY_DATABASE1) == 0))
> -           || (StrCmp (VariableName, EFI_IMAGE_SECURITY_DATABASE2)) == 0) {
> +             ((StrCmp (VariableName, EFI_IMAGE_SECURITY_DATABASE)  == 0) ||
> +              (StrCmp (VariableName, EFI_IMAGE_SECURITY_DATABASE1) == 0) ||
> +              (StrCmp (VariableName, EFI_IMAGE_SECURITY_DATABASE2) == 0)
> +             )
> +            ) {
>      Status = ProcessVarWithPk (VariableName, VendorGuid, Data, DataSize, &Variable, Attributes, FALSE);
>      if (EFI_ERROR (Status)) {
>        Status = ProcessVarWithKek (VariableName, VendorGuid, Data, DataSize, &Variable, Attributes);
> --
> 1.8.3.1
> 


------------------------------------------------------------------------------
Comprehensive Server Monitoring with Site24x7.
Monitor 10 servers for $9/Month.
Get alerted through email, SMS, voice calls or mobile push notifications.
Take corrective actions from your mobile device.
http://pubads.g.doubleclick.net/gampad/clk?id=154624111&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel