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 return code #3705

Merged

Conversation

ctron
Copy link
Contributor

@ctron ctron commented Sep 22, 2020

Fix #2512

Notes:

  • Pull requests cannot be accepted until the PR follows the contributing guidelines. In particular, each commit must have at least one Signed-off-by: line from the committer to certify that the contribution is made under the terms of the Developer Certificate of Origin.
  • This is just a template, so feel free to use/remove the unnecessary things

Description

A few sentences describing the overall goals of the pull request's commits.

In all other cases, the invalid public key error, caused by an ASN1 length mismatch error, is calculated as:

https://github.com/ARMmbed/mbedtls/blob/5cb54f7b27e1ebc1602233f50c44c1ff05304852/library/pkparse.c#L638-L640

In the case this PR fixes, a + is missing:

https://github.com/ARMmbed/mbedtls/blob/5cb54f7b27e1ebc1602233f50c44c1ff05304852/library/pkparse.c#L664-L666

From a mathematical point of view, it doesn't matter, since in one case it expands as:

-0x3B00 + -0x0066

And in the other as:

-0x3B00 -0x0066

Which results in the same outcome.

However, should someone decide to change to defined values in the future, it might break compilation or come to a different outcome.

Status

READY

Requires Backporting

Yes, #3707 and #3708.

Additional comments

Any additional information that could be of interest

Todos

  • Tests
  • Documentation
  • Changelog updated
  • Backported

Steps to test or reproduce

Outline the steps to test or reproduce the PR here.

Signed-off-by: Jens Reimann <jreimann@redhat.com>
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is indeed better like this. Thanks. The CI is ok, the Mbed OS Testing is currently broken and we are about to remove it. The error reported by "continuous-integration/jenkins/pr-head" and "PR-3705-head TLS Testing" are CI glitches.

@ronald-cron-arm ronald-cron-arm added this to To do in Mbed TLS PRs via automation Sep 22, 2020
@ronald-cron-arm ronald-cron-arm moved this from To do to 1 Approval in Mbed TLS PRs Sep 22, 2020
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This was actually reported before #2512 but as there was no functional issue we'd never bothered to make the patch.

Mbed TLS PRs automation moved this from 1 Approval to Approved Sep 22, 2020
@gilles-peskine-arm gilles-peskine-arm added the needs-backports Backports are missing or are pending review and approval. label Sep 22, 2020
@ronald-cron-arm
Copy link
Contributor

@ctron I've done the backport PRs: #3707 and #3708 to fix it everywhere while at it. I hope this is ok with you.

@gilles-peskine-arm gilles-peskine-arm merged commit 9b33eb3 into Mbed-TLS:development Sep 22, 2020
Mbed TLS PRs automation moved this from Approved to Done Sep 22, 2020
@daverodgman daverodgman removed this from Done in Mbed TLS PRs Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement needs-backports Backports are missing or are pending review and approval.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Weird result code from mbedtls_pk_parse_subpubkey()
3 participants