-
-
Notifications
You must be signed in to change notification settings - Fork 205
bin/flash-gui.sh & initrd/bin/flash.sh: Show SHA256SUM for manual verification prior of flashing #1514
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bin/flash-gui.sh & initrd/bin/flash.sh: Show SHA256SUM for manual verification prior of flashing #1514
Conversation
f112bc4 to
6f253cd
Compare
|
Cleaner code at 6f253cd: -i doesn't need any argument, just as -c and -r. And all other flash.sh caller verified to see if they needed to have -i passed (no manual integrity validation needed since rom backup/cbfs injected or npf file provided). From command line, one can test manually with (done personally)
Also, config manager, oem-factory-reset and other callers who backup and inject files in cbfs were verified (-i passed). @JonathonHall-Purism please review! |
|
(thanks for patience while I was away 😁 ) @tlaurion I support integrity checks on the ROMs in general, but the workflow changes here put me in a tough place to distribute future updates - there's no single file I can distribute that would work well on all recent releases, since NPF support was just merged recently. Adding another incompatible update format also complicates life a bit - we support external and manual flashing, so plain ROMs are still needed for that. I will still have to ship plain ROMs until the NPF has been out for long enough that we can break backward compatibility, so I don't think a long, scary, wordy prompt is appropriate. Purism's update script verifies the integrity of the decompressed ROM, so it does eliminate many (but admittedly not all) sources of corruption. FWIW I have not had any reports of corrupt flashes being a problem. Could we do this in a backward compatible way? Possibly, add a cbfs region to the ROM designed for the SHA-256 digest. It could be initially all zero, calculate the digest of that ROM, then replace it with the actual digest. Verification would zero it again to check the digest. This way the ROM could still be flashed as-is or by an older release unaware of the digest. Also, IMO, three confirmation prompts for flashing a plain ROM is excessive: Could we combine this into one prompt? Such as: |
|
I also noted a few problems in the current state of NPF support. (I don't think I had a chance to review it while it was in a PR)
I worked on create-npf.sh a bit to fix the git describe and add a way to generate a bad NPF: |
initrd/bin/flash.sh
Outdated
| CLEAN=0 | ||
| READ=0 | ||
| ROM="$1" | ||
| #Refactor argument parsing so arguments are not positional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick, the comments here shouldn't be worded with "refactor" since this refactoring has been done at this point
|
To be honest, I missed that prompt in my approval of nv41/ns50 merge which included npf support (nitrokey format). Basically, Heads now supports npf if provided, but most people will get to second prompt and third prompt with current state of PR since most do not use npf files but Nitrokey. I think we could easily modify codebase to merge second and third prompts above together, showing FLASH ROM? with sha256sum calculated. As for CBFS hash file of rom, Heads has this hidden feature as of now: reflashing the same externally provided rom as currently flashed will result in preservation of cbfs user files being added in ROM, resulting in temp rom matching what is currently flashed. As a result, going forward and flashing same git commit hash will result in no flashrom write. We could add a prompt when the firmware flashed is not resulting in a real write of the SPI flash where flashrom reports no change. This hidden feature could bring awareness to the users on the fact that upon reboot, TOTP/HOTP will be the same. Reflashing current ROM is actually a proper way of making sure that the firmware on SPI is the same as on external flashing media and is recommended to be reflashed in case of doubts. @JonathonHall-Purism thoughts? Based on that, I would not add a CBFS file, but I am ready to be challenged on that. The whole filename and git hash can be used to track the sha256sum of rom and therefore the original hash of the ROM that was flashed for verification and reflashing. TLDR regarding this PR further changes:
Will look in merging the two last prompts |
I still think we should combine all three prompts, not just the last two. There's no reason to ask the same question twice. The NPF change still doesn't offer a solution for me to actually switch to distributing NPF cleanly due to supporting backward compatibility and external flashing, so I'm going to have to continue distributing ROMs as-is for the foreseeable future.
I don't see how any of this relates to "verifying integrity of an update" to avoid a corrupted flash, which it seems the NPF change is trying to solve. (Which is a good thing to solve, but I would prefer to solve it in a way that is easier to support.) The "No flashrom write" scenario doesn't help avoid corrupted firmware images because you've already flashed it at that point. It also doesn't offer any assurance that your firmware wasn't tampered, because the tampered firmware could lie. If I mock up a strategy with a backward-compatible built-in digest, would you consider moving to that instead of NPF? |
6f253cd to
722121b
Compare
|
722121b replaces multiple prompts with the following (Also fixes whiptail fixed size window, text previously neglecting *.npf promotion) |
|
@daringer: I could fix npf-builder to build zip files, and have zip files support (where npf is a zip rebranding I'm not sure how to handle under Heads as of now). Also fixing things to remove /tmp/verified_rom. @JonathonHall-Purism @daringer ? This needs a little bit of prioritizing, current state of master with npf promotion without Heads building npf is just weird. |
initrd/bin/flash-gui.sh
Outdated
| if [ "$ROM_INTEGRITY_VERIFIED" == "1" ]; then | ||
| /bin/flash.sh -c -i "$ROM" | ||
| else | ||
| /bin/flash.sh -c "$ROM" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unreachable now and it is the only place flash.sh is invoked without -i. Can we please delete the integrity-verified variables, the -i parameter to flash.sh, and confirm_rom_sha256sum_hash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right. any other change is irrelevant as of current scope. Calling flash.sh manually implies one knows what he is doing.
…nual hash verification
722121b to
f90a9d4
Compare
This comment was marked as outdated.
This comment was marked as outdated.
f90a9d4 to
c0cf446
Compare
|
@daringer @JonathonHall-Purism: c0cf446 compared to master just changes the main rom prompt (to promote npf which was missing) and non-npf rom selection so show ROM hash (with automatic length whiptail prompt), and stating that verification cannot be automatically done on rom file type, removing all other changes in codebase. |
|
@tlaurion Thanks for working on this. Given the current state of master, I'm good with merging this. I just tested it on Mini v1 to confirm the behavior when flashing a plain ROM. Re: the bigger issues around NPF and the built-in alternative in #1518, I have asked our support team about migration strategies to start distributing a ZIP-based format which is not backward compatible, which will help determine where we stand on that. I'll follow up on #1518 and/or other PRs if we open them, I have the create_npf.sh fixes from above that I will put in a branch if we go that route. |





Add ROM and ROM hash when non-npf file is selected for flashing as per #1514 (comment)
Old
Notes: