Add downgrade protection #285
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mrnerdhair
force-pushed
the
downgrade-protection
branch
from
c006bcb to
ac8edb9
Compare
markrypto
suggested changes
Jul 13, 2021
mrnerdhair
force-pushed
the
downgrade-protection
branch
from
ac8edb9 to
ac04043
Compare
mrnerdhair
commented
Aug 2, 2021
mrnerdhair
commented
Aug 2, 2021
| @@ -558,7 +561,7 @@ TEST(Storage, StorageRoundTrip) { | |||
| 0x73, 0x74, 0x6f, 0x72, 0xab, 0xab, 0xab, 0xab, 0xab, 0xab, 0xab, 0xab, | |||
| 0xab, 0xab, 0xab, 0xab, 0xab, 0xab, 0xab, 0xab, 0xab, 0xab, 0xab, 0xab, | |||
| 0xab, 0xab, 0xab, 0xab, 0xab, 0xab, 0xab, 0xab, 0xab, 0xab, 0xab, 0xab, | |||
| 0xab, 0xab, 0xab, 0xab, 0xab, 0x00, 0x00, 0x00, 0x10, 0x00, 0x00, 0x00, | |||
| 0xab, 0xab, 0xab, 0xab, 0xab, 0x00, 0x00, 0x00, 0x11, 0x00, 0x00, 0x00, | |||
mrnerdhair
commented
Aug 2, 2021
| @@ -683,7 +686,7 @@ TEST(Storage, StorageRoundTrip) { | |||
| 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, | |||
| 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, | |||
| 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, | |||
| 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, 0x00, 0x00, 0x00, 0xe4, 0x8d, 0xfe, | |||
| 0x00, 0x00, 0x00, 0x00, 0x00, 0x11, 0x00, 0x00, 0x00, 0xe4, 0x8d, 0xfe, | |||
There was a problem hiding this comment.
encrypted_secrets_version
|
(just for posterity, in case it's not clear from the history above, @markrypto's feedback about storage format has been addressed.) |
markrypto
previously requested changes
Aug 4, 2021
mrnerdhair
force-pushed
the
downgrade-protection
branch
2 times, most recently
from
019d5c4 to
ae82249
Compare
mrnerdhair
force-pushed
the
downgrade-protection
branch
from
ae82249 to
354cf89
Compare
|
Are we OK to move forward with this now that the language is approved? |
mrnerdhair
requested review from
BitHighlander
and removed request for
markrypto
|
Closed, not selected. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds a form of downgrade protection which prevents as-yet-undiscovered vulnerabilities in older firmware versions from being used to compromise the keys of a user who has upgraded, but whose device is subsequently stolen by an attacker who downgrades it to a vulnerable version.
Storage format 17 is introduced, which is identical to format 16 except that the three bytes at offsets 1, 2, and 3 are used to store the major, minor, and patch version numbers of the firmware version which wrote the structure. Moving to a newer firmware version will cause these version numbers to be updated; moving to an older version will cause the storage to be reset and the user's private keys to be wiped -- the same thing that would happen if an unsigned firmware image were loaded.
A warning is displayed before resetting the storage, which provides a user the opportunity to unplug the device and update it to avoid the wipe. However, if the user chooses to downgrade to a version released before this warning message was added, their keys will be wiped without any warning beyond the bootloader's usual admonishment that your keys might be erased after an upgrade and that you should verify that your recovery sentence is available.
This approach increases security while maintaining a user's sovereignty over what firmware runs on their device.