ASN1 integer encoding by bjwtaylor · Pull Request #289 · Mbed-TLS/TF-PSA-Crypto

bjwtaylor · 2025-05-09T07:42:38Z

Description

Introduce a new function for asn1 encoding of mpi integers. resolves Mbed-TLS/mbedtls#9372 depends #184.

There is currently a bug in #184, which is causing the tests to fail, I will rebase it once this is fixed.

PR checklist

changelog provided
framework PR not required
mbedtls development PR not required because: No changes
mbedtls 3.6 PR not required because: No backports
tests provided

gilles-peskine-arm

The design is on the right track, but to do a better review I'd need to see the new function's documentation. In particular I don't understand the role of buffer_length: it seems redundant with integer_length?

gilles-peskine-arm

I just reviewed the documentation of mbedtls_asn1_write_integer.

I expected mbedtls_asn1_write_integer to be used in library code, at least to implement mbedtls_asn1_write_integer. It's ok to not do that yet, we can do it later. But having used the function in more than unit tests would help us validate that the design is good.

gilles-peskine-arm · 2025-06-11T15:36:41Z

+ *
+ * \param p                The reference to the current position pointer.
+ * \param start            The start of the buffer, for bounds-checking.
+ * \param integer          The start of the integer buffer.


Is it ok if integer overlaps with the output buffer? That can be useful in typical cases that we'll encounter when printing out a bignum. For that we first convert the bignum to a big-endian byte array, then convert that to ASN.1. If we can do the second step in place, then we don't need to allocate an intermediate step.

We may not want to promise that it works if it's too difficult to implement. We do have to be careful that the ASN.1 representation is usually a few bytes larger than the byte array (because there's 2 or more bytes of overhead for the ASN.1 type, ASN.1 length and the possible additional byte for the ASN.1 sign bit), but it can also be smaller if the byte array has leading zeros.

Currently overlap is not handled, but it should be possible using memmove(). I'll have a careful think about it tomorrow and see if it is easy to do.

Given that we're pressed for time, I suggest making that a follow-up in 1.1.

gilles-peskine-arm · 2025-06-17T19:26:36Z

#184 is merged, so please rebase on top of development.

Instead, make integer_start const Signed-off-by: David Horstmann <david.horstmann@arm.com>

These will cause a NULL dereference if they are NULL. Signed-off-by: David Horstmann <david.horstmann@arm.com>

We still want to test a non-NULL output buffer that is zero-length. Signed-off-by: David Horstmann <david.horstmann@arm.com>

Don't set p to its previous position on failure, since this is untested. Signed-off-by: David Horstmann <david.horstmann@arm.com>

Signed-off-by: David Horstmann <david.horstmann@arm.com>

davidhorstmann-arm · 2025-06-30T11:13:57Z

Second rebase completed (sorry for the delay). I have squashed some commits together and done some merging so that there are only 45 commits total (rather than 67).

The commits from 1a3e2cb onwards I have left unchanged. before that point there were previously 39 commits and there are now 18.

If this is still too much to review commit-by-commit I can do some more rebasing work, let me know.

gilles-peskine-arm

Explicitly approving after the partial history squash from a39d1ea to 4e726ae

bjwtaylor · 2025-06-30T11:59:29Z

LGTM

davidhorstmann-arm

Approving on behalf of:

Myself, for @bjwtaylor's commits
@bjwtaylor, for my commits

mpg · 2025-06-30T13:39:12Z

I think this needs a ChangeLog entry. #337 took care of the migration guide part, but we still need a ChangeLog entry. (We have one of for the parsing side.)

davidhorstmann-arm · 2025-06-30T13:41:52Z

I think this needs a ChangeLog entry. #337 took care of the migration guide part, but we still need a ChangeLog entry. (We have one of for the parsing side.)

Ah you're right, I'll add that momentarily.

Signed-off-by: David Horstmann <david.horstmann@arm.com>

gilles-peskine-arm

LGTM

bjwtaylor · 2025-06-30T14:06:07Z

Changelog also looks good.

davidhorstmann-arm

Approving again on behalf of myself and @bjwtaylor

bjwtaylor changed the title ~~Asn1 integer encoding~~ ASN1 integer encoding May 9, 2025

bjwtaylor requested a review from gilles-peskine-arm May 9, 2025 08:14

gilles-peskine-arm requested changes May 14, 2025

View reviewed changes

Comment thread drivers/builtin/src/asn1write.c Outdated

Comment thread drivers/builtin/src/asn1write.c Outdated

Comment thread drivers/builtin/src/asn1write.c Outdated

bjwtaylor force-pushed the ASN1-integer-encoding branch 3 times, most recently from b09ea1a to 62cfb00 Compare May 28, 2025 09:17

bjwtaylor force-pushed the ASN1-integer-encoding branch 2 times, most recently from 76064e8 to 41047aa Compare May 29, 2025 09:28

bjwtaylor added the needs-preceding-pr Requires another PR to be merged first label May 29, 2025

bjwtaylor marked this pull request as ready for review June 3, 2025 14:06

bjwtaylor added needs-review Every commit must be reviewed by at least two team members needs-reviewer This PR needs someone to pick it up for review needs-ci Needs to pass CI tests labels Jun 3, 2025

valeriosetti self-requested a review June 5, 2025 14:39

valeriosetti added needs-review Every commit must be reviewed by at least two team members and removed needs-review Every commit must be reviewed by at least two team members needs-reviewer This PR needs someone to pick it up for review labels Jun 9, 2025

bjwtaylor requested a review from gilles-peskine-arm June 10, 2025 08:22

gilles-peskine-arm requested changes Jun 11, 2025

View reviewed changes

gilles-peskine-arm added this to Roadmap pull requests (new board) Jun 17, 2025

github-project-automation Bot moved this to In Development in Roadmap pull requests (new board) Jun 17, 2025

gilles-peskine-arm added priority-high High priority - will be reviewed soon needs-work and removed needs-review Every commit must be reviewed by at least two team members needs-ci Needs to pass CI tests labels Jun 17, 2025

davidhorstmann-arm added 5 commits June 30, 2025 11:59

Remove useless cast

ebbbb01

Instead, make integer_start const Signed-off-by: David Horstmann <david.horstmann@arm.com>

Do not check for zero-length output buffers

0f6b5ba

These will cause a NULL dereference if they are NULL. Signed-off-by: David Horstmann <david.horstmann@arm.com>

Re-add zero-length test

16e2a36

We still want to test a non-NULL output buffer that is zero-length. Signed-off-by: David Horstmann <david.horstmann@arm.com>

Remove guarantee about p on failure

49db86f

Don't set p to its previous position on failure, since this is untested. Signed-off-by: David Horstmann <david.horstmann@arm.com>

Fix buffer overread in tests

4e726ae

Signed-off-by: David Horstmann <david.horstmann@arm.com>

davidhorstmann-arm force-pushed the ASN1-integer-encoding branch from a39d1ea to 4e726ae Compare June 30, 2025 11:10

davidhorstmann-arm removed the needs-work label Jun 30, 2025

gilles-peskine-arm previously approved these changes Jun 30, 2025

View reviewed changes

davidhorstmann-arm previously approved these changes Jun 30, 2025

View reviewed changes

github-project-automation Bot moved this from In Development to Has Approval in Roadmap pull requests (new board) Jun 30, 2025

davidhorstmann-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 Jun 30, 2025

Add ChangeLog entry for ASN.1 integer function

6a525ed

Signed-off-by: David Horstmann <david.horstmann@arm.com>

davidhorstmann-arm dismissed stale reviews from gilles-peskine-arm and themself via 6a525ed June 30, 2025 13:52

davidhorstmann-arm requested a review from gilles-peskine-arm June 30, 2025 13:52

gilles-peskine-arm approved these changes Jun 30, 2025

View reviewed changes

davidhorstmann-arm added the needs-review Every commit must be reviewed by at least two team members label Jun 30, 2025

davidhorstmann-arm approved these changes Jun 30, 2025

View reviewed changes

davidhorstmann-arm removed the needs-review Every commit must be reviewed by at least two team members label Jun 30, 2025

gilles-peskine-arm enabled auto-merge June 30, 2025 16:12

gilles-peskine-arm added this pull request to the merge queue Jul 1, 2025

Merged via the queue into Mbed-TLS:development with commit f6b7624 Jul 1, 2025
4 of 7 checks passed

github-project-automation Bot moved this from Has Approval to Done in Roadmap pull requests (new board) Jul 1, 2025

Conversation

bjwtaylor commented May 9, 2025 • edited by davidhorstmann-arm Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

PR checklist

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gilles-peskine-arm Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

davidhorstmann-arm Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Jun 24, 2025

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm commented Jun 17, 2025

Uh oh!

davidhorstmann-arm commented Jun 30, 2025

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

bjwtaylor commented Jun 30, 2025

Uh oh!

davidhorstmann-arm left a comment

Choose a reason for hiding this comment

Uh oh!

mpg commented Jun 30, 2025

Uh oh!

davidhorstmann-arm commented Jun 30, 2025

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

bjwtaylor commented Jun 30, 2025

Uh oh!

davidhorstmann-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bjwtaylor commented May 9, 2025 •

edited by davidhorstmann-arm

Loading