Skip to content

Commit

Permalink
fix: math: fix Uint.Unmarshal's lack of negative value checking (#11996)
Browse files Browse the repository at this point in the history
This change adds a negative value check to Uint.Unmarshal,
which coincidentally is fixed by refactoring for code reuse.
While here, added tests to ensure we don't regress.

Fixes #11995
  • Loading branch information
odeke-em authored May 19, 2022
1 parent 89c727c commit 54d764b
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 7 deletions.
12 changes: 5 additions & 7 deletions math/uint.go
Original file line number Diff line number Diff line change
Expand Up @@ -190,11 +190,8 @@ func (u *Uint) Unmarshal(data []byte) error {
return err
}

if u.i.BitLen() > MaxBitLen {
return fmt.Errorf("integer out of range; got: %d, max: %d", u.i.BitLen(), MaxBitLen)
}

return nil
// Finally check for overflow.
return UintOverflow(u.i)
}

// Size implements the gogo proto custom type interface.
Expand All @@ -213,8 +210,9 @@ func UintOverflow(i *big.Int) error {
if i.Sign() < 0 {
return errors.New("non-positive integer")
}
if i.BitLen() > 256 {
return fmt.Errorf("bit length %d greater than 256", i.BitLen())

if g, w := i.BitLen(), MaxBitLen; g > w {
return fmt.Errorf("integer out of range; got: %d, max: %d", g, w)
}
return nil
}
Expand Down
38 changes: 38 additions & 0 deletions math/uint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"math"
"math/big"
"math/rand"
"strings"
"testing"

sdkmath "cosmossdk.io/math"
Expand Down Expand Up @@ -300,6 +301,7 @@ func TestRoundTripMarshalToUint(t *testing.T) {
1<<63 - 1,
1<<32 - 7,
1<<22 - 8,
math.MaxUint64,
}

for _, value := range values {
Expand All @@ -323,3 +325,39 @@ func TestRoundTripMarshalToUint(t *testing.T) {
})
}
}

func TestWeakUnmarshalNegativeSign(t *testing.T) {
neg10, _ := new(big.Int).SetString("-10", 0)
blob, err := neg10.MarshalText()
if err != nil {
t.Fatal(err)
}

ui := new(sdkmath.Uint)
err = ui.Unmarshal(blob)
if err == nil {
t.Fatal("Failed to catch the negative value")
}
if errStr := err.Error(); !strings.Contains(errStr, "non-positive") {
t.Fatalf("negative value not reported, got instead %q", errStr)
}
}

func TestWeakUnmarshalOverflow(t *testing.T) {
exp := new(big.Int).SetUint64(256)
pos10, _ := new(big.Int).SetString("10", 0)
exp10Pow256 := new(big.Int).Exp(pos10, exp, nil)
blob, err := exp10Pow256.MarshalText()
if err != nil {
t.Fatal(err)
}

ui := new(sdkmath.Uint)
err = ui.Unmarshal(blob)
if err == nil {
t.Fatal("Failed to catch the overflowed value")
}
if errStr := err.Error(); !strings.Contains(errStr, "out of range") {
t.Fatalf("out of range value not reported, got instead %q", errStr)
}
}

0 comments on commit 54d764b

Please sign in to comment.