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

Fix ecp_mul() so that the group can be made const. #7601

Open
mpg opened this issue May 16, 2023 · 2 comments · May be fixed by #7623
Open

Fix ecp_mul() so that the group can be made const. #7601

mpg opened this issue May 16, 2023 · 2 comments · May be fixed by #7623
Assignees
Labels
enhancement size-m Estimated task size: medium (~1w)

Comments

@mpg
Copy link
Contributor

mpg commented May 16, 2023

The function mbedtls_ecp_mul() is still not quite ready yet to have its first argument const, though that would only be a moderate amount of work now that the pre-computed multiples table are stored as static data, instead of computed at runtime. The trouble is, the code still supports computing it at runtime, which is only useful when adding a new curve, as we compute the table at runtime then dump it before we add it to ecp_curves.c, and then it never needs to be computed at runtime again - see scripts/ecp_comb_table.py.

We should really get rid of that and instead compute the table for new curves in python or using a dedicated C program, but it's ridiculous to keep code in all builds of the library that's only useful when developing the library, not when using it. Especially when it shows in the API, as some parameters that should morally be const are not.

Specifically, we need to quit modifying T and T_size. Actually these probably don't need to be part of the ecp_group structure. (Edit: yes they do, that's how we get the pre-computed table.)

@mpg mpg added enhancement size-m Estimated task size: medium (~1w) labels May 16, 2023
@mpg mpg self-assigned this May 16, 2023
@gilles-peskine-arm
Copy link
Contributor

It's not clear to me what you can do with the public API: can users take advantage of the runtime computation? Or can we remove the runtime computation without breaking the API?

  • Removing T and T_size from mbedtls_ecp_group: they're private fields so we can remove them at any time.
  • Adding const to pointer parameters in the signature of mbedtls_ecp_mul and other functions: we can't do that in 3.x because that's an API change.
  • Removing support for runtime computation: ?

@mpg
Copy link
Contributor Author

mpg commented May 16, 2023

let more provide some more context. The way we perform scalar multiplication for short Weierstrass curves is:

  1. Get some a table with some multiples of the input point.
    a. If the input point if the conventional base point (G), we should have those in ecp_curves.c (since 3.0, specifically Static initialize comb table #4315). Do we?
    - if yes: use it
    - if not: compute it then store as grp->T for re-use
    b. Otherwise, compute the table.
  2. Use the table to do the multiplication.

Note: the "if not" part of 1a is what we always did before #4315. Now, if never happens in normal use. It only happens during development, when you're in the process of adding a new curve, and haven't added its pre-computed table yet. We take advantage of that by running the code, letting it compute the table, then dumping it and copying the result to ecp_curves.c.

The "if not" part of 1a is also the only one that writes to grp->T: the "if yes" part just reads from it (if has been set by ecp_group_load()), and 1b ignores it completely: its table is specific to the variable input point, so won't be re-used, so there's no point keeping it once the computation is over.

So, what prevents us from making the group const is exactly what can never happen except during development.

With that in mind, circling back to your questions:

It's not clear to me what you can do with the public API: can users take advantage of the runtime computation? Or can we remove the runtime computation without breaking the API?

Users can't see the table unless they access private fields, so I don't see how they would take advantage of runtime computation.

Removing T and T_size from mbedtls_ecp_group: they're private fields so we can remove them at any time.

I was mistaken on that.: T points to the pre-computed table (it's populated by ecp_group_load(), so we should keep it. We just need to stop potentially writing to it in a case that doesn't happen in practice even though the compile can't know.

Adding const to pointer parameters in the signature of mbedtls_ecp_mul and other functions: we can't do that in 3.x because that's an API change.

Yes, that's out of scope of this issue, which is only about preparatory work.

Removing support for runtime computation: ?

We need to be more precise here. We'll always have runtime computation for variable-point multiplication (when the input point is not the conventional base point G). What we want to remove is the part where if the input point is the G and we somehow don't have its table pre-computed, then after computing it, we update grp->T: we want to stop writing to grp->T. The impact on normal users is non-existent since the condition "if the input point is the G and we somehow don't have its table pre-computed, then after computing it" never happens outside a PR that adds a new curve.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement size-m Estimated task size: medium (~1w)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants