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

crypto/elliptic: handle compressed formats in new MarshalCompressed, UnmarshalCompressed #34105

Closed
roman-khimov opened this issue Sep 5, 2019 · 9 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@roman-khimov
Copy link

What version of Go are you using (go version)?

$ go version
go version go1.12.9 linux/amd64

Does this issue reproduce with the latest release?

I think so.

What operating system and processor architecture are you using (go env)?

play.golang.org

What did you do?

https://play.golang.org/p/ugkQeobnYRP

What did you expect to see?

Successful unmarshaling.

What did you see instead?

Failed unmarshaling.

The real problem

elliptic.Unmarshal() only supports uncompressed format (prefix 0x04) at the moment, as can be seen in the source code, which probably is fine given that it's supposed to be a companion for elliptic.Marshal() that only uses that format to serialize the point. But at the same time it would be very nice to have support for compressed (prefixes 0x02 and 0x03) format that is being used in the wild, it doesn't cost much in terms of code (there is a nice snippet on SO) and this proposed change is certainly backwards-compatible. At the moment I see this (or similar) code being duplicated by several projects that use compressed format, thus this issue.

Of course it would be very nice if elliptic could also serialize into this format, but that's a bit more complicated, because you can't change Marshal() without breaking backwards compatibility (the only option I see at the moment is introducing another function like MarshalCompressed()).

@ALTree ALTree added the Proposal-Crypto Proposal related to crypto packages or other security issues label Sep 5, 2019
@ALTree ALTree changed the title crypto/elliptic: Unmarshal() is unable to handle compressed formats crypto/elliptic: handle compressed formats in Unmarshal Sep 5, 2019
@katiehockman katiehockman added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 5, 2019
@katiehockman
Copy link
Contributor

/cc @FiloSottile

@FiloSottile FiloSottile added this to the Unplanned milestone Sep 9, 2019
im-kulikov added a commit to im-kulikov/go that referenced this issue Oct 23, 2019
First byte can be 0x2 or 0x3 (compressed form).
To unmarshal used formula y² = x³ - 3x + b.
Reuse code from `IsOnCurve`.

Closes golang#34105 issue
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/202819 mentions this issue: crypto/elliptic: implement unmarshal for compressed format

im-kulikov added a commit to im-kulikov/go that referenced this issue Oct 23, 2019
First byte can be 0x2 or 0x3 (compressed form).
To unmarshal used formula y² = x³ - 3x + b.
Reuse code from `IsOnCurve`.

Closes golang#34105 issue
im-kulikov added a commit to im-kulikov/go that referenced this issue Oct 24, 2019
First byte can be 0x2 or 0x3 (compressed form).
To unmarshal used formula y² = x³ - 3x + b.
Reuse code from `IsOnCurve`.

Closes golang#34105 issue
im-kulikov added a commit to im-kulikov/go that referenced this issue Nov 18, 2019
First byte can be 0x2 or 0x3 (compressed form).
To unmarshal used formula y² = x³ - 3x + b.
Reuse code from `IsOnCurve`.

Closes golang#34105 issue
@FiloSottile
Copy link
Contributor

Can you provide some examples of projects that had to implement this? If it's indeed common, I'd support implementing it.

@im-kulikov
Copy link
Contributor

im-kulikov commented Dec 3, 2019

Can you provide some examples of projects that had to implement this? If it's indeed common, I'd support implementing it.

@katiehockman
Copy link
Contributor

katiehockman commented Feb 5, 2020

Supporting compressed formats in elliptic.Unmarshal() would break the backwards compatibility rule, since the docs say

It is an error if the point is not in uncompressed form or is not on the curve.

In order to support this, we could add two new functions to the elliptic package:

func MarshalCompressed(curve Curve, x, y *big.Int) []byte
func UnmarshalCompressed(curve Curve, data []byte) (x, y *big.Int)

@rsc rsc changed the title crypto/elliptic: handle compressed formats in Unmarshal crypto/elliptic: handle compressed formats in new MarshalCompressed, UnmarshalCompressed Feb 12, 2020
@rsc
Copy link
Contributor

rsc commented Feb 12, 2020

Adding to proposal minutes, seems headed for likely accept.

im-kulikov added a commit to im-kulikov/go that referenced this issue Feb 13, 2020
First byte can be 0x2 or 0x3 (compressed form).
To unmarshal used formula y² = x³ - 3x + b.
Reuse code from `IsOnCurve`.

closes golang#34105 issue
im-kulikov added a commit to im-kulikov/go that referenced this issue Feb 13, 2020
First byte can be 0x2 or 0x3 (compressed form).
To unmarshal used formula y² = x³ - 3x + b.
Reuse code from `IsOnCurve`.

closes golang#34105 issue
@im-kulikov
Copy link
Contributor

im-kulikov commented Feb 13, 2020

@katiehockman PR updated

MarshalCompressed implemented as specified in section 4.3.6 (2. compressed form) of ANSI X9.62

@rsc
Copy link
Contributor

rsc commented Feb 26, 2020

Based on the discussion above and the specific retitling of adding new functions instead of overloading the existing ones, this seems like a likely accept.

@rsc
Copy link
Contributor

rsc commented Mar 4, 2020

No change in consensus, so accepted.

@FiloSottile FiloSottile removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 4, 2020
@FiloSottile FiloSottile added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 4, 2020
@FiloSottile FiloSottile modified the milestones: Unplanned, Go1.15 Mar 4, 2020
gdbelvin added a commit to gdbelvin/keytransparency that referenced this issue Apr 7, 2020
Encode and Decode elliptic curve points in compressed format.

This will be superceeded by golang/go#34105 in
Go 1.15
gdbelvin added a commit to google/keytransparency that referenced this issue Apr 20, 2020
* SECG1 Encode/Decode

Encode and Decode elliptic curve points in compressed format.

This will be superceeded by golang/go#34105 in
Go 1.15
@golang golang locked and limited conversation to collaborators May 7, 2021
@rsc rsc moved this to Accepted in Proposals Aug 10, 2022
@rsc rsc added this to Proposals Aug 10, 2022
@rsc rsc removed this from Proposals Oct 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants