Fix ECDSA failing when the hash is all-bits-zero

[TRodziewicz]

Description

Fix function mbedtls_ecp_mul_shortcuts() to skip multiplication when m
is 0 and simply assignt 0 to R. Additionally fix ecjpake_zkp_read() to
return MBEDTLS_ERR_BAD_INPUT_DATA when r length is 0 and ecjpake
test to expect that.

Fix #1792

Status

READY

Requires Backporting

Yes

Migrations

NO

Todos

• Tests
• Documentation
• Backported

Steps to test or reproduce

Please check: #1792

[stevew817]

If the ZKP is indeed strictly non-zero, then this change would be correct.

However, I read through RFC's 8235 and 8236, and couldn't find anywhere that a ZKP is guaranteed to be non-zero. Sure, it's a corner case, but consider this (according to the ecjpake draft, paragraph 7.4.2):

• v is a random integer [1,n-1] (i.e. ephemeral private key)
• V is the public point of private key v
• h is a SHA-256 hash mod n, which has a theoretical range of [0,2^256-1] mod n, ergo [0,n-1]
• x is a private key, meaning it has a range of [1,n-1] just like v

The ZKP signature is calculated as r = v - x*h mod n. Given the range of v and h, it's theoretically possible that they collide in such a way that makes r evaluate to zero, meaning that a signature of all-bits zero could theoretically be correct.

[mpg]

@stevew817 Excellent point, thanks for noticing! It was my mistake, when I saw this test failing I assumed r == 0 was forbidden (probably reasoning by analogy with ECDSA which does forbid it), but actually I should have read the description of the test better: what's forbidden is for the length of the encoded version of r to be zero. The Thread spec, as cited in the comments defines the encoding as opaque r<1..2^8-1>; so even if r was 0, it would be illegal to encode it as a zero-length string.

[mpg]

As Steven pointed out, the fix I initially suggested for the EC J-PAKE part was misguided, so it should be changed.

Also, the ChangeLog entry needs to be updated.

Finally, I left a note about the ideal commit structures, but that's just things to keep in mind for future PRs. For this one you can just add new commits to fix things, no worries.

[TRodziewicz]

Thanks Manuel, works like a charm.

[mpg]

This looks mostly good to me, except for two things:

1. We should have a regression test for 1792. As Ronald mentioned in CMake: usage of CMAKE_SOURCE_DIR in CMakeLists.txt files. #4268, an easy way to do that would be to modify the existing test functions ecdsa_prim_random and ecdsa_write_read_random: currently they generate a verify a signature with a random hash. We could turn the body into a loop that runs twice: once with a random hash, once with an all-zero hash.
2. In the ChangeLog entry I think we should say "hash" instead of "payload".

[mpg]

Thanks for adding a non-regresssion test and fixing the ChangeLog entry as requested. Looks all good to me now!

[gilles-peskine-arm]

The code changes and test data changes are correct, but the way the regression test is implemented is confusing.

[gilles-peskine-arm]

I think this should be backported to 2.16 since it's a bug, but given how much of a corner case it is, I'd be ok with not doing the backport.

[stevew817]

It was my mistake, when I saw this test failing I assumed r == 0 was forbidden (probably reasoning by analogy with ECDSA which does forbid it), but actually I should have read the description of the test better: what's forbidden is for the length of the encoded version of r to be zero.

@mpg Wouldn't it make sense to add an explicit test for ECJPAKE with r == 0, since it's apparently easy to run afoul of this corner case?

[mpg]

Looks good to me!

[mpg]

Wouldn't it make sense to add an explicit test for ECJPAKE with r == 0, since it's apparently easy to run afoul of this corner case?

I think that would be a nice addition if it can be done* but I don't think it's important enough to block this PR. This case if very unlikely to happen at random, and if an attacker forced it, the worst that could happen is that we reject the message for the wrong reason.

*I didn't check for NIZP, but for ECDSA r == 0 is forbidden (the signature generation function should re-try with a fresh ephemeral key when it happens) but that's one part of your signature generation routine that you just can't test without solving a discrete log on the curve, which in practice means you need to have a weak curve available in your library if you want to test this... One of the many things that are annoying with ECDSA. Again I didn't check how it is with NIZP, so perhaps there it's possible to generate a valid ZKP with r == 0 for testing. I you feel like checking, I'm curious to know what you find, but unfortunately I don't have time to investigate myself now.