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.

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

all: use of cosmossdk.io/math.ParseUint then cast to uint32 is unnecessary and susceptible to uint64->uint32 underflows which could be a security issue #292

Open
odeke-em opened this issue Mar 13, 2024 · 2 comments · May be fixed by #293
Open

all: use of cosmossdk.io/math.ParseUint then cast to uint32 is unnecessary and susceptible to uint64->uint32 underflows which could be a security issue #292

odeke-em opened this issue Mar 13, 2024 · 2 comments · May be fixed by #293

Comments

@odeke-em
Copy link

odeke-em commented Mar 13, 2024 •
edited
Loading

Summary of Bug

Noticed while auditing the code in x/auction/client/cli that there is this code

sommelier/x/auction/client/cli/query.go

Lines 333 to 341 in 3157d97

auctionID, err := math.ParseUint(args[0])
if err != nil {
return err
}
queryClient := types.NewQueryClient(ctx)
req := &types.QueryBidsByAuctionRequest{
AuctionId: uint32(auctionID.Uint64()),
}

in which firstly cosmossdk.io/math.ParseUint is invoked and that returns a Uint object whose underlying structure uses a math/big.Int with arbitrary precision.

Down below to send in a uint32 into the query, we invoke uint32(u.Uint64()) of which this code will cause an underflow for example if I pass in any value greater than uint32 which can actually help me with randomization to get random bids and other values and probably even clog y'all network if there are no gas fees to even make these queries

Reproduction

package repro_test

import (
	"testing"

	"cosmossdk.io/math"
)

func TestUnderflowExhibit(t *testing.T) {
	s := "4300000000"
	auctionID, err := math.ParseUint(s)
	if err != nil {
		t.Fatal(err)
	}

	dd := uint32(auctionID.Uint64())
	if g, w := uint64(dd), auctionID.Uint64(); g != w {
		t.Fatalf("Reflexive failure uint64(uint32): got %d want %d", g, w)
	}
}

Remedy

You can trivially use strconv.ParseUint(args[0], 10, 32) which is so much simpler and will directly help all these sites and catch range errors

Location

There are 11 sites where this code pattern is used and can be fixed by extracting the fix into a helper function called parseUint32

Kindly /cc-ing @zmanian @fedekunze @jackzampolin
odeke-em added a commit to orijtech/sommelier that referenced this issue Mar 13, 2024
@odeke-em
fix(x/auction/client/cli): fix uint64->uint32 underflow issues
a882442 
This fixes a potential security issue resulting from extraneous
parsing that used cosmossdk.math.ParseUint which uses math/big.Int
which is a big integer package and can allow uint64 in places
where uint32 is used and there were underflow checks.
The fix for this change was to simply use

    strconv.ParseUint(s, 10, BIT_SIZE)

where BIT_SIZE is any of 32 or 64 bits for uint32 and uint64 respectively.

Fixes PeggyJV#292
@odeke-em odeke-em linked a pull request Mar 13, 2024 that will close this issue
Open
@odeke-em
Copy link
Author

I've mailed out PR #293

odeke-em added a commit to orijtech/sommelier that referenced this issue Mar 13, 2024
@odeke-em
fix(x/auction/client/cli): fix uint64->uint32 underflow issues
af08855 
This fixes a potential security issue resulting from extraneous
parsing that used cosmossdk.math.ParseUint which uses math/big.Int
which is a big integer package and can allow uint64 in places
where uint32 is used and there were underflow checks.
The fix for this change was to simply use

    strconv.ParseUint(s, 10, BIT_SIZE)

where BIT_SIZE is any of 32 or 64 bits for uint32 and uint64 respectively.

Fixes PeggyJV#292
odeke-em added a commit to orijtech/sommelier that referenced this issue Mar 13, 2024
@odeke-em
fix: fix uint64->uint32 underflow issues resulting from using cosmoss…
1cd0de7 
…dk.io/math

This fixes potential security issues resulting from extraneous
parsing that used cosmossdk.math.ParseUint which uses math/big.Int
which is a big integer package and can allow uint64 in places
where uint32 is used and there were underflow checks.
The fix for this change was to simply use

    strconv.ParseUint(s, 10, BIT_SIZE)

where BIT_SIZE is any of 32 or 64 bits for uint32 and uint64 respectively,
and by extracting the shared code to an internal package.

Fixes PeggyJV#292
@odeke-em
Copy link
Author

Turns out that there were even many more instances of the erroneous code in a bunch of packages so I've updated the PR #293.

odeke-em added a commit to orijtech/sommelier that referenced this issue Mar 13, 2024
@odeke-em
fix: fix uint64->uint32 underflow issues resulting from using cosmoss…
3425674 
…dk.io/math

This fixes potential security issues resulting from extraneous
parsing that used cosmossdk.math.ParseUint which uses math/big.Int
which is a big integer package and can allow uint64 in places
where uint32 is used and there were underflow checks.
The fix for this change was to simply use

    strconv.ParseUint(s, 10, BIT_SIZE)

where BIT_SIZE is any of 32 or 64 bits for uint32 and uint64 respectively,
and by extracting the shared code to an internal package.

Fixes PeggyJV#292
odeke-em added a commit to orijtech/sommelier that referenced this issue Mar 13, 2024
@odeke-em
fix: fix uint64->uint32 underflow issues resulting from using cosmoss…
9f66b41 
…dk.io/math

This fixes potential security issues resulting from extraneous
parsing that used cosmossdk.math.ParseUint which uses math/big.Int
which is a big integer package and can allow uint64 in places
where uint32 is used and there were underflow checks.
The fix for this change was to simply use

    strconv.ParseUint(s, 10, BIT_SIZE)

where BIT_SIZE is any of 32 or 64 bits for uint32 and uint64 respectively,
and by extracting the shared code to an internal package.

Fixes PeggyJV#292
@odeke-em odeke-em changed the title x/auction/client/cli: use of cosmossdk.io/math.ParseUint then cast to uint32 is unnecessary and susceptible to uint64->uint32 underflows which could be a security issue all: use of cosmossdk.io/math.ParseUint then cast to uint32 is unnecessary and susceptible to uint64->uint32 underflows which could be a security issue Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: fix uint64->uint32 underflow issues resulting from using cosmossdk.io/math orijtech/sommelier
1 participant
@odeke-em