Doc: Add note on special use of A in ecp group structure #6999

ivq · 2023-01-31T15:38:48Z

Description

ECP implementation sets A to NULL in structure mbedtls_ecp_group for optimizing Short Weierstrass curves with A = -3.
We should warn on this in the document, as using domain parameters in external program is likely to encounter errors if not
noticing this.

Fixes #8045

Gatekeeper checklist

changelog provided
backport Backport 2.28: Doc: Add note on special use of A in ecp group structure #8046
tests not required - only documentation and a one-line static inline function (that's used in the code so we know it's OK)

gilles-peskine-arm

Looks good in principle, but the wording isn't quite right.

gilles-peskine-arm · 2023-05-18T17:17:40Z

include/mbedtls/ecp.h

@@ -196,6 +196,9 @@ mbedtls_ecp_point;
 * cardinality is denoted by \p N. Our code requires that \p N is an
 * odd prime as mbedtls_ecp_mul() requires an odd number, and
 * mbedtls_ecdsa_sign() requires that it is prime for blinding purposes.
+ * The default implementation sets \p A to NULL rather than the authentic value


A isn't a pointer. This should read something like “sets A to 0 (with A.p == NULL) rather than …”

We should be careful about the wording here: setting A to 0 with A.p != NULL actually means zero. I think using parentheses here is dangerous, as this could be interpreted as optional while it's actually essential.

gilles-peskine-arm · 2023-05-18T17:19:18Z

include/mbedtls/ecp.h

-    mbedtls_mpi A;              /*!< For Short Weierstrass: \p A in the equation. For
-                                     Montgomery curves: <code>(A + 2) / 4</code>. */
+    mbedtls_mpi A;              /*!< For Short Weierstrass: \p A in the equation. Note that
+                                     \p A is set to NULL in some cases.


As above: “the value 0 (with A.p == NULL) may be used to represent -3.”

tom-daubney-arm · 2023-05-18T17:28:08Z

@ivq Will you have time to make the small adjustment to the wording to get this PR through review? It would also need a backport and then we can merge it.

ivq · 2023-05-28T09:29:50Z

Updated. Feel free to point out any wording improvement. Sorry for my poor English.

mpg

This is better, but unfortunately, reviewing this made me realise that just documentation is not going to be enough here.

mpg · 2023-05-30T07:52:17Z

include/mbedtls/ecp.h

@@ -205,6 +205,9 @@ mbedtls_ecp_point;
 * cardinality is denoted by \p N. Our code requires that \p N is an
 * odd prime as mbedtls_ecp_mul() requires an odd number, and
 * mbedtls_ecdsa_sign() requires that it is prime for blinding purposes.
+ * The default implementation only initializes \p A without setting it to the
+ * authentic value for curves with <code>A = -3</code>(SECP256R1, etc),
+ * so pay attention to \p A when using domain parameters.


This is clearly an improvement over the existing, but as a general rule documentation shouldn't just say "pay attention" but give concrete guidance. Unfortunately here I'm not sure what users are supposed to do: mbedtls_cmp_int(grp.A, 0) == 0; is not precise enough (some curves do have A == 0), the only correct test would be grp.A.p == NULL, but the field p of mbedtls_mpi is private.

So, we have a weird situation where mbedtls_ecp_group::A is public, but an user can't make sense of it without accessing a private field...

I think documentation alone is not going to be enough here, and we need a new public function. I'm thinking something like mbedtls_ecp_group_a_is_minus_3() returns 1 is A is -3 mod P and returns 0 otherwise. Then the instructions here would be that for Short Weierstrass curves you need to call this function first before trying to use A.

@gilles-peskine-arm wdyt?

Note: for 2.28 fields are not private, so we could do without the new function, but OTOH I don't think it would be a problem to add such small function even in an LTS.

Looking back, making A and B public was probably a mistake (mine in #4595). A lot of code relies on P and N, even G gets used in ECDSA, but in our code base A and B are only used in ecp*.c and in pkparse to check whether a given list of parameters correspond to a known curve. But it's too late to go back on that in 3.x.

Given that it's public — and even if it wasn't public, for that matter — I'd be tempted to take advantage of the fact that mbedtls_mpi can represent negative numbers and represent this special value as -3 (which would be the only supported negative value). I think this wouldn't change the code size at the point of use (just replacing grp->A.p == 0 by grp->A.s < 0), but it would add a few bytes of code where the groups are defined. However, using negative bignums feels wrong: we're trying to get rid of them.

So I guess the best thing is to keep the representation as is. And since we've made it a public interface, as you suggest, create a new public function (which can be static inline). We should use that function in ecp.c anyway because it's better documentation.

In the LTS, I wouldn't change anything (except the documentation).

In the LTS, I wouldn't change anything (except the documentation).

Actually, is adding a static inline function really a problem in an LTS? The documentation still needs to tell people what to do, and telling them to access grp->A.p feels a bit dirty.

No, adding a static inline function is fine. I'm not sure it's necessary though. But I wouldn't change ecp.c to use it: it's a risk and you did find a problem that I'd missed).

Signed-off-by: Chien Wong <[email protected]>

ivq · 2023-08-07T15:08:32Z

Updated. Added a new public API mbedtls_ecp_group_a_is_minus_3().

gilles-peskine-arm

LGTM arpart from some minor issues with the documentation.

include/mbedtls/ecp.h

gilles-peskine-arm · 2023-08-07T18:53:03Z

include/mbedtls/ecp.h

+                                     Refer to detailed description of mbedtls_ecp_group if
+                                     using domain parameters in the structure. */


This sentence applies to Weierstrass curves (for Montgomery curves, there isn't more information about A in the description of the type), so it should be before the line about Montgomery curves.

gilles-peskine-arm · 2023-08-07T18:53:21Z

include/mbedtls/ecp.h

+    mbedtls_mpi A;              /*!< For Short Weierstrass: \p A in the equation. Note that
+                                     \p A is not set to the authentic value in some cases.
+                                     For Montgomery curves: <code>(A + 2) / 4</code>.
+                                     Refer to detailed description of mbedtls_ecp_group if


Please make a Doxygen link:

Suggested change

Refer to detailed description of mbedtls_ecp_group if

Refer to detailed description of ::mbedtls_ecp_group if

gilles-peskine-arm · 2023-08-08T15:52:39Z

Can you please do the rework by making a commit on top of the previous commit that we reviewed? Force-pushing after a review makes it hard to re-review without doing a complete review from scratch, and that's very time-consuming.

Signed-off-by: Chien Wong <[email protected]>

ivq · 2023-08-09T04:40:11Z

Can you please do the rework by making a commit on top of the previous commit that we reviewed? Force-pushing after a review makes it hard to re-review without doing a complete review from scratch, and that's very time-consuming.

I made a commit on top of commit 153ae46 .
Is it OK now?

mpg

LGTM, thanks!

gilles-peskine-arm

LGTM

gilles-peskine-arm

Please add a changelog entry.

include/mbedtls/ecp.h

gilles-peskine-arm · 2023-08-09T08:22:38Z

Please make a backport: a pull request for the branch mbedtls-2.28 with the same changes in ecp.h (documentation, new static inline function) and the changelog entry, but no changes in the .c files (to minimize the risk).

Signed-off-by: Chien Wong <[email protected]>

gilles-peskine-arm

LGTM

ivq force-pushed the ecp_doc branch from 04668e8 to 4540aa4 Compare January 31, 2023 15:49

ivq changed the title ~~Add note on special use of A in ecp group structure~~ Doc: Add note on special use of A in ecp group structure Feb 21, 2023

tom-daubney-arm added the historical-reviewing Currently reviewing (for legacy PR/issues) label May 18, 2023

gilles-peskine-arm requested changes May 18, 2023

View reviewed changes

ivq force-pushed the ecp_doc branch from 4540aa4 to ff91698 Compare May 28, 2023 09:27

mpg reviewed May 30, 2023

View reviewed changes

gilles-peskine-arm mentioned this pull request Jun 22, 2023

Why the domain parameters for secp384r1 doesn't have parameter A? #7817

Closed

Improve doc on special use of A in ecp group structure

153ae46

Signed-off-by: Chien Wong <[email protected]>

ivq force-pushed the ecp_doc branch from ff91698 to 153ae46 Compare August 7, 2023 15:07

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work single-reviewer This PR qualifies for having only one reviewer labels Aug 7, 2023

gilles-peskine-arm requested changes Aug 7, 2023

View reviewed changes

ivq force-pushed the ecp_doc branch 3 times, most recently from 6bcdd9c to 237def0 Compare August 8, 2023 14:34

gilles-peskine-arm removed the needs-review Every commit must be reviewed by at least two team members, label Aug 8, 2023

Fix doc

aa9a158

Signed-off-by: Chien Wong <[email protected]>

ivq force-pushed the ecp_doc branch from 237def0 to aa9a158 Compare August 9, 2023 04:36

mpg previously approved these changes Aug 9, 2023

View reviewed changes

gilles-peskine-arm mentioned this pull request Aug 9, 2023

Why the cofactor A from secp256r1 of ECDSA is NULL #8045

Closed

gilles-peskine-arm approved these changes Aug 9, 2023

View reviewed changes

gilles-peskine-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-work labels Aug 9, 2023

gilles-peskine-arm requested changes Aug 9, 2023

View reviewed changes

include/mbedtls/ecp.h Show resolved Hide resolved

gilles-peskine-arm added needs-work and removed approved Design and code approved - may be waiting for CI or backports labels Aug 9, 2023

Add changelog

a559c05

Signed-off-by: Chien Wong <[email protected]>

ivq dismissed mpg’s stale review via a559c05 August 9, 2023 13:52

ivq mentioned this pull request Aug 9, 2023

Backport 2.28: Doc: Add note on special use of A in ecp group structure #8046

Merged

3 tasks

gilles-peskine-arm approved these changes Aug 9, 2023

View reviewed changes

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Aug 9, 2023

mpg approved these changes Aug 10, 2023

View reviewed changes

tom-cosgrove-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Aug 10, 2023

mpg removed the needs-backports Backports are missing or are pending review and approval. label Aug 10, 2023

mpg added this pull request to the merge queue Aug 10, 2023

Merged via the queue into Mbed-TLS:development with commit 91c8372 Aug 10, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Doc: Add note on special use of A in ecp group structure #6999

Doc: Add note on special use of A in ecp group structure #6999

ivq commented Jan 31, 2023 •

edited by mpg

Loading

gilles-peskine-arm left a comment

gilles-peskine-arm May 18, 2023

mpg May 19, 2023

gilles-peskine-arm May 18, 2023

tom-daubney-arm commented May 18, 2023

ivq commented May 28, 2023

mpg left a comment

mpg May 30, 2023

mpg May 30, 2023

gilles-peskine-arm May 30, 2023 •

edited

Loading

mpg Aug 8, 2023

gilles-peskine-arm Aug 8, 2023

ivq commented Aug 7, 2023

gilles-peskine-arm left a comment

gilles-peskine-arm Aug 7, 2023

gilles-peskine-arm Aug 7, 2023

gilles-peskine-arm commented Aug 8, 2023

ivq commented Aug 9, 2023 •

edited

Loading

mpg left a comment

gilles-peskine-arm left a comment

gilles-peskine-arm left a comment

gilles-peskine-arm commented Aug 9, 2023

gilles-peskine-arm left a comment

		Refer to detailed description of mbedtls_ecp_group if
		using domain parameters in the structure. */

	Refer to detailed description of mbedtls_ecp_group if
	Refer to detailed description of ::mbedtls_ecp_group if

Doc: Add note on special use of A in ecp group structure #6999

Doc: Add note on special use of A in ecp group structure #6999

Conversation

ivq commented Jan 31, 2023 • edited by mpg Loading

Description

Gatekeeper checklist

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tom-daubney-arm commented May 18, 2023

ivq commented May 28, 2023

mpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilles-peskine-arm May 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivq commented Aug 7, 2023

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Aug 8, 2023

ivq commented Aug 9, 2023 • edited Loading

mpg left a comment

Choose a reason for hiding this comment

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Aug 9, 2023

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

ivq commented Jan 31, 2023 •

edited by mpg

Loading

gilles-peskine-arm May 30, 2023 •

edited

Loading

ivq commented Aug 9, 2023 •

edited

Loading