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

[security] Downcasting of 64-bit integer #820

Closed
duglin opened this issue Nov 8, 2022 · 0 comments · Fixed by #824
Closed

[security] Downcasting of 64-bit integer #820

duglin opened this issue Nov 8, 2022 · 0 comments · Fixed by #824

Comments

@duglin
Copy link
Contributor

duglin commented Nov 8, 2022

Description

The strconv.Atoi function parses an int: a machine dependent integer type that will be int64 for 64-bit targets. There are places throughout the codebase where the result returned from strconv.Atoi is later converted to a smaller type: int16 or int32. This may overflow with a certain input.

func (v *expressionVisitor) VisitIntegerLiteral(ctx*gen.IntegerLiteralContext) interface{} {
  val, err := strconv.Atoi(ctx.GetText())
  if err != nil {
    v.parsingErrors = append(v.parsingErrors, err)
  }
  return expression.NewLiteralExpression(int32(val))
}

code

case cesql.IntegerType:
      switch val.(type) {
      case string:
             v, err := strconv.Atoi(val.(string))
             if err != nil {
                    err = fmt.Errorf("cannot cast from String to Integer: %w", err)
             }
             return int32(v), err
      }

code

Exploit Scenario

A value is parsed from a configuration file with Atoi, resulting in an integer. It is then downcasted to a lower precision value, resulting in a potential overflow or underflow that is not handled by the Golang compiler an error or panic.

Recommendations

Short term, when parsing strings into fixed-width integer types, use strconv.ParseInt or strconv.ParseUint with an appropriate bitSize argument instead of strconv.Atoi.
Long term, use open-source automated static-analysis tools such as Semgrep as part of the development process to check for common vulnerabilities in the code.

This was opened due to the Trail of Bits security review

duglin added a commit to duglin/sdk-go that referenced this issue Dec 8, 2022
See: cloudevents#820
Closes: cloudevents#820

Signed-off-by: Doug Davis <dug@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant