Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 48 additions & 27 deletions sql/analyzer/analyzer.go
Original file line number Diff line number Diff line change
Expand Up @@ -294,63 +294,74 @@ func (a *Analyzer) Log(msg string, args ...interface{}) {
if a != nil && a.Debug {
if len(a.contextStack) > 0 {
ctx := strings.Join(a.contextStack, "/")
log.Infof("%s: "+msg, append([]interface{}{ctx}, sanitizeArguments(args)...)...)
sanitizedArgs := sanitizeArguments(args)
log.Infof("%s: "+msg, append([]interface{}{ctx}, sanitizedArgs...)...)

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

Copilot Autofix

AI 4 months ago

The best way to fix this problem is to ensure that sensitive information, such as passwords, is never logged, even in debug mode. This can be achieved by strengthening the sanitizeArguments and containsSensitiveData functions to more reliably detect and redact sensitive fields, especially for known node types like plan.CreateUser and any structure that may contain password fields. Specifically, we should:

  • Update sanitizeArguments to explicitly handle plan.CreateUser and similar types, redacting any password fields within them.
  • Ensure that any slice or struct containing password fields is recursively sanitized.
  • Optionally, add or update tests for the sanitizer to ensure coverage for all relevant node types.
  • All changes should be made in sql/analyzer/analyzer.go, within the sanitizeArguments and sanitizeStruct functions.

No changes are needed in engine.go or sql/parse/parse.go, as the fix is best localized to the logging/sanitization logic.


Suggested changeset 1
sql/analyzer/analyzer.go

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/sql/analyzer/analyzer.go b/sql/analyzer/analyzer.go
--- a/sql/analyzer/analyzer.go
+++ b/sql/analyzer/analyzer.go
@@ -326,2 +326,6 @@
 			args[i] = "[PASSWORD_REDACTED]"
+		case *plan.CreateUser:
+			args[i] = sanitizeCreateUser(v)
+		case plan.CreateUser:
+			args[i] = sanitizeCreateUser(&v)
 		default:
@@ -367,2 +371,28 @@
 }
+
+// sanitizeCreateUser redacts password fields in plan.CreateUser nodes.
+func sanitizeCreateUser(cu *plan.CreateUser) interface{} {
+	if cu == nil {
+		return nil
+	}
+	// Create a shallow copy
+	safe := *cu
+	// Redact passwords in Users slice
+	users := make([]plan.AuthenticatedUser, len(cu.Users))
+	for i, u := range cu.Users {
+		userCopy := u
+		// Redact Auth1 if it's a password type
+		switch auth := userCopy.Auth1.(type) {
+		case plan.AuthenticationMysqlNativePassword:
+			userCopy.Auth1 = "[PASSWORD_REDACTED]"
+		case plan.OtherAuthentication:
+			userCopy.Auth1 = "[PASSWORD_REDACTED]"
+		case plan.DefaultAuthentication:
+			userCopy.Auth1 = "[PASSWORD_REDACTED]"
+		}
+		users[i] = userCopy
+	}
+	safe.Users = users
+	return safe
+}
 
EOF
@@ -326,2 +326,6 @@
args[i] = "[PASSWORD_REDACTED]"
case *plan.CreateUser:
args[i] = sanitizeCreateUser(v)
case plan.CreateUser:
args[i] = sanitizeCreateUser(&v)
default:
@@ -367,2 +371,28 @@
}

// sanitizeCreateUser redacts password fields in plan.CreateUser nodes.
func sanitizeCreateUser(cu *plan.CreateUser) interface{} {
if cu == nil {
return nil
}
// Create a shallow copy
safe := *cu
// Redact passwords in Users slice
users := make([]plan.AuthenticatedUser, len(cu.Users))
for i, u := range cu.Users {
userCopy := u
// Redact Auth1 if it's a password type
switch auth := userCopy.Auth1.(type) {
case plan.AuthenticationMysqlNativePassword:
userCopy.Auth1 = "[PASSWORD_REDACTED]"
case plan.OtherAuthentication:
userCopy.Auth1 = "[PASSWORD_REDACTED]"
case plan.DefaultAuthentication:
userCopy.Auth1 = "[PASSWORD_REDACTED]"
}
users[i] = userCopy
}
safe.Users = users
return safe
}

Copilot is powered by AI and may make mistakes. Always verify output.
Unable to commit as this autofix suggestion is now outdated
} else {
log.Infof(msg, sanitizeArguments(args)...)
sanitizedArgs := sanitizeArguments(args)
log.Infof(msg, sanitizedArgs...)
}
}
}

func sanitizeArguments(args []interface{}) []interface{} {
sanitizedArgs := make([]interface{}, len(args))
for i, arg := range args {
switch v := arg.(type) {
case string:
if isSensitiveString(v) {
args[i] = "[REDACTED]"
sanitizedArgs[i] = "[REDACTED]"
} else {
sanitizedArgs[i] = v
}
case map[string]interface{}:
args[i] = sanitizeMap(v)
sanitizedArgs[i] = sanitizeMap(v)
case []interface{}:
args[i] = sanitizeArguments(v)
sanitizedArgs[i] = sanitizeArguments(v)
case plan.AuthenticationMysqlNativePassword:
args[i] = "[PASSWORD_REDACTED]"
sanitizedArgs[i] = "[PASSWORD_REDACTED]"
default:
// Use reflection to handle structs and pointers to structs
rv := reflect.ValueOf(arg)
if rv.Kind() == reflect.Ptr {
rv = rv.Elem()
if rv.Kind() == reflect.Struct {
args[i] = sanitizeStruct(rv)
sanitizedArgs[i] = sanitizeStruct(rv)
continue
}
}
if rv.Kind() == reflect.Struct {
args[i] = sanitizeStruct(rv)
sanitizedArgs[i] = sanitizeStruct(rv)
} else if rv.Kind() == reflect.Slice {
// Recursively sanitize slice elements
slice := make([]interface{}, rv.Len())
for j := 0; j < rv.Len(); j++ {
slice[j] = sanitizeArguments([]interface{}{rv.Index(j).Interface()})[0]
}
args[i] = slice
sanitizedArgs[i] = slice
} else if rv.Kind() == reflect.Map {
// Recursively sanitize map values
mapSanitized := make(map[interface{}]interface{})
for _, key := range rv.MapKeys() {
val := rv.MapIndex(key).Interface()
if isSensitiveString(fmt.Sprintf("%v", key.Interface())) || isSensitive(val) {
keyStr := fmt.Sprintf("%v", key.Interface())
if isSensitiveString(keyStr) || isSensitive(val) {
mapSanitized[key.Interface()] = "[REDACTED]"
} else {
mapSanitized[key.Interface()] = sanitizeArguments([]interface{}{val})[0]
}
}
args[i] = mapSanitized
sanitizedArgs[i] = mapSanitized
} else {
args[i] = "[REDACTED]"
// Catch-all for unhandled types
if strVal := fmt.Sprintf("%v", arg); isSensitiveString(strVal) {
sanitizedArgs[i] = "[REDACTED]"
} else {
sanitizedArgs[i] = arg
}
}
}
}
return args
return sanitizedArgs
}

func sanitizeStruct(rv reflect.Value) interface{} {
Expand Down Expand Up @@ -380,7 +391,8 @@ func sanitizeStruct(rv reflect.Value) interface{} {
mapSanitized := make(map[interface{}]interface{})
for _, key := range rv.Field(i).MapKeys() {
val := rv.Field(i).MapIndex(key).Interface()
if isSensitiveString(fmt.Sprintf("%v", key.Interface())) || isSensitive(val) {
keyStr := fmt.Sprintf("%v", key.Interface())
if isSensitiveString(keyStr) || isSensitive(val) {
mapSanitized[key.Interface()] = "[REDACTED]"
} else {
mapSanitized[key.Interface()] = sanitizeArguments([]interface{}{val})[0]
Expand All @@ -396,9 +408,10 @@ func sanitizeStruct(rv reflect.Value) interface{} {
}

func sanitizeMap(m map[string]interface{}) map[string]interface{} {
result := make(map[string]interface{})
for key, value := range m {
if isSensitiveString(key) || isSensitive(value) {
m[key] = "[REDACTED]"
result[key] = "[REDACTED]"
} else {
rv := reflect.ValueOf(value)
switch rv.Kind() {
Expand All @@ -407,31 +420,33 @@ func sanitizeMap(m map[string]interface{}) map[string]interface{} {
subMap := make(map[interface{}]interface{})
for _, subKey := range rv.MapKeys() {
val := rv.MapIndex(subKey).Interface()
if isSensitiveString(fmt.Sprintf("%v", subKey.Interface())) || isSensitive(val) {
keyStr := fmt.Sprintf("%v", subKey.Interface())
if isSensitiveString(keyStr) || isSensitive(val) {
subMap[subKey.Interface()] = "[REDACTED]"
} else {
subMap[subKey.Interface()] = sanitizeArguments([]interface{}{val})[0]
}
}
m[key] = subMap
result[key] = subMap
case reflect.Slice:
slice := make([]interface{}, rv.Len())
for j := 0; j < rv.Len(); j++ {
slice[j] = sanitizeArguments([]interface{}{rv.Index(j).Interface()})[0]
}
m[key] = slice
result[key] = slice
case reflect.Struct:
m[key] = sanitizeStruct(rv)
result[key] = sanitizeStruct(rv)
default:
m[key] = value
result[key] = value
}
}
}
return m
return result
}

// isSensitiveString checks if a string contains sensitive information
func isSensitiveString(str string) bool {
sensitiveKeywords := []string{"password", "secret", "token", "key"}
sensitiveKeywords := []string{"password", "secret", "token", "key", "auth", "credential", "private"}
str = strings.ToLower(str)
for _, keyword := range sensitiveKeywords {
if strings.Contains(str, keyword) {
Expand All @@ -441,13 +456,19 @@ func isSensitiveString(str string) bool {
return false
}

// isSensitive checks if a value contains sensitive information
func isSensitive(arg interface{}) bool {
// Add logic to identify sensitive data (e.g., passwords)
// This may involve checking types or specific fields
if str, ok := arg.(string); ok && strings.Contains(strings.ToLower(str), "password") {
return true
if arg == nil {
return false
}
return false

// Check strings directly
if str, ok := arg.(string); ok {
return isSensitiveString(str)
}

// Check string representation
return isSensitiveString(fmt.Sprintf("%v", arg))
}

// LogNode prints the node given if Verbose logging is enabled.
Expand Down
14 changes: 6 additions & 8 deletions sql/system_settype.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,15 +90,13 @@ func (t systemSetType) Convert(v interface{}) (interface{}, error) {
case float64:
// Float values aren't truly accepted, but the engine will give them when it should give ints.
// Therefore, if the float doesn't have a fractional portion, we treat it as an int.
if value >= float64(math.MinInt64) && value <= float64(math.MaxInt64) {
if math.Trunc(value) == value { // Ensure no fractional part exists
// Additional bounds check for out-of-range values
if value < float64(math.MinInt64) || value > float64(math.MaxInt64) {
return nil, ErrInvalidSystemVariableValue.New(t.varName, v) // Reject out-of-range values
}
intValue := int64(value)
return t.SetType.Convert(intValue)
if math.Trunc(value) == value { // Ensure no fractional part exists
// Explicit bounds check for int64 range
if value < float64(math.MinInt64) || value > float64(math.MaxInt64) {
return nil, ErrInvalidSystemVariableValue.New(t.varName, v) // Reject out-of-range values
}
intValue := int64(value)

Check failure

Code scanning / CodeQL

Incorrect conversion between integer types High

Incorrect conversion of an unsigned 64-bit integer from strconv.ParseUint to a lower bit size type int64 without an upper bound check.
Incorrect conversion of an unsigned 64-bit integer from strconv.ParseUint to a lower bit size type int64 without an upper bound check.
Incorrect conversion of an unsigned 64-bit integer from strconv.ParseUint to a lower bit size type int64 without an upper bound check.

Copilot Autofix

AI 4 months ago

To fix this issue, we need to ensure that when converting a float64 to int64, the value did not originate as a uint64 that could be outside the int64 range. The best way to do this is to add an explicit check: if the value is outside the range of int64 (i.e., less than math.MinInt64 or greater than math.MaxInt64), the conversion should be rejected. This is already done in the code. However, the taint flow suggests that a float64 value could have originated from a uint64 that is greater than math.MaxInt64, and thus would be incorrectly converted to a negative int64. To address this, we should also check that the value is not greater than math.MaxInt64 before converting to int64. If the value is outside the int64 range, we should return an error.

Additionally, if the value originated as a uint64, we should avoid converting it to int64 altogether. Instead, we should handle uint64 values separately and ensure that they are not incorrectly converted to int64. This can be done by adding a case for uint64 in the type switch and handling it appropriately.

Summary of changes:

  • In sql/system_settype.go, in the Convert method, add a check to ensure that when converting from float64 to int64, the value is within the int64 range.
  • If the value is outside the int64 range, return an error.
  • Ensure that uint64 values are not converted to int64 without proper bounds checking.

Suggested changeset 1
sql/system_settype.go

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/sql/system_settype.go b/sql/system_settype.go
--- a/sql/system_settype.go
+++ b/sql/system_settype.go
@@ -97,2 +97,6 @@
 			}
+			// Additional check: if value originated as uint64 and is > MaxInt64, reject
+			if value > float64(math.MaxInt64) {
+				return nil, ErrInvalidSystemVariableValue.New(t.varName, v) // Reject out-of-range values
+			}
 			intValue := int64(value)
EOF
@@ -97,2 +97,6 @@
}
// Additional check: if value originated as uint64 and is > MaxInt64, reject
if value > float64(math.MaxInt64) {
return nil, ErrInvalidSystemVariableValue.New(t.varName, v) // Reject out-of-range values
}
intValue := int64(value)
Copilot is powered by AI and may make mistakes. Always verify output.
return t.SetType.Convert(intValue)
}
return nil, ErrInvalidSystemVariableValue.New(t.varName, v) // Reject out-of-range values
case decimal.Decimal:
Expand Down
3 changes: 3 additions & 0 deletions sql/timetype.go
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,9 @@ func (t timespanType) ConvertToTimespan(v interface{}) (Timespan, error) {
case float32:
return t.ConvertToTimespan(float64(value))
case float64:
if value < float64(math.MinInt64) || value > float64(math.MaxInt64) {
return Timespan(0), fmt.Errorf("float64 value %f exceeds int64 bounds", value)
}
intValue := int64(value)
microseconds := int64Abs(int64(math.Round((value - float64(intValue)) * float64(microsecondsPerSecond))))
absValue := int64Abs(intValue)
Expand Down
Loading