| Message ID | 1415961226-12899-1-git-send-email-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <qin.long@intel.com> 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 <lersek@redhat.com> > --- > 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
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);
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 <lersek@redhat.com> --- SecurityPkg/VariableAuthenticated/RuntimeDxe/Variable.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)