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

x/gov/client/cli: integer overflow from ignoring output of strconv.Atoi #13346

Closed
odeke-em opened this issue Sep 20, 2022 · 0 comments · Fixed by #13347
Closed

x/gov/client/cli: integer overflow from ignoring output of strconv.Atoi #13346

odeke-em opened this issue Sep 20, 2022 · 0 comments · Fixed by #13347
Assignees
Labels
S:orijtech Squad: OrijTech

Comments

@odeke-em
Copy link
Collaborator

Summary of Bug

Reported by cosmos/gosec in https://github.com/cosmos/cosmos-sdk/security/code-scanning/5730; on further inspection it does indeed turn out to be a bug as an overflow per this repro

package cli_test

import (
	"io"
	"strings"
	"testing"

	"github.com/chzyer/readline"
	"github.com/stretchr/testify/assert"

	"github.com/cosmos/cosmos-sdk/x/gov/client/cli"
)

type st struct {
	ToOverflow int
}

func TestPromptOverflow(t *testing.T) {
	origStdin := readline.Stdin
	defer func() {
		// Restore what .Stdin was before.
		readline.Stdin = origStdin
	}()
	// Intentionally sending a value out of the range of
	intOverflowers := []string{
		"-9223372036854775809",
		"9223372036854775808",
		"9923372036854775809",
		"-9923372036854775809",
		"18446744073709551616",
		"-18446744073709551616",
	}

	for _, intOverflower := range intOverflowers {
		overflowStr := intOverflower
		t.Run(overflowStr, func(t *testing.T) {
			sr := strings.NewReader(overflowStr + "\n")
			readline.Stdin = io.NopCloser(sr)

			v, err := cli.Prompt(st{}, "")
			assert.NotNil(t, err, "expected a report of an overflow")
			assert.Equal(t, st{}, v, "expected a value of zero")
		})
	}
}

Version

79f277c

Fix

diff --git a/x/gov/client/cli/prompt.go b/x/gov/client/cli/prompt.go
index b997f5a4a..faf7d352e 100644
--- a/x/gov/client/cli/prompt.go
+++ b/x/gov/client/cli/prompt.go
@@ -94,7 +94,16 @@ func Prompt[T any](data T, namePrefix string) (T, error) {
 		case reflect.String:
 			v.Field(i).SetString(result)
 		case reflect.Int:
-			resultInt, _ := strconv.Atoi(result)
+			resultInt, err := strconv.Atoi(result)
+			if err != nil {
+				return data, fmt.Errorf("invalid value for int: %w", err)
+			}
+			// If a value was successfully parsed the ranges of:
+			//      [minInt,     maxInt]
+			// are within the ranges of:
+			//      [minInt64, maxInt64]
+			// of which on 64-bit machines, which are most common,
+			// int==int64
 			v.Field(i).SetInt(int64(resultInt))
 		default:
 			// skip other types
@odeke-em odeke-em added the S:orijtech Squad: OrijTech label Sep 20, 2022
@odeke-em odeke-em self-assigned this Sep 20, 2022
odeke-em added a commit that referenced this issue Sep 20, 2022
…alues

Reported by cosmos/gosec in https://github.com/cosmos/cosmos-sdk/security/code-scanning/5730.
This change checks for errors from strconv.Atoi in which case we were
susceptible to out of range errors, this change also adds tests to
prevent regressions.

Fixes #13346
odeke-em added a commit that referenced this issue Sep 20, 2022
…alues (#13347)

Reported by cosmos/gosec in https://github.com/cosmos/cosmos-sdk/security/code-scanning/5730.
This change checks for errors from strconv.Atoi in which case we were
susceptible to out of range errors, this change also adds tests to
prevent regressions.

Fixes #13346
odeke-em added a commit that referenced this issue Sep 20, 2022
…alues

Reported by cosmos/gosec in https://github.com/cosmos/cosmos-sdk/security/code-scanning/5730.
This change checks for errors from strconv.Atoi in which case we were
susceptible to out of range errors, this change also adds tests to
prevent regressions.

Fixes #13346
odeke-em added a commit that referenced this issue Sep 21, 2022
…Parse tests

Uses strconv.ParseInt(s, 10, 0) where 0 as the bitSize lets
the system determine the bitSize to parse int into its range.
Adds more rigorous checks to ensure that the output
is as expected with range failures. While here also
added control tests to ensure that we can parse integers
within range of int.

Updates #13346
odeke-em added a commit that referenced this issue Sep 21, 2022
Adds more rigorous checks to ensure that the output
is as expected with range failures. While here also
added control tests to ensure that we can parse integers
within range of int.

Updates #13346
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S:orijtech Squad: OrijTech
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant