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

[1.0.2-bp][ec_asn1.c] Avoid injecting seed when built-in matches #10141

Closed

Conversation

@romen
Copy link
Member

@romen romen commented Oct 10, 2019

This is a backport of #10140 for 1.0.2 where it did not cherry-pick cleanly.

I'd like to keep the discussion about patching vs non-patching in the parent PR.

It is marked as WIP until the discussion in #10140 reaches a consensus.
Also we might need to add a CHANGES entry.

An unintended consequence of #9808
is that when an explicit parameters curve is matched against one of the
well-known builtin curves we automatically inherit also the associated
seed parameter, even if the the input parameters excluded such
parameter.

This later affects the serialization of such parsed keys, causing their
input DER encoding and output DER encoding to differ due to the
additional optional field.

This does not cause problems internally but could affect external
applications, as reported in
#9811 (comment)

This commit fixes the issue by conditionally clearing the seed field if
the original input parameters did not include it.
@romen romen added the branch: 1.0.2 label Oct 10, 2019
@romen romen requested a review from mattcaswell Oct 10, 2019
@romen romen self-assigned this Oct 10, 2019
@romen romen mentioned this pull request Oct 10, 2019
0 of 1 task complete
@romen
Copy link
Member Author

@romen romen commented Oct 10, 2019

@matthauck could you give this PR a try to verify if it does indeed resolve your problem?

@matthauck
Copy link

@matthauck matthauck commented Oct 10, 2019

Awesome, thank you! Will give this a try and report back.

@matthauck
Copy link

@matthauck matthauck commented Oct 10, 2019

This did indeed fix our issue. Thank you! 🎉 🎈

@romen romen changed the title WIP: [1.0.2-bp][ec_asn1.c] Avoid injecting seed when built-in matches [1.0.2-bp][ec_asn1.c] Avoid injecting seed when built-in matches Oct 11, 2019
@romen
Copy link
Member Author

@romen romen commented Oct 11, 2019

Out of WIP as #10140 was approved without further changes.

@t8m
t8m approved these changes Oct 11, 2019
@mspncp
Copy link
Contributor

@mspncp mspncp commented Oct 11, 2019

Same here: 'the the input parameters' -> 'the input parameters'

levitte pushed a commit that referenced this pull request Oct 15, 2019
An unintended consequence of #9808
is that when an explicit parameters curve is matched against one of the
well-known builtin curves we automatically inherit also the associated
seed parameter, even if the input parameters excluded such parameter.

This later affects the serialization of such parsed keys, causing their
input DER encoding and output DER encoding to differ due to the
additional optional field.

This does not cause problems internally but could affect external
applications, as reported in
#9811 (comment)

This commit fixes the issue by conditionally clearing the seed field if
the original input parameters did not include it.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #10141)
@romen
Copy link
Member Author

@romen romen commented Oct 15, 2019

Merged with 4e545c6

Thanks everyone and thanks again @matthauck for reporting and testing this.

@romen romen closed this Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants