🛡️ Sentinel: [CRITICAL] Fix integer overflow in rate limiter

.jules/sentinel.md

@@ -0,0 +1,4 @@
+## 2024-05-23 - Integer Overflow in Rate Limiter
+**Vulnerability:** Token bucket algorithm used standard integer arithmetic for token refill calculations. High refill rates or long idle periods caused `periodsElapsed * refillTokens` to overflow `long`, resulting in negative values. `Math.min` picked the negative value, halting refills (DoS). Additionally, `current + tokensToAdd` could overflow `int` if capacity was near `MAX_VALUE`, locking the bucket.
+**Learning:** Rate limiters operating with nanosecond precision and potentially large capacities are susceptible to integer overflows even with `long`. Standard arithmetic operators do not check for overflow.
+**Prevention:** Use explicit overflow checks (e.g., `Math.multiplyExact` or logic checks like `a > limit / b`) for token calculations. Perform intermediate accumulation in `long` and clamp to capacity before casting to `int`.

src/main/java/io/github/spaceleam/cadence/core/TokenBucketLimiter.java

@@ -185,13 +185,21 @@ private void refill() {
 
         if (timeSinceLastRefill >= refillPeriodNanos) {
             long periodsElapsed = timeSinceLastRefill / refillPeriodNanos;
-            int tokensToAdd = (int) Math.min(periodsElapsed * refillTokens, capacity);
+            int tokensToAdd;
+
+            // Check for potential long overflow in periodsElapsed * refillTokens
+            // If periodsElapsed > capacity / refillTokens, the result will exceed capacity anyway
+            if (periodsElapsed > capacity / refillTokens) {
+                tokensToAdd = capacity;
+            } else {
+                tokensToAdd = (int) Math.min(periodsElapsed * refillTokens, capacity);
+            }
 
             if (tokensToAdd > 0) {
                 long newRefillTime = lastRefill + (periodsElapsed * refillPeriodNanos);
 
                 if (lastRefillTime.compareAndSet(lastRefill, newRefillTime)) {
-                    int newTotal = availableTokens.updateAndGet(current -> Math.min(capacity, current + tokensToAdd));
+                    int newTotal = availableTokens.updateAndGet(current -> (int) Math.min(capacity, (long) current + tokensToAdd));
                     if (listener != null) {
                         listener.onRefill(tokensToAdd, newTotal);
                     }

src/test/java/io/github/spaceleam/cadence/OverflowTest.java

@@ -0,0 +1,97 @@
+package io.github.spaceleam.cadence;
+
+import io.github.spaceleam.cadence.core.RateLimiter;
+import io.github.spaceleam.cadence.core.TokenBucketLimiter;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+import java.lang.reflect.Field;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicLong;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+class OverflowTest {
+
+    @Test
+    @DisplayName("Reproduce Integer Overflow in availableTokens")
+    void testAvailableTokensOverflow() throws Exception {
+        // Create a limiter with capacity = Integer.MAX_VALUE
+        RateLimiter limiter = Cadence.builder()
+                .capacity(Integer.MAX_VALUE)
+                .refillRate(10, TimeUnit.MILLISECONDS) // Refill 10 tokens every 10ms
+                .build();
+
+        // 1. Consume some tokens to make room for refill
+        limiter.tryAcquire(100);
+
+        // availableTokens should be Integer.MAX_VALUE - 100
+        assertEquals(Integer.MAX_VALUE - 100, limiter.getAvailableTokens());
+
+        // 2. Manipulate lastRefillTime to simulate time passing, forcing a refill
+        // We want tokensToAdd to be at least 200, so current + tokensToAdd > Integer.MAX_VALUE
+
+        // We need to access private fields to manipulate state directly for deterministic testing
+        Field availableTokensField = TokenBucketLimiter.class.getDeclaredField("availableTokens");
+        availableTokensField.setAccessible(true);
+        AtomicInteger availableTokens = (AtomicInteger) availableTokensField.get(limiter);
+
+        // Set available tokens to close to MAX_VALUE
+        availableTokens.set(Integer.MAX_VALUE - 10);
+
+        Field lastRefillTimeField = TokenBucketLimiter.class.getDeclaredField("lastRefillTime");
+        lastRefillTimeField.setAccessible(true);
+        AtomicLong lastRefillTime = (AtomicLong) lastRefillTimeField.get(limiter);
+
+        // Set last refill time to be 1 second ago (100 * 10ms periods)
+        // Refill is 10 tokens per period. So 100 periods * 10 tokens = 1000 tokens to add.
+        // current (MAX - 10) + tokensToAdd (1000) = Overflow!
+        long oneSecondAgo = System.nanoTime() - TimeUnit.SECONDS.toNanos(1);
+        lastRefillTime.set(oneSecondAgo);
+
+        // 3. Trigger refill by calling getAvailableTokens() or tryAcquire()
+        int available = limiter.getAvailableTokens();
+
+        // If bug exists, available will be negative due to overflow
+        assertTrue(available > 0, "Available tokens should be positive, got: " + available);
+        assertEquals(Integer.MAX_VALUE, available, "Should be capped at Integer.MAX_VALUE");
+    }
+
+    @Test
+    @DisplayName("Reproduce Long Overflow in periodsElapsed * refillTokens")
+    void testRefillCalculationOverflow() throws Exception {
+         // Create a limiter
+        RateLimiter limiter = Cadence.builder()
+                .capacity(100)
+                .refillRate(Integer.MAX_VALUE, TimeUnit.NANOSECONDS) // Huge refill rate
+                .build();
+
+        // Manipulate lastRefillTime to be long ago
+        Field lastRefillTimeField = TokenBucketLimiter.class.getDeclaredField("lastRefillTime");
+        lastRefillTimeField.setAccessible(true);
+        AtomicLong lastRefillTime = (AtomicLong) lastRefillTimeField.get(limiter);
+
+        // Set last refill to 5 seconds ago
+        // periodsElapsed = 5e9
+        // refillTokens = 2e9
+        // product = 10e18 > Long.MAX_VALUE (9e18) -> Overflow to negative
+        long fiveSecondsAgo = System.nanoTime() - TimeUnit.SECONDS.toNanos(5);
+        lastRefillTime.set(fiveSecondsAgo);
+
+        // Drain tokens first so refill is needed
+        // Note: Because refillRate is massive (MAX_INT per ns), any call to tryAcquire/getAvailableTokens
+        // will trigger a refill if time has passed.
+        // If the overflow fix works, the refill happens correctly.
+        // If the bug exists, the calculation overflows, refill is skipped, and available tokens stay low.
+
+        limiter.tryAcquire(100);
+
+        // Trigger refill
+        int available = limiter.getAvailableTokens();
+
+        // If overflow happens, tokensToAdd becomes negative, refill skipped, available remains 0.
+        // If fixed, it refills to capacity (100).
+        assertEquals(100, available, "Should have refilled to capacity");
+    }
+}