Security: Fix G115 integer overflow conversion warnings (131 instances)

[jose-compu]

Summary

The codebase contains 131 instances of G115 (integer overflow conversion) warnings identified by gosec. These are currently excluded from CI checks but should be systematically addressed to improve security.

Current Status (or Soon)

• Excluded from CI: G115 rule is currently excluded in .github/workflows/client.yml
• Impact: 131 warnings across the codebase
• Severity: Medium-High (potential integer overflow vulnerabilities)

Affected Areas

The warnings are primarily found in:

• Protocol marshaling code (pkg/beacon/, pkg/tecdsa/, pkg/tbtc/)
• Ethereum chain interactions (pkg/chain/ethereum/)
• Bitcoin-related code (pkg/bitcoin/, pkg/tbtcpg/)
• Member index conversions (uint32/uint64 → uint8)
• Block number conversions (uint64 → int64)

Common Patterns to Fix

1. uint64 → uint8 conversions

// Current (unsafe):
memberIndex := group.MemberIndex(result.SubmitterMemberIndex.Uint64())

// Fixed (with bounds checking):
if result.SubmitterMemberIndex.Uint64() > 255 {
    return fmt.Errorf("member index overflow: %d", result.SubmitterMemberIndex.Uint64())
}
memberIndex := group.MemberIndex(uint8(result.SubmitterMemberIndex.Uint64()))

2. uint64 → int64 conversions

// Current (unsafe):
attemptSeed := int64(binary.BigEndian.Uint64(messageSha256[:8]))

// Fixed (with bounds checking):
rawSeed := binary.BigEndian.Uint64(messageSha256[:8])
if rawSeed > math.MaxInt64 {
    return fmt.Errorf("seed overflow: %d", rawSeed)
}
attemptSeed := int64(rawSeed)

3. uint32 → uint8 conversions

// Current (unsafe):
senderID := group.MemberIndex(pbMsg.SenderID)

// Fixed (with bounds checking):
if pbMsg.SenderID > 255 {
    return fmt.Errorf("sender ID overflow: %d", pbMsg.SenderID)
}
senderID := group.MemberIndex(uint8(pbMsg.SenderID))

Implementation Strategy

Phase 1: Critical Paths (High Priority)

1. Authentication/Signing code - pkg/tecdsa/signing/
2. DKG operations - pkg/beacon/dkg/, pkg/tecdsa/dkg/
3. Member index handling - All protocol marshaling

Phase 2: Chain Interactions (Medium Priority)

1. Ethereum chain calls - pkg/chain/ethereum/
2. Bitcoin operations - pkg/bitcoin/, pkg/tbtcpg/
3. Block number handling

Phase 3: Protocol Code (Lower Priority)

1. General marshaling - Remaining protocol code
2. Test utilities - Internal test packages

Acceptance Criteria

• All G115 warnings resolved
• Bounds checking added for all integer conversions
• Proper error handling for overflow conditions
• Tests updated to cover overflow scenarios
• CI updated to remove G115 exclusion
• Documentation updated with overflow handling patterns

Testing Requirements

• Unit tests for overflow conditions
• Integration tests with large values
• Performance impact assessment
• Backward compatibility verification

Files to Update

Key files requiring fixes:

• pkg/tecdsa/signing/marshaling.go (25+ instances)
• pkg/beacon/gjkr/marshaling.go (15+ instances)
• pkg/chain/ethereum/tbtc.go (10+ instances)
• pkg/tbtc/marshaling.go (8+ instances)
• pkg/tecdsa/dkg/marshaling.go (8+ instances)
• And 50+ additional files

Notes

• Most conversions are in protocol marshaling where values are controlled
• Some conversions may be safe due to domain constraints (e.g., member indices < 256)
• Consider adding custom linter rules for specific safe patterns
• Document any intentionally unsafe conversions with //nolint:gosec comments

Related

• Current CI exclusion: .github/workflows/client.yml line ~245
• Gosec rule: G115 - Type conversion which leads to integer overflow