Skip to content
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

Firmware file consistency verification #3

Closed
szszszsz opened this issue Aug 12, 2020 · 16 comments
Closed

Firmware file consistency verification #3

szszszsz opened this issue Aug 12, 2020 · 16 comments

Comments

@szszszsz
Copy link
Member

At the moment firmware file consistency check is not made before the flashing. This could result in hard to debug issues, or complete "bricking" of the hardware on the user side, shall the data transfer to the USB drive be interrupted.

This should be automated. The firmware binary and metadata (hash,signature) should be packed to a single .tar archive, which would be then checked by the Heads during the update execution.

For now we instruct users to do it by hand:

@alex-nitrokey
Copy link

I am not sure if this is best adresses in osresearch/heads instead.

I am not sure yet if this is necessary, though.

@szszszsz
Copy link
Member Author

Let's discuss it here first, and it can later be moved to upstream.

Why do you think its not needed? Is there any firmware file verification implemented already?

@alex-nitrokey
Copy link

Okay, at first I was wondering if flashrom itself would complain about a incomplete file, but even then it would not detect bitwise errors. I am not sure how severe this issue is, but in my opinion it is a valid point.

To keep it simple I would recommend a solution in which the checksum is only checked if a checksum file is present and ignored otherwise so that users could flash without the check if they want.

Furthermore, I think this is something that we might be able to provide ourselves and creating a PR upstream afterwards. Only finding some time would be vital 😬

@szszszsz
Copy link
Member Author

Exactly. PR possible, but time is scarce.

I would not agree with the hash sum check being optional - support side this will be a nightmare to troubleshoot, if any of that will happen. And 1 in 1000 will probably do.
Why not building own firmware format, like suggested? I do not see any disadvantages. tar is already available and supported in the Heads.

@alex-nitrokey
Copy link

I see, I missed the tar part. Sound like a valid idea, just wanted to avoid letting the user store multiple files. Actually, now that I think about it I pretty much like it 👍
We just need to look which (if any) compression/archive tools are supported in heads.

@szszszsz
Copy link
Member Author

Tar and zip are AFAIR :-)

@alex-nitrokey
Copy link

I added a simple file integrity check here

What do you think about including yours and mine PGP key in the built and adding signing checks as well? Otherwise, I think this can be closed by next release.

@szszszsz
Copy link
Member Author

szszszsz commented Sep 23, 2020

  1. Please rename extension to something unique and recognizable as Nitropad firmware package, like e.g. .npf (from NitroPadFirmware). See docx and xlsx as analogy. The reason is, that it will get mixed with regular user archives during file selection, and perhaps confuse user to unpack archive after downloading and before flashing.
  2. UI: potential improvement - on using ROM file there should be a confirmation dialog asking, whether it was checked with sha256sum tool by hand earlier on (could be safely done later).
  3. Do you mean public keys for import and verifying the package against? I think that's a good idea in general, I am worried though about possibly of easily done phishing attacks on the UI, if provided with the firmware pack file. How would you like to implement it? It would be better probably to have this integrated with the firmware image in the keychain.

@alex-nitrokey
Copy link

alex-nitrokey commented Sep 23, 2020

Please rename extension to something unique and recognizable as Nitropad firmware package, like e.g. .npf (from NitroPadFirmware).

Are you sure? Actually, I like the fact that it is a plain .zip archive 🤔

UI: potential improvement - on using ROM file there should be a confirmation dialog asking, whether it was checked with sha256sum tool by hand earlier on.

I will add a warning.

Do you mean public keys for import and verifying the package against? I think that's a good idea in general, I am worried though about possibly easily done phishing attacks on the UI, if provided with the firmware pack file. How would you like to implement it? It would be better probably to have this integrated with the firmware image in the keychain.

I mean having our key pairs already included in the trustdb of the gnupg folder in the rom an checking the signatures of a sha256sum.txt.gpg in the archive. On the other hand this would mean that we would be two people able to manipulate the machine. On the other hand this would make it much more difficult to flash roms of the bad guys... And some trust in Nitrokey needs to be taken for granted anyway.

@alex-nitrokey
Copy link

Please rename extension to something unique and recognizable as Nitropad firmware package, like e.g. .npf (from NitroPadFirmware). See docx and xlsx as analogy. The reason is, that it will get mixed with regular user archives during file selection, and perhaps confuse user to unpack archive after downloading and before flashing.

What about using .tar instead? Less likely confusion. But I am fine with arbitrary file endings as well 🤷

@szszszsz
Copy link
Member Author

szszszsz commented Sep 23, 2020

What about using .tar instead? Less likely confusion. But I am fine with arbitrary file endings as well

I do not see how that would help :-) Let's not be afraid of the custom extension. We have a format with a structure we expect in the software, hence we are fully entitled to have one. From the UX perspective I think it would be better.

@szszszsz
Copy link
Member Author

I mean having our key pairs already included in the trustdb of the gnupg folder in the rom an checking the signatures of a sha256sum.txt.gpg in the archive. On the other hand this would mean that we would be two people able to manipulate the machine. On the other hand this would make it much more difficult to flash roms of the bad guys... And some trust in Nitrokey needs to be taken for granted anyway.

It would be nice to have this but for informative purposes only, as long as the firmware is not planned to be locked. If time will come, the proper key will be established and handled with care. Now the decision to execute firmware update is on the user, and obligation to confirm the authenticy.

@alex-nitrokey
Copy link

alex-nitrokey commented Sep 23, 2020

It would be nice to have this but for informative purposes only, as long as the firmware is not planned to be locked. If time will come, the proper key will be established and handled with care. Now the decision to execute firmware update is on the user, and obligation to confirm the authenticy.

Of course, the user shall always have the possibility to shoot herself in the foot :) No, seriously, I am pretty much against any kind of restriction of flashing just some random file! There should be a fat warning though.

@alex-nitrokey
Copy link

change .zip to .npf and added warning for *.rom users.
https://github.com/Nitrokey/heads/commits/verified_firmware

@szszszsz
Copy link
Member Author

Looks good!

@alex-nitrokey
Copy link

Fixed in v1.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants