Add generic ECDSA TLV Signature type & P384 to imgtool #1617
Conversation
| """Return the actual signature""" | ||
| return self.key.sign( | ||
| data=payload, | ||
| signature_algorithm=ec.ECDSA(SHA384())) |
There was a problem hiding this comment.
Hmmm, this should be using SHA256(), no?
There was a problem hiding this comment.
This one is a big question. I would be interested, what others think about it? @d3zd3z
AFAICT it is a common/recommended (somewhere required) practice to use a specific ECDSA curve with a hash algorithm of similar level (bits) of security. If P384_SHA256 is acceptable for us, that would be the best interms of code size.
There was a problem hiding this comment.
Changing the digest will affect other tools that expect an image to use SHA-256, so it should be out of scope of this PR. If you think it needs discussion, please open an issue. SHA-256 is not broken as of today, so until someone finds a pre-image collision, or any related attack we can just stay as is.
There was a problem hiding this comment.
It's not replacing the SHA-256 image digest, that part is unmodified (see line 283-286). When the image digest is signed, it goes through SHA-384 as well (double hashing). As I read double hashing also happened in the past of MCUboot, when the ECDSA API of the used crypto backend wanted to do the hashing itself (could not be skipped).
If P384_SHA256 is acceptable, the double hashing can be easily avoided. Using SHA-384 for calculating the image digest and replacing SHA-256 seems to have too much impact. The third solution is current one.. double hashing.
There was a problem hiding this comment.
SHA-256 is not broken as of today, so until someone finds a pre-image collision, or any related attack we can just stay as is.
But, neither has P-256 been broken. But, increasing the size of the curve without increasing the size of the hash is kind of pointless. There are two general attacks that increasing sizes helps with. One is an increase in raw computation performance, and especially something like quantum computing. The other are vulnerabilities found in the underlying operations that reduce the amount of work needed to attack the operation.
There was a problem hiding this comment.
I've updated the imgtool to use SHA384 instead of SHA256 everywhere if an ECDSAP384 key is being used. There is a separate TLV for SHA384 and SHA256.
There was a problem hiding this comment.
I guess your argument is "given a random 256-bit string, signing it with with P-384 wouldn't be better than using P-256", which makes sense, but then this PR still is P-384(SHA-384(SHA-256(image))). Doesn't that make the SHA-384 just a glorified bit expander?
Agreed, computing a SHA-384 over a SHA-256 is fairly pointless. You might as well, just use the SHA-256 result with some padding. You aren't gaining any security by adding the extra hash.
I'd have to think about it, but the reduction in work a quantum computer can make over an EC curve will be based on the size of the input, so with a 256-bit input, a quantum computer built to break P-384 would only need the work of P-256, given the smaller input. It isn't likely to be that straightforward, though.
Any other attack against the EC is going to depend on the input size, so I don't understand the point in using P-384 unless we are taking a SHA-384 over the entire image.
BTW, there is no reason that the "image hash" tag needs to be related to how the signature is done. It really is just a shortcut to be able to reject an invalid image without having to verify the curve signature. We're kind of unusual in even storing it, as it is easily computed, and not generally included in signatures. If we want to use P-384, why don't we just compute the SHA384 over the whole image, like we're supposed to?
There was a problem hiding this comment.
I would also be up for the idea of just removing the hash entirely from the TLV. There isn't really a very good reason for it to be there in the first place. The only thing it really gives us is the ability to reject an invalid image before we have to compute the curve signature. This, for example, could speed up a boot if there is an invalid upgrade image by avoiding having to verify the signature of the upgrade before the primary image could be booted.
There was a problem hiding this comment.
We should discuss it and eventually, it could be removed. I've created a GitHub issue to track the conversation:
#1675
Focusing on the main goal of this PR, I think the ECDSA TLV related modifications are fine.
I admit the P384 part for imgtool doesn't quite belong here and it can be moved out into a separate PR
if it's requested, but I think after this discussion we could leave it here - in its current form (SHA-384 image hash
when P384 is selected).
@utzig , AFAIK the validation code and overall support for P384 is coming...
Roolli
force-pushed
the
gen-ecdsa
branch
4 times, most recently
from
5dda701
to
011b442
Compare
Create a new generic ECDSA TLV type that can be used to store any signatures irrespective of the curve type. Signed-off-by: Roland Mikhel <roland.mikhel@arm.com> Change-Id: I2aeb885251fd25e23f5430328b8cc64b8cc8d7be
Remove those TLVs that are tied to a specific curve and update the image validation logic to look for the new generic TLV Signed-off-by: Roland Mikhel <roland.mikhel@arm.com> Change-Id: I924f2742424bc255fbed1b0941648baa88f60147
Update imgtool to support the new generic ECDSA TLV and the ECDSA p384 curve type with sha-384 Signed-off-by: Roland Mikhel <roland.mikhel@arm.com> Change-Id: I9b1887610cc5d0e7cde90f47999fcdf3500ef51c
Add backwards compatibility to the imgtool to support the old curve specific TLVs. Currently only ECDSA256 needs this. Signed-off-by: Roland Mikhel <roland.mikhel@arm.com> Change-Id: I275894ebc713ea8adcaab4198b036c41233b11e8
Add support to the simulator so that the generic ECDSA TLV can be tested. Signed-off-by: Roland Mikhel <roland.mikhel@arm.com> Change-Id: I3322ed829d150ff35abfaaa8ecf69ab7017bd7cf
Remove those TLVs that are tied to a specific curve and modify the code to use the new generic ECDSA TLV. Signed-off-by: Roland Mikhel <roland.mikhel@arm.com> Change-Id: Iffe9052580c99e75118cf5df4286e0e9a2af4a8c
Signed-off-by: Roland Mikhel <roland.mikhel@arm.com> Change-Id: I6837467e985af644f124ae8a9cceb0f68736ec84
|
Considering that the main purpose (TLVs) of this PR has been reviewed and Roland updated the PR #1666 adds the P384 signature verification support to the bootloader, using the new PSA Crypto API backend. |
This pull request removes the current curve specific signature TLV types (ECDSAP224, ECDSAP256) and introduces a new generic one (ECDSASIG) that is not tied a curve, and adds P384 curve support to the imgtool.
Even in the current implementation the way the signature parsing happens is not relevant from a TLV standpoint as the curve to use is encoded in the signature itself (as per RFC 5820) and all the current crypto backends parse the signature the same way. The curve independent TLV would be beneficial later if a new crypto backend or new curve type would be introduced, such as P384.
Relevant issue: #1596