| Message ID | 20211101220513.835940-7-erichte@linux.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Secvar adjustments and fixes | expand |
Hi Eric, > When the system is in setup mode, that is, no PK has been enrolled in the PK > variable, variable signature enforcement is disabled. Therefore, variable > update signatures do not need to be authorized by a variable already stored in > a variable. > > However, variable updates must still be in the AUTH format, which still > contains a pkcs7 signature. Bare ESL files are *not* accepted as an update to > a variable. So, a variable update must still be signed, albeit with any key. > > Not enforcing signature checks is intended to be a convenience feature, however > enrolling a variable into the PK carries extra weight. Once data is enrolled > into the PK variable, signatures are now required for all further variable > updates. > > Due to this extra importance when enrolling a PK, the PK should most definitely > contain valid data. Therefore, as a sanity check, when enrolling a PK for the > first time (i.e. in setup mode), the PK variable update must be signed by its > own private key, which ensures ownership of the private key and validity of the > public key to be stored in the PK variable. Hmm. In my work for [REDACTED], I had explictly _not_ required this. I could do this, I'll just need to get things renamed from "Allow unauthenticated PK updates" to "Allow self-signed PK updates". Although, as with your case, I had also done all my tests (such as they are at the moment) with a self-signed key. Does it take us closer to EDK2 compat or further away? I am guessing EDK2 also requires that there is an AUTH structure but just throws it away after maybe doing some basic syntactical sanity checks? I'm not _really_ sold on the value of this but I'm open to being convinced. Beyond that it looks OK, I'll have a closer look once I get through the refactorings. Kind regards, Daniel
On 11/2/21 8:16 AM, Daniel Axtens wrote: > Hi Eric, > >> When the system is in setup mode, that is, no PK has been enrolled in the PK >> variable, variable signature enforcement is disabled. Therefore, variable >> update signatures do not need to be authorized by a variable already stored in >> a variable. >> >> However, variable updates must still be in the AUTH format, which still >> contains a pkcs7 signature. Bare ESL files are *not* accepted as an update to >> a variable. So, a variable update must still be signed, albeit with any key. >> >> Not enforcing signature checks is intended to be a convenience feature, however >> enrolling a variable into the PK carries extra weight. Once data is enrolled >> into the PK variable, signatures are now required for all further variable >> updates. >> >> Due to this extra importance when enrolling a PK, the PK should most definitely >> contain valid data. Therefore, as a sanity check, when enrolling a PK for the >> first time (i.e. in setup mode), the PK variable update must be signed by its >> own private key, which ensures ownership of the private key and validity of the >> public key to be stored in the PK variable. > > Hmm. In my work for [REDACTED], I had explictly _not_ required this. I > could do this, I'll just need to get things renamed from "Allow > unauthenticated PK updates" to "Allow self-signed PK updates". > > Although, as with your case, I had also done all my tests (such as they > are at the moment) with a self-signed key. > I'm okay with unifying this with [REDACTED] if it means we don't pull another project-that-must-not-be-named and have a thousand different quirks and different behaviors across implementations. > Does it take us closer to EDK2 compat or further away? I am guessing > EDK2 also requires that there is an AUTH structure but just throws it > away after maybe doing some basic syntactical sanity checks? > Slightly further, though again, so far every example I'm finding does the same procedure. Apparently it's a bit of a mess, where certain firmware may allow bare ESLs or even certs. As a side soapbox moment: I'm not a fan of unauthenticated updates period. I don't see why we shouldn't be signature checking all updates, the convenience isn't really that much more when the updates still have to be signed in the AUTH format. Either that or we support bare ESLs when in setup mode, but that'd likely end up being more work to introduce more fun error cases when users enroll a PK first. > I'm not _really_ sold on the value of this but I'm open to being > convinced. > Neither am I tbh, but this is bringing it in line with our documentation and examples. We don't have a ton of documentation to update if we skip this patch, but I tend to favor the documentation being what the code *should* be. > Beyond that it looks OK, I'll have a closer look once I get through the > refactorings. > > Kind regards, > Daniel >
> I'm okay with unifying this with [REDACTED] if it means we don't > pull another project-that-must-not-be-named and have a thousand > different quirks and different behaviors across implementations. > >> Does it take us closer to EDK2 compat or further away? I am guessing >> EDK2 also requires that there is an AUTH structure but just throws it >> away after maybe doing some basic syntactical sanity checks? >> > > Slightly further, though again, so far every example I'm finding does > the same procedure. Apparently it's a bit of a mess, where certain > firmware may allow bare ESLs or even certs. > > As a side soapbox moment: I'm not a fan of unauthenticated updates > period. I don't see why we shouldn't be signature checking all > updates, the convenience isn't really that much more when the updates > still have to be signed in the AUTH format. Either that or we support > bare ESLs when in setup mode, but that'd likely end up being more > work to introduce more fun error cases when users enroll a PK first. > >> I'm not _really_ sold on the value of this but I'm open to being >> convinced. >> > > Neither am I tbh, but this is bringing it in line with our > documentation and examples. We don't have a ton of documentation > to update if we skip this patch, but I tend to favor the > documentation being what the code *should* be. Yeah it's all a bit lineball to me too. Is there a doc link I can look at? (internal is fine) I pretty carefully chose the descriptor 'unauthenticated' as opposed to 'unsigned' when specifying what [redacted] would accept specifically because an auth structure with a signature is still required. Having said that, it's not too late to swap it to 'self-signed', and that might actually be even less ambiguous. I'll ask about that on [internal communications platform]. >> Beyond that it looks OK, I'll have a closer look once I get through the >> refactorings. It looks fine, I'd probably just nitpick the wording: "PK not able to update itself, rejecting enrollment." I think 'not able to verify itself' or even "Verification of self-signature on PK failed" might be clearer? Kind regards, Daniel
diff --git a/libstb/secvar/backend/edk2-compat-process.c b/libstb/secvar/backend/edk2-compat-process.c index 4f4b7e71..79eecf84 100644 --- a/libstb/secvar/backend/edk2-compat-process.c +++ b/libstb/secvar/backend/edk2-compat-process.c @@ -752,7 +752,12 @@ int process_update(const struct secvar *update, char **newesl, goto out; } - if (setup_mode) { + /* If we're in setup_mode, signature checks are not enforced, + * so we can stop here. + * Exception: enrolling a PK update in setup mode must verify the update + * signature by the proposed PK public key. + * i.e. a new PK update must be signed by itself. */ + if (setup_mode && !key_equals(update->key, "PK")) { rc = OPAL_SUCCESS; goto out; } @@ -765,6 +770,17 @@ int process_update(const struct secvar *update, char **newesl, goto out; } + /* Special case for checking self-signed PK signature in setup mode */ + if (setup_mode && key_equals(update->key, "PK")) { + rc = verify_signature(auth, hash, 32, *newesl, *new_data_size); + if (rc == OPAL_SUCCESS) + prlog(PR_INFO, "PK successfully verified by itself\n"); + else + prlog(PR_ERR, "PK not able to update itself, rejecting enrollment.\n"); + + goto out; + } + /* Get the authority to verify the signature */ get_key_authority(key_authority, update, bank);
When the system is in setup mode, that is, no PK has been enrolled in the PK variable, variable signature enforcement is disabled. Therefore, variable update signatures do not need to be authorized by a variable already stored in a variable. However, variable updates must still be in the AUTH format, which still contains a pkcs7 signature. Bare ESL files are *not* accepted as an update to a variable. So, a variable update must still be signed, albeit with any key. Not enforcing signature checks is intended to be a convenience feature, however enrolling a variable into the PK carries extra weight. Once data is enrolled into the PK variable, signatures are now required for all further variable updates. Due to this extra importance when enrolling a PK, the PK should most definitely contain valid data. Therefore, as a sanity check, when enrolling a PK for the first time (i.e. in setup mode), the PK variable update must be signed by its own private key, which ensures ownership of the private key and validity of the public key to be stored in the PK variable. As it currently stands, all documentation and examples published thus far already sign the PK variable updates by itself, this patch now makes it a requirement. NOTE: This does not affect PK replacement. If a PK is already enrolled, then the PK replacement must be signed with the old PK, not with itself. Signed-off-by: Eric Richter <erichte@linux.ibm.com> --- libstb/secvar/backend/edk2-compat-process.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)