Message ID | 20211101220513.835940-4-erichte@linux.ibm.com |
---|---|
State | Superseded |
Headers | show |
Series | Secvar adjustments and fixes | expand |
Eric Richter <erichte@linux.ibm.com> writes: > Each signed variable update contains a timestamp -- this timestamp is checked > against the previous timestamp seen for that particular variable (if any), and > the update is rejected if the timestamp is not a later time than the previous. > > This timestamp check is intended to prevent re-use of signed update files. > Currently, the code stores the timestamps in the TS variable, which is then > stored in regular variable storage (typically PNOR). This patch promotes the > variable to "protected storage" (typically TPM NV), so avoid this variable > being accidentally cleared. > Or maliciously cleared... Anyway, good catch, I can't believe it never occurred to me to check this! > This change should only come into effect when either: > - initializing secvar for the first time (i.e. first boot, or > after a key-clear-request) > - processing any variable update > > Systems that already have a TS variable in PNOR will not be affected until > either of the above actions are taken. Hmm. So you could obviously be more agressive here - for example moving it unconditionally. I am trying to construct an attack scenario where it matters. What would you need? - something that would be blocked by TS. This would be a variable update. - an ability to reset PNOR TS. This is easy, just corrupt the PNOR. - A variable update you could perform based solely on the contents of TPM protected storage. That's not too hard, let's say you have a db update that's signed by PK. The obvious attack doesn't work: - PK-holder distributes a signed db update ("old update"). Machine owner applies this update. Attacker saves the update. - Later, the db key is compromised. * Attacker obtainst the compromised key and signs a malicious kernel with the compromised key. * PK-holder distributes a new signed db update ("new update") which overwrites the compromised key. * Machine owner applies the new update. - The machine owner applies this new firmware and reboots. - Attacker: * compromises the box * corrupts the SECBOOT partition of the PNOR * puts the old update in the PNOR. * installs their malicious kernel * reboots the machine This fails because skiboot will reset the update queue along with the rest of the PNOR when verification fails. So if an attacker were to corrupt the secboot partition, they'd instead render the OS unbootable: you'd get to petitboot and then not be able to verify anything because you have no db. An attacker would have to do the following more complex dance: - Attacker: * compromises the control plane such that they get console/petitboot access (so BMC/FSP serial or IPMI) * corrupts the SECBOOT partition of the PNOR * installs their malicious kernel * reboots the machine to petitboot * drops to the petitboot shell * installs the old update * reboots to malicious kernel (assuming I haven't missed anything!) Obviously the flip side of being more agressive is that you end up running a greater risk of Weird Bad Things Happening In The Field. Given the general... for lack of a better term "fiddlyness" of TPMs, that's a pretty live concern. idk what the right answer is, I guess it's a risk tolerance question. I guess there's possibly an argument to be made for doing what is fast becoming my favourite dumb trick for resolving things I don't like about skiboot, which is to say do the agressive and more correct thing upstream and another more conservative thing in stable :P > > Signed-off-by: Eric Richter <erichte@linux.ibm.com> > --- > libstb/secvar/backend/edk2-compat-process.c | 4 +++- > libstb/secvar/backend/edk2-compat.c | 1 + > 2 files changed, 4 insertions(+), 1 deletion(-) > > diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c > index 770c3706..d69e066f 100644 > --- a/libstb/secvar/backend/edk2-compat-process.c > +++ b/libstb/secvar/backend/edk2-compat-process.c > @@ -45,7 +45,9 @@ int update_variable_in_bank(struct secvar *update_var, const char *data, > else > var->flags |= SECVAR_FLAG_VOLATILE; > > - if (key_equals(update_var->key, "PK") || key_equals(update_var->key, "HWKH")) > + if (key_equals(update_var->key, "PK") > + || key_equals(update_var->key, "HWKH") > + || key_equals(update_var->key, "TS")) > var->flags |= SECVAR_FLAG_PROTECTED; > > return 0; > diff --git a/libstb/secvar/backend/edk2-compat.c b/libstb/secvar/backend/edk2-compat.c > index 9e61fbc6..d7975fa2 100644 > --- a/libstb/secvar/backend/edk2-compat.c > +++ b/libstb/secvar/backend/edk2-compat.c > @@ -89,6 +89,7 @@ static int edk2_compat_pre_process(struct list_head *variable_bank, > memcpy(tsvar->key, "TS", 3); > tsvar->key_len = 3; > tsvar->data_size = sizeof(struct efi_time) * 4; > + tsvar->flags = SECVAR_FLAG_PROTECTED; > memset(tsvar->data, 0, tsvar->data_size); > list_add_tail(variable_bank, &tsvar->link); > } I don't really understand quite enough of how the variable storage works to know if this is adequate. I can have another crack at it another day. Kind regards, Daniel
On 11/2/21 9:45 AM, Daniel Axtens wrote: > Eric Richter <erichte@linux.ibm.com> writes: > >> Each signed variable update contains a timestamp -- this timestamp is checked >> against the previous timestamp seen for that particular variable (if any), and >> the update is rejected if the timestamp is not a later time than the previous. >> >> This timestamp check is intended to prevent re-use of signed update files. >> Currently, the code stores the timestamps in the TS variable, which is then >> stored in regular variable storage (typically PNOR). This patch promotes the >> variable to "protected storage" (typically TPM NV), so avoid this variable >> being accidentally cleared. >> > > Or maliciously cleared... > > Anyway, good catch, I can't believe it never occurred to me to check this! > Thank Nick, he's the one that pointed it out to me when testing the first version :) >> This change should only come into effect when either: >> - initializing secvar for the first time (i.e. first boot, or >> after a key-clear-request) >> - processing any variable update >> >> Systems that already have a TS variable in PNOR will not be affected until >> either of the above actions are taken. > > Hmm. So you could obviously be more agressive here - for example moving > it unconditionally. I am trying to construct an attack scenario where it > matters. > > What would you need? > > - something that would be blocked by TS. This would be a variable > update. > > - an ability to reset PNOR TS. This is easy, just corrupt the PNOR. > > - A variable update you could perform based solely on the contents of > TPM protected storage. That's not too hard, let's say you have a db > update that's signed by PK. > > The obvious attack doesn't work: > > - PK-holder distributes a signed db update ("old update"). Machine > owner applies this update. Attacker saves the update. > > - Later, the db key is compromised. > > * Attacker obtainst the compromised key and signs a malicious kernel > with the compromised key. > > * PK-holder distributes a new signed db update ("new update") which > overwrites the compromised key. > > * Machine owner applies the new update. > > - The machine owner applies this new firmware and reboots. > > - Attacker: > * compromises the box > * corrupts the SECBOOT partition of the PNOR > * puts the old update in the PNOR. > * installs their malicious kernel > * reboots the machine > > This fails because skiboot will reset the update queue along with the > rest of the PNOR when verification fails. > > So if an attacker were to corrupt the secboot partition, they'd instead > render the OS unbootable: you'd get to petitboot and then not be able to > verify anything because you have no db. > > An attacker would have to do the following more complex dance: > > - Attacker: > > * compromises the control plane such that they get console/petitboot > access (so BMC/FSP serial or IPMI) > Outside the threat model, however still a reasonable concern. > * corrupts the SECBOOT partition of the PNOR > * installs their malicious kernel > * reboots the machine to petitboot > > * drops to the petitboot shell > * installs the old update > * reboots to malicious kernel > > (assuming I haven't missed anything!) > > Obviously the flip side of being more agressive is that you end up > running a greater risk of Weird Bad Things Happening In The Field. Given > the general... for lack of a better term "fiddlyness" of TPMs, that's a > pretty live concern. > > idk what the right answer is, I guess it's a risk tolerance question. > A more aggressive shoving of TS into TPM only saves us from that window of firmware update -> variable update, which granted, might be a long time unless the user is really active in updating their keys for some reason. The biggest Weird Bad Thing Happening In The Field™ I'm afraid of is having a duplicate TS in PNOR and TPM. That *shouldn't* happen, but there are no duplicate variable protections currently in secvar code. Migrating on a variable bank write ensures that the old PNOR TS gets wiped and written to TPM instead. Side note: an attacker can't slip a TS into PNOR without breaking the hash check, so exploiting this duplicate variable "weakness" shouldn't be possible. That said, it can be done with a forced write to the variable bank in .process() if !(TS->flags & PROTECTED), even if there is nothing in the queue. > I guess there's possibly an argument to be made for doing what is fast > becoming my favourite dumb trick for resolving things I don't like about > skiboot, which is to say do the agressive and more correct thing > upstream and another more conservative thing in stable :P > I'm open to either. > >> >> Signed-off-by: Eric Richter <erichte@linux.ibm.com> >> --- >> libstb/secvar/backend/edk2-compat-process.c | 4 +++- >> libstb/secvar/backend/edk2-compat.c | 1 + >> 2 files changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c >> index 770c3706..d69e066f 100644 >> --- a/libstb/secvar/backend/edk2-compat-process.c >> +++ b/libstb/secvar/backend/edk2-compat-process.c >> @@ -45,7 +45,9 @@ int update_variable_in_bank(struct secvar *update_var, const char *data, >> else >> var->flags |= SECVAR_FLAG_VOLATILE; >> >> - if (key_equals(update_var->key, "PK") || key_equals(update_var->key, "HWKH")) >> + if (key_equals(update_var->key, "PK") >> + || key_equals(update_var->key, "HWKH") >> + || key_equals(update_var->key, "TS")) >> var->flags |= SECVAR_FLAG_PROTECTED; This is the bit that forces the flag setting on a variable update. This function is be called on TS if any variable is updated, as the timestamp for that particular variable will need to be adjusted. >> >> return 0; >> diff --git a/libstb/secvar/backend/edk2-compat.c b/libstb/secvar/backend/edk2-compat.c >> index 9e61fbc6..d7975fa2 100644 >> --- a/libstb/secvar/backend/edk2-compat.c >> +++ b/libstb/secvar/backend/edk2-compat.c >> @@ -89,6 +89,7 @@ static int edk2_compat_pre_process(struct list_head *variable_bank, >> memcpy(tsvar->key, "TS", 3); >> tsvar->key_len = 3; >> tsvar->data_size = sizeof(struct efi_time) * 4; >> + tsvar->flags = SECVAR_FLAG_PROTECTED; >> memset(tsvar->data, 0, tsvar->data_size); >> list_add_tail(variable_bank, &tsvar->link); >> } > > I don't really understand quite enough of how the variable storage works > to know if this is adequate. I can have another crack at it another day. > This is the part that defines the TS variable for the first time, so now new TS variable definitions (i.e. after key-clear or on first boot) are stored in protected storage, i.e. TPM NV. I marked the other space where this flag is set above. secvar->flags are suggestions to the storage driver on how to handle certain variables. Given that we have exactly one(1) storage driver at the moment, this is directly instructing it to store it in TPM NV. Question preempt: It is a suggestion, as if we had a system with enough actually lockable storage, this flag would be ignored as all variables are "protected" by default. > Kind regards, > Daniel >
Hi, >>> Each signed variable update contains a timestamp -- this timestamp is checked >>> against the previous timestamp seen for that particular variable (if any), and >>> the update is rejected if the timestamp is not a later time than the previous. >>> >>> This timestamp check is intended to prevent re-use of signed update files. >>> Currently, the code stores the timestamps in the TS variable, which is then >>> stored in regular variable storage (typically PNOR). This patch promotes the >>> variable to "protected storage" (typically TPM NV), so avoid this variable >>> being accidentally cleared. >>> >> >> Or maliciously cleared... >> >> Anyway, good catch, I can't believe it never occurred to me to check this! >> > > Thank Nick, he's the one that pointed it out to me when testing the first > version :) Nick, finding all our embarassing bugs _again_. Good work Nick! >> An attacker would have to do the following more complex dance: >> >> - Attacker: >> >> * compromises the control plane such that they get console/petitboot >> access (so BMC/FSP serial or IPMI) >> > Outside the threat model, however still a reasonable concern. > >> * corrupts the SECBOOT partition of the PNOR >> * installs their malicious kernel >> * reboots the machine to petitboot >> >> * drops to the petitboot shell >> * installs the old update >> * reboots to malicious kernel >> >> (assuming I haven't missed anything!) >> > Obviously the flip side of being more agressive is that you end up >> running a greater risk of Weird Bad Things Happening In The Field. Given >> the general... for lack of a better term "fiddlyness" of TPMs, that's a >> pretty live concern. >> >> idk what the right answer is, I guess it's a risk tolerance question. >> > > A more aggressive shoving of TS into TPM only saves us from that window > of firmware update -> variable update, which granted, might be a long time > unless the user is really active in updating their keys for some reason. > > The biggest Weird Bad Thing Happening In The Field™ I'm afraid of is > having a duplicate TS in PNOR and TPM. That *shouldn't* happen, but there > are no duplicate variable protections currently in secvar code. Migrating > on a variable bank write ensures that the old PNOR TS gets wiped and > written to TPM instead. > > Side note: an attacker can't slip a TS into PNOR without breaking the > hash check, so exploiting this duplicate variable "weakness" shouldn't > be possible. > > That said, it can be done with a forced write to the variable bank in > .process() if !(TS->flags & PROTECTED), even if there is nothing in > the queue. > >> I guess there's possibly an argument to be made for doing what is fast >> becoming my favourite dumb trick for resolving things I don't like about >> skiboot, which is to say do the agressive and more correct thing >> upstream and another more conservative thing in stable :P >> > > I'm open to either. Ok, I see how my preferred ultimate resolution to this (move TS to TPM on any boot if it's in the PNOR) could get complex and probably needs some more preceeding patches before we put it in firmware. On the theory that "While there may be things that could be improved with this submission, I believe that it is, at this time, (1) a worthwhile modification to [skiboot], and (2) free of known issues which would argue against its inclusion:" Reviewed-by: Daniel Axtens <dja@axtens.net> Kind regards, Daniel [1]: https://www.kernel.org/doc/html/v5.15/process/submitting-patches.html#reviewer-s-statement-of-oversight
diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c index 770c3706..d69e066f 100644 --- a/libstb/secvar/backend/edk2-compat-process.c +++ b/libstb/secvar/backend/edk2-compat-process.c @@ -45,7 +45,9 @@ int update_variable_in_bank(struct secvar *update_var, const char *data, else var->flags |= SECVAR_FLAG_VOLATILE; - if (key_equals(update_var->key, "PK") || key_equals(update_var->key, "HWKH")) + if (key_equals(update_var->key, "PK") + || key_equals(update_var->key, "HWKH") + || key_equals(update_var->key, "TS")) var->flags |= SECVAR_FLAG_PROTECTED; return 0; diff --git a/libstb/secvar/backend/edk2-compat.c b/libstb/secvar/backend/edk2-compat.c index 9e61fbc6..d7975fa2 100644 --- a/libstb/secvar/backend/edk2-compat.c +++ b/libstb/secvar/backend/edk2-compat.c @@ -89,6 +89,7 @@ static int edk2_compat_pre_process(struct list_head *variable_bank, memcpy(tsvar->key, "TS", 3); tsvar->key_len = 3; tsvar->data_size = sizeof(struct efi_time) * 4; + tsvar->flags = SECVAR_FLAG_PROTECTED; memset(tsvar->data, 0, tsvar->data_size); list_add_tail(variable_bank, &tsvar->link); }
Each signed variable update contains a timestamp -- this timestamp is checked against the previous timestamp seen for that particular variable (if any), and the update is rejected if the timestamp is not a later time than the previous. This timestamp check is intended to prevent re-use of signed update files. Currently, the code stores the timestamps in the TS variable, which is then stored in regular variable storage (typically PNOR). This patch promotes the variable to "protected storage" (typically TPM NV), so avoid this variable being accidentally cleared. This change should only come into effect when either: - initializing secvar for the first time (i.e. first boot, or after a key-clear-request) - processing any variable update Systems that already have a TS variable in PNOR will not be affected until either of the above actions are taken. Signed-off-by: Eric Richter <erichte@linux.ibm.com> --- libstb/secvar/backend/edk2-compat-process.c | 4 +++- libstb/secvar/backend/edk2-compat.c | 1 + 2 files changed, 4 insertions(+), 1 deletion(-)