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

Jump to bottom

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

Closed
roman-khimov opened this issue Sep 5, 2019 · 9 comments
Closed

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

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
Projects
Proposals (old)
Milestone
Go1.15

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
@roman-khimov roman-khimov mentioned this issue Oct 23, 2019
Closed
im-kulikov added a commit to im-kulikov/go that referenced this issue Oct 23, 2019
@im-kulikov
Implement unmarshal for compressed format for crypto/elliptic
b46a8f8 
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 im-kulikov mentioned this issue Oct 23, 2019
Closed
@gopherbot
Copy link

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
@im-kulikov
Implement unmarshal for compressed format for crypto/elliptic
d9a339d 
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
@im-kulikov
Implement unmarshal for compressed format for crypto/elliptic
a158b95 
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
@im-kulikov
Implement unmarshal for compressed format for crypto/elliptic
bfab7bc 
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 •
edited

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 •
edited

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 rsc added this to Active in Proposals (old) Feb 12, 2020
@rsc
Copy link
Contributor

rsc commented Feb 12, 2020

Adding to proposal minutes, seems headed for likely accept.

@rsc rsc mentioned this issue Feb 12, 2020
Open
im-kulikov added a commit to im-kulikov/go that referenced this issue Feb 13, 2020
@im-kulikov
crypto/elliptic: Implement marshal / unmarshal for compressed format
ea579f5 
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
@im-kulikov
crypto/elliptic: Implement marshal / unmarshal for compressed format
9b38b0a 
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 •
edited

@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 rsc moved this from Active to Likely Accept in Proposals (old) Feb 26, 2020
@rsc
Copy link
Contributor

rsc commented Mar 4, 2020

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Mar 4, 2020
@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 gdbelvin mentioned this issue Apr 1, 2020
Closed
gdbelvin added a commit to gdbelvin/keytransparency that referenced this issue Apr 7, 2020
@gdbelvin
SECG1 Encode/Decode
75503f6 
Encode and Decode elliptic curve points in compressed format.

This will be superceeded by golang/go#34105 in
Go 1.15
@gdbelvin gdbelvin mentioned this issue Apr 7, 2020
Merged
gdbelvin added a commit to google/keytransparency that referenced this issue Apr 20, 2020
@gdbelvin
SECG1 Encode/Decode (#1513)
6115a89 
* SECG1 Encode/Decode

Encode and Decode elliptic curve points in compressed format.

This will be superceeded by golang/go#34105 in
Go 1.15
@ianlancetaylor ianlancetaylor mentioned this issue Jun 12, 2020
Closed
@golang golang locked and limited conversation to collaborators May 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
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
No open projects
Proposals (old)
Accepted
Milestone
Go1.15
Development

Successfully merging a pull request may close this issue.

crypto/elliptic: implement unmarshal for compressed format im-kulikov/go
7 participants
@rsc @im-kulikov @FiloSottile @ALTree @gopherbot @roman-khimov @katiehockman