Privatize functions and some types/macros in asymmetric crypto headers #290

2025-05-09T05:53:13-05:00

felixc-arm commented

2025-05-09 05:53:13 -05:00

(Migrated from github.com)

Privatizes all functions in the following files by guarding them with MBEDTLS_DECLARE_PRIVATE_IDENTIFIERS

bignum.h
ecdh.h
ecdsa.h
ecjpake.h
ecp.h

Privatizes the types:
mbedtls_ecdh_side
mbedtls_ecp_curve_type
mbedtls_ecp_curve_info

Privatizes the macros:
MBEDTLS_MPI_MAX_BITS
MBEDTLS_MPI_MAX_BITS_SCALE100
MBEDTLS_LN_2_DIV_LN_10_SCALE100
MBEDTLS_MPI_RW_BUFFER_SIZE
MBEDTLS_ECDSA_MAX_SIG_LEN
MBEDTLS_ECDSA_MAX_LEN
MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED
MBEDTLS_ECP_MONTGOMERY_ENABLED
MBEDTLS_ECP_DP_MAX
MBEDTLS_ECP_OPS_CHK
MBEDTLS_ECP_OPS_DBL
MBEDTLS_ECP_OPS_ADD
MBEDTLS_ECP_OPS_INV
MBEDTLS_ECP_BUDGET

Resolves https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/227

n.b. I made changes in psa_util to unlink MBEDTLS_ECDSA_MAX_SIG_LEN from the Doxygen documentation. Although this would have had to be done anyway in https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/281 as psa_util.h is public and ecdsa.h will become internal/no longer rendered, I just wanted to make this specific change a bit more visible, as although psa-transition.md seems to imply that this macro is going away ("The following are specific to the DER format and therefore have no PSA equivalent: MBEDTLS_ECDSA_MAX_SIG_LEN, MBEDTLS_ECDSA_MAX_LEN"), I just want to make sure that the change here makes sense in terms of documentation.

Also includes the addition of the macros MBEDTLS_ECDSA_DER_MAX_SIG_LEN & MBEDTLS_ECDSA_DER_MAX_LEN, which are public equivalents of MBEDTLS_ECDSA_MAX_SIG_LEN & MBEDTLS_ECDSA_MAX_LEN, specifically for this patch to fix a Doxygen issue, but useful to have anyway.

PR Checklist

changelog not required because: will come later encompassing all privatization using MBEDTLS_DECLARE_PRIVATE_IDENTIFIERS
framework PR not required
mbedtls development PR not required because: crypto change only
mbedtls 3.6 PR not required because: 4.0/1.0 only change
tests not required because: no functional change

Privatizes all functions in the following files by guarding them with `MBEDTLS_DECLARE_PRIVATE_IDENTIFIERS` ``` bignum.h ecdh.h ecdsa.h ecjpake.h ecp.h ``` Privatizes the types: `mbedtls_ecdh_side` `mbedtls_ecp_curve_type` `mbedtls_ecp_curve_info` Privatizes the macros: `MBEDTLS_MPI_MAX_BITS` `MBEDTLS_MPI_MAX_BITS_SCALE100` `MBEDTLS_LN_2_DIV_LN_10_SCALE100` `MBEDTLS_MPI_RW_BUFFER_SIZE` `MBEDTLS_ECDSA_MAX_SIG_LEN` `MBEDTLS_ECDSA_MAX_LEN` `MBEDTLS_ECP_SHORT_WEIERSTRASS_ENABLED` `MBEDTLS_ECP_MONTGOMERY_ENABLED` `MBEDTLS_ECP_DP_MAX` `MBEDTLS_ECP_OPS_CHK` `MBEDTLS_ECP_OPS_DBL` `MBEDTLS_ECP_OPS_ADD` `MBEDTLS_ECP_OPS_INV` `MBEDTLS_ECP_BUDGET` Resolves https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/227 ~~n.b. I made changes in `psa_util` to unlink `MBEDTLS_ECDSA_MAX_SIG_LEN` from the Doxygen documentation. Although this would have had to be done anyway in https://github.com/Mbed-TLS/TF-PSA-Crypto/issues/281 as `psa_util.h` is public and `ecdsa.h` will become internal/no longer rendered, I just wanted to make this specific change a bit more visible, as although `psa-transition.md` seems to imply that this macro is going away ("The following are specific to the DER format and therefore have no PSA equivalent: MBEDTLS_ECDSA_MAX_SIG_LEN, MBEDTLS_ECDSA_MAX_LEN"), I just want to make sure that the change here makes sense in terms of documentation.~~ Also includes the addition of the macros `MBEDTLS_ECDSA_DER_MAX_SIG_LEN` & `MBEDTLS_ECDSA_DER_MAX_LEN`, which are public equivalents of `MBEDTLS_ECDSA_MAX_SIG_LEN` & `MBEDTLS_ECDSA_MAX_LEN`, specifically for this patch to fix a Doxygen issue, but useful to have anyway. ## PR Checklist - [x] **changelog** not required because: will come later encompassing all privatization using `MBEDTLS_DECLARE_PRIVATE_IDENTIFIERS` - [x] **framework PR** not required - [x] **mbedtls development PR** not required because: crypto change only - [x] **mbedtls 3.6 PR** not required because: 4.0/1.0 only change - **tests** not required because: no functional change

mpg commented

2025-05-21 04:52:00 -05:00

(Migrated from github.com)

although psa-transition.md seems to imply that this macro is going away ("The following are specific to the DER format and therefore have no PSA equivalent: MBEDTLS_ECDSA_MAX_SIG_LEN, MBEDTLS_ECDSA_MAX_LEN"), I just want to make sure that the change here makes sense in terms of documentation.

I think we need public replacements for these macros, so that people know how to size their output buffer when using MBEDTLS_ECDSA_MAX_LEN - and even pk_sign() (if we decide to keep it).

I think we want them defined in a proper public header - probably psa_util.h (or pk.h if we keep pk_sign()?). We might take this opportunity to rename them and include DER in their name.

Suggestion: in psa_util.h:

/**
 * \brief           Maximum size of a DER-encoded ECDSA signaturefor a given curve bit size
 *
 * \param bits      Curve size in bits
 * \return          Maximum signature size in bytes
 *
 * \note            This macro returns a compile-time constant if its argument
 *                  is one. It may evaluate its argument multiple times.
 */
/*
 *     Ecdsa-Sig-Value ::= SEQUENCE {
 *         r       INTEGER,
 *         s       INTEGER
 *     }
 *
 * For each of r and s, the value (V) may include an extra initial "0" bit.
 */
#define MBEDTLS_ECDSA_DER_MAX_SIG_LEN(bits)                               \
    (/*T,L of SEQUENCE*/ ((bits) >= 61 * 8 ? 3 : 2) +              \
     /*T,L of r,s*/ 2 * (((bits) >= 127 * 8 ? 3 : 2) +     \
                         /*V of r,s*/ ((bits) + 8) / 8))

/** The maximal size of a DER-encoded ECDSA signature in Bytes. */
#define MBEDTLS_ECDSA_DER_MAX_LEN  MBEDTLS_ECDSA_MAX_SIG_LEN(PSA_VENDOR_ECC_MAX_CURVE_BITS)

That is, mostly the same as in ecdsa.h, but using new names and using PSA_VENDOR_ECC_MAX_CURVE_BITS on the last line.

Wdyt? Cc @gilles-peskine-arm

> although `psa-transition.md` seems to imply that this macro is going away ("The following are specific to the DER format and therefore have no PSA equivalent: MBEDTLS_ECDSA_MAX_SIG_LEN, MBEDTLS_ECDSA_MAX_LEN"), I just want to make sure that the change here makes sense in terms of documentation. I think we need public replacements for these macros, so that people know how to size their output buffer when using `MBEDTLS_ECDSA_MAX_LEN` - and even `pk_sign()` (if we decide to keep it). I think we want them defined in a proper public header - probably `psa_util.h` (or `pk.h` if we keep `pk_sign()`?). We might take this opportunity to rename them and include `DER` in their name. Suggestion: in `psa_util.h`: ```c /** * \brief Maximum size of a DER-encoded ECDSA signaturefor a given curve bit size * * \param bits Curve size in bits * \return Maximum signature size in bytes * * \note This macro returns a compile-time constant if its argument * is one. It may evaluate its argument multiple times. */ /* * Ecdsa-Sig-Value ::= SEQUENCE { * r INTEGER, * s INTEGER * } * * For each of r and s, the value (V) may include an extra initial "0" bit. */ #define MBEDTLS_ECDSA_DER_MAX_SIG_LEN(bits) \ (/*T,L of SEQUENCE*/ ((bits) >= 61 * 8 ? 3 : 2) + \ /*T,L of r,s*/ 2 * (((bits) >= 127 * 8 ? 3 : 2) + \ /*V of r,s*/ ((bits) + 8) / 8)) /** The maximal size of a DER-encoded ECDSA signature in Bytes. */ #define MBEDTLS_ECDSA_DER_MAX_LEN MBEDTLS_ECDSA_MAX_SIG_LEN(PSA_VENDOR_ECC_MAX_CURVE_BITS) ``` That is, mostly the same as in `ecdsa.h`, but using new names and using `PSA_VENDOR_ECC_MAX_CURVE_BITS` on the last line. Wdyt? Cc @gilles-peskine-arm

mpg (Migrated from github.com) reviewed

2025-05-21 05:00:09 -05:00

mpg (Migrated from github.com) left a comment

LGTM except for the one point about the ECDSA DER len macros.

include/mbedtls/psa_util.h

					
				@ -112,0 +117,4 @@

				 * \return          Maximum signature size in bytes.

				 *

				 * \note            This macro returns a compile-time constant if its argument

				 *                  is one. It may evaluate its argument multiple times.

As indicated in a previous comment, I think we want a replacement here. TBD whether this needs to be part of this PR (I'm inclined to say yes, until it turns out to be more complex than expected) or can be done in a follow-up.

felixc-arm (Migrated from github.com) reviewed

2025-05-21 05:39:40 -05:00

include/mbedtls/psa_util.h

					
				@ -112,0 +117,4 @@

				 * \return          Maximum signature size in bytes.

				 *

				 * \note            This macro returns a compile-time constant if its argument

				 *                  is one. It may evaluate its argument multiple times.

Make sense to me, I've added the replacements in following your suggestion above. Would this then need a change in the mbedtls documentation? i.e. in psa-transition.md:

The following are specific to the DER format and therefore have no PSA equivalent: MBEDTLS_ECDSA_MAX_SIG_LEN, MBEDTLS_ECDSA_MAX_LEN

might need to mention the new macros, although I guess they're still not PSA macros, just public mbedtls macros (?)

Make sense to me, I've added the replacements in following your suggestion above. Would this then need a change in the `mbedtls` documentation? i.e. in `psa-transition.md`: >The following are specific to the DER format and therefore have no PSA equivalent: MBEDTLS_ECDSA_MAX_SIG_LEN, MBEDTLS_ECDSA_MAX_LEN might need to mention the new macros, although I guess they're still not `PSA` macros, just public `mbedtls` macros (?)

bjwtaylor (Migrated from github.com) approved these changes

2025-05-21 05:45:39 -05:00

gilles-peskine-arm (Migrated from github.com) reviewed

2025-05-22 02:40:14 -05:00

include/mbedtls/psa_util.h

					
				@ -112,0 +117,4 @@

				 * \return          Maximum signature size in bytes.

				 *

				 * \note            This macro returns a compile-time constant if its argument

				 *                  is one. It may evaluate its argument multiple times.

I guess MBEDTLS_PK_SIGNATURE_MAX_SIZE could be the public way of knowing how large a signature can grow. But this needs some kind of ECDSA-specific macro to be visible to the compiler. And it's also useful for users who implement an ECDSA-based protocol to know how large an ECDSA signature can get, either for a given curve or for all supported curves. So I concur that it's a good idea to have public macros for ECDSA signature length.

I guess `MBEDTLS_PK_SIGNATURE_MAX_SIZE` could be the public way of knowing how large a signature can grow. But this needs some kind of ECDSA-specific macro to be visible to the compiler. And it's also useful for users who implement an ECDSA-based protocol to know how large an ECDSA signature can get, either for a given curve or for all supported curves. So I concur that it's a good idea to have public macros for ECDSA signature length.

felixc-arm commented

2025-05-22 10:27:07 -05:00

(Migrated from github.com)

Just needed a rebase to fix the GCC-15 CI job, otherwise no extra change 👍

Just needed a rebase to fix the GCC-15 CI job, otherwise no extra change :+1:

mpg (Migrated from github.com) approved these changes

2025-05-26 02:33:23 -05:00

mpg (Migrated from github.com) left a comment

LGTM, thanks!

Sign in to join this conversation.

No reviewers

bjwtaylor

mpg

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-Mbed-TLS/TF-PSA-Crypto!290

Rows
Columns