| Message ID | 16661.1480075392@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 25 November 2016 at 12:03, David Howells <dhowells@redhat.com> wrote: > How about the attached additional patch? Should I be checking the UEFI > version number if such is available? > Yes. In pre-2.6, DeployedMode is not a reserved name, and so it may be possible for someone to slip in a DeployedMode=0 on a secure boot enabled system to trick the kernel into thinking lockdown should be disabled. > David > --- > commit 981110f45ba73798875af7639d0328dc2d6f9919 > Author: David Howells <dhowells@redhat.com> > Date: Fri Nov 25 11:52:05 2016 +0000 > > efi: Handle secure boot from UEFI-2.6 > > UEFI-2.6 adds a new variable, DeployedMode. If it exists, this must be 1 > to engage lockdown mode. > > Reported-by: James Bottomley <James.Bottomley@HansenPartnership.com> > Signed-off-by: David Howells <dhowells@redhat.com> > > diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c > index ca643eba5a4b..4c3bddef4fb3 100644 > --- a/drivers/firmware/efi/libstub/secureboot.c > +++ b/drivers/firmware/efi/libstub/secureboot.c > @@ -22,6 +22,9 @@ static const efi_char16_t const efi_SecureBoot_name[] = { > static const efi_char16_t const efi_SetupMode_name[] = { > 'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', 0 > }; > +static const efi_char16_t const efi_DeployedMode_name[] = { > + 'D', 'e', 'p', 'l', 'o', 'y', 'e', 'd', 'M', 'o', 'd', 'e', 0 > +}; > > /* SHIM variables */ > static const efi_guid_t shim_guid = EFI_SHIM_LOCK_GUID; > @@ -62,6 +65,16 @@ int efi_get_secureboot(efi_system_table_t *sys_table_arg) > if (val == 1) > return 0; > > + status = get_efi_var(efi_DeployedMode_name, &efi_variable_guid, > + NULL, &size, &val); > + if (status != EFI_NOT_FOUND) { > + if (status != EFI_SUCCESS) > + goto out_efi_err; > + > + if (val == 1) > + return 0; I think the logic is the wrong way around here. Secure Boot is enabled if SecureBoot=1 and SetupMode=0, unless DeployedMode=0. So you should return 0 here if val == 0, but only when running on 2.6 or later.
Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > Yes. In pre-2.6, DeployedMode is not a reserved name, and so it may be > possible for someone to slip in a DeployedMode=0 on a secure boot > enabled system to trick the kernel into thinking lockdown should be > disabled. How does one get the version number? Unfortunately, searching the document for 'version' doesn't help as every page has that in the footer:-/ > > + if (val == 1) > > + return 0; > > I think the logic is the wrong way around here. Secure Boot is enabled > if SecureBoot=1 and SetupMode=0, unless DeployedMode=0. So you should > return 0 here if val == 0, but only when running on 2.6 or later. Good point. David -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 25 November 2016 at 12:35, David Howells <dhowells@redhat.com> wrote: > Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > >> Yes. In pre-2.6, DeployedMode is not a reserved name, and so it may be >> possible for someone to slip in a DeployedMode=0 on a secure boot >> enabled system to trick the kernel into thinking lockdown should be >> disabled. > > How does one get the version number? Unfortunately, searching the document > for 'version' doesn't help as every page has that in the footer:-/ > There is a 'revision' field in the header ('hdr') of the EFI system table, so something like (sys_table_arg->hdr.revision >> 16) > 2 || ((sys_table_arg->hdr.revision >> 16) == 2 && (sys_table_arg->hdr.revision & 0xffff) >= 6) should do the trick I think >> > + if (val == 1) >> > + return 0; >> >> I think the logic is the wrong way around here. Secure Boot is enabled >> if SecureBoot=1 and SetupMode=0, unless DeployedMode=0. So you should >> return 0 here if val == 0, but only when running on 2.6 or later. > > Good point. > > David -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > There is a 'revision' field in the header ('hdr') of the EFI system > table, so something like Is this the same as the fw_revision in the system table? #define EFI_2_60_SYSTEM_TABLE_REVISION ((2<<16) | (60)) #define EFI_2_50_SYSTEM_TABLE_REVISION ((2<<16) | (50)) #define EFI_2_40_SYSTEM_TABLE_REVISION ((2<<16) | (40)) ... David -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 25 November 2016 at 12:51, David Howells <dhowells@redhat.com> wrote: > Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > >> There is a 'revision' field in the header ('hdr') of the EFI system >> table, so something like > > Is this the same as the fw_revision in the system table? > > #define EFI_2_60_SYSTEM_TABLE_REVISION ((2<<16) | (60)) > #define EFI_2_50_SYSTEM_TABLE_REVISION ((2<<16) | (50)) > #define EFI_2_40_SYSTEM_TABLE_REVISION ((2<<16) | (40)) > ... > Yes. And in fact, that means my example is incorrect (60 not 6 in the minor field) -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c index ca643eba5a4b..4c3bddef4fb3 100644 --- a/drivers/firmware/efi/libstub/secureboot.c +++ b/drivers/firmware/efi/libstub/secureboot.c @@ -22,6 +22,9 @@ static const efi_char16_t const efi_SecureBoot_name[] = { static const efi_char16_t const efi_SetupMode_name[] = { 'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', 0 }; +static const efi_char16_t const efi_DeployedMode_name[] = { + 'D', 'e', 'p', 'l', 'o', 'y', 'e', 'd', 'M', 'o', 'd', 'e', 0 +}; /* SHIM variables */ static const efi_guid_t shim_guid = EFI_SHIM_LOCK_GUID; @@ -62,6 +65,16 @@ int efi_get_secureboot(efi_system_table_t *sys_table_arg) if (val == 1) return 0; + status = get_efi_var(efi_DeployedMode_name, &efi_variable_guid, + NULL, &size, &val); + if (status != EFI_NOT_FOUND) { + if (status != EFI_SUCCESS) + goto out_efi_err; + + if (val == 1) + return 0; + } + /* See if a user has put shim into insecure mode. If so, and if the * variable doesn't have the runtime attribute set, we might as well * honor that.
How about the attached additional patch? Should I be checking the UEFI version number if such is available? David --- commit 981110f45ba73798875af7639d0328dc2d6f9919 Author: David Howells <dhowells@redhat.com> Date: Fri Nov 25 11:52:05 2016 +0000 efi: Handle secure boot from UEFI-2.6 UEFI-2.6 adds a new variable, DeployedMode. If it exists, this must be 1 to engage lockdown mode. Reported-by: James Bottomley <James.Bottomley@HansenPartnership.com> Signed-off-by: David Howells <dhowells@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html