Skip to content

x/crypto/bn256: TestG1Marshal fails with Go tip #25199

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.

Sign up for GitHub

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
Closed
mundaym opened this issue May 1, 2018 · 5 comments
Closed

x/crypto/bn256: TestG1Marshal fails with Go tip #25199

mundaym opened this issue May 1, 2018 · 5 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone
Go1.11

Comments

@mundaym
Copy link
Member

mundaym commented May 1, 2018

This package calls math/big.ModInverse and does not check that the result is non-nil, which appears to result in a segmentation fault when it is tested against Go tip.

See https://storage.googleapis.com/go-build-log/9ecf899b/freebsd-amd64-11_1_a2ef6f73.log for an example.
@gopherbot gopherbot added this to the Unreleased milestone May 1, 2018
@mundaym mundaym added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 1, 2018
@mundaym
Copy link
Member Author

mundaym commented May 1, 2018

Presumably a result of CL 108996.

cc @bmkessler @griesemer @FiloSottile

@bradfitz bradfitz modified the milestones: Unreleased, Go1.11 May 1, 2018
@bradfitz
Copy link
Contributor

bradfitz commented May 1, 2018

Presumably a result of CL 108996.

Yes, it was CL 108996.

@griesemer
Copy link
Contributor 

diff --git a/bn256/curve.go b/bn256/curve.go
index 55b7063..9b8ff23 100644
--- a/bn256/curve.go
+++ b/bn256/curve.go
@@ -251,6 +251,9 @@ func (c *curvePoint) MakeAffine(pool *bnPool) *curvePoint {
        }
 
        zInv := pool.Get().ModInverse(c.z, p)
+       if zInv == nil {
+               zInv = pool.Get()
+       }
        t := pool.Get().Mul(c.y, zInv)
        t.Mod(t, p)
        zInv2 := pool.Get().Mul(zInv, zInv)

essentially restores the old behavior and makes the test pass. But I don't know if this is the correct fix. (If the ModInverse does not exist, in the past the returned value was undefined/the behavior of ModInverse was not specified.)

@agl ?

@griesemer griesemer added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 1, 2018
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/110695 mentions this issue: bn256: skip converting to affine coordinates for ∞

@FiloSottile FiloSottile assigned FiloSottile and unassigned agl May 1, 2018
@FiloSottile
Copy link
Contributor

I honestly have no idea how it used to work. I think it relied on the fact that pool.Get() was returning 0, and that ModInverse was returning 0 as well.

@gopherbot gopherbot mentioned this issue May 2, 2018
Closed
@FiloSottile FiloSottile mentioned this issue Jun 29, 2018
Closed
@golang golang locked and limited conversation to collaborators May 2, 2019
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 28, 2022
@FiloSottile
bn256: explicitly fix MakeAffine for ∞
ed5ca68 
When c = ∞, z = 0 and 0 does not have a modular inverse, triggering
undefined behavior (recently changed to returning nil) in ModInverse.

Unclear how this used to work anyway. Looks like ModInverse was
leaving the receiver untouched, making zInv = 0 when pool = nil.

Fixes golang/go#25199

Change-Id: Ib39abf59f0e71cf43cdb5836142ebdd3b206fb3f
Reviewed-on: https://go-review.googlesource.com/110695
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
c-expert-zigbee added a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
@c-expert-zigbee
bn256: explicitly fix MakeAffine for ∞
8e7f0c7 
When c = ∞, z = 0 and 0 does not have a modular inverse, triggering
undefined behavior (recently changed to returning nil) in ModInverse.

Unclear how this used to work anyway. Looks like ModInverse was
leaving the receiver untouched, making zInv = 0 when pool = nil.

Fixes golang/go#25199

Change-Id: Ib39abf59f0e71cf43cdb5836142ebdd3b206fb3f
Reviewed-on: https://go-review.googlesource.com/110695
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
c-expert-zigbee added a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
@c-expert-zigbee
bn256: explicitly fix MakeAffine for ∞
2fda71e 
When c = ∞, z = 0 and 0 does not have a modular inverse, triggering
undefined behavior (recently changed to returning nil) in ModInverse.

Unclear how this used to work anyway. Looks like ModInverse was
leaving the receiver untouched, making zInv = 0 when pool = nil.

Fixes golang/go#25199

Change-Id: Ib39abf59f0e71cf43cdb5836142ebdd3b206fb3f
Reviewed-on: https://go-review.googlesource.com/110695
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
LewiGoddard pushed a commit to LewiGoddard/crypto that referenced this issue Feb 16, 2023
@FiloSottile
bn256: explicitly fix MakeAffine for ∞
e50c3e1 
When c = ∞, z = 0 and 0 does not have a modular inverse, triggering
undefined behavior (recently changed to returning nil) in ModInverse.

Unclear how this used to work anyway. Looks like ModInverse was
leaving the receiver untouched, making zInv = 0 when pool = nil.

Fixes golang/go#25199

Change-Id: Ib39abf59f0e71cf43cdb5836142ebdd3b206fb3f
Reviewed-on: https://go-review.googlesource.com/110695
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
BiiChris pushed a commit to BiiChris/crypto that referenced this issue Sep 15, 2023
@FiloSottile
bn256: explicitly fix MakeAffine for ∞
6e92842 
When c = ∞, z = 0 and 0 does not have a modular inverse, triggering
undefined behavior (recently changed to returning nil) in ModInverse.

Unclear how this used to work anyway. Looks like ModInverse was
leaving the receiver untouched, making zInv = 0 when pool = nil.

Fixes golang/go#25199

Change-Id: Ib39abf59f0e71cf43cdb5836142ebdd3b206fb3f
Reviewed-on: https://go-review.googlesource.com/110695
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
desdeel2d0m added a commit to desdeel2d0m/crypto that referenced this issue Jul 1, 2024
@desdeel2d0m @FiloSottile
bn256: explicitly fix MakeAffine for ∞
f3c4625 
When c = ∞, z = 0 and 0 does not have a modular inverse, triggering
undefined behavior (recently changed to returning nil) in ModInverse.

Unclear how this used to work anyway. Looks like ModInverse was
leaving the receiver untouched, making zInv = 0 when pool = nil.

Fixes golang/go#25199

Change-Id: Ib39abf59f0e71cf43cdb5836142ebdd3b206fb3f
Reviewed-on: https://go-review.googlesource.com/110695
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
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. release-blocker
Projects
None yet
Milestone
Go1.11
Development

No branches or pull requests
6 participants
@bradfitz @agl @FiloSottile @griesemer @gopherbot @mundaym