diff --git a/api/client.go b/api/client.go index 4d33ee5cf..ad7d5b351 100644 --- a/api/client.go +++ b/api/client.go @@ -315,7 +315,7 @@ func (a *ClientAPI) ElevateClient(ctx *gin.Context) { return } - elevatedUntil := time.Now().Add(time.Duration(params.DurationSeconds) * time.Second) + elevatedUntil := time.Now().Add(model.ElevationDuration(params.DurationSeconds)) if err := a.DB.UpdateClientElevatedUntil(client.ID, &elevatedUntil); err != nil { ctx.AbortWithError(500, err) return diff --git a/api/client_test.go b/api/client_test.go index 7c9ae60a1..d28861def 100644 --- a/api/client_test.go +++ b/api/client_test.go @@ -343,6 +343,26 @@ func (s *ClientSuite) Test_ElevateClient_expectBadRequestOnMissingDuration() { assert.Nil(s.T(), client.ElevatedUntil) } +func (s *ClientSuite) Test_ElevateClient_clampsHugeDuration() { + s.db.User(5).Client(8) + + test.WithUser(s.ctx, 5) + // A value that would overflow time.Duration must be clamped to the ceiling + // (not rejected, and not wrapped to a past timestamp). + s.ctx.AddParam("id", "8") + s.withElevateBody(`{"durationSeconds":9223372036854775807}`) + + before := time.Now() + s.a.ElevateClient(s.ctx) + + assert.Equal(s.T(), 204, s.ctx.Writer.Status()) + client, err := s.db.GetClientByID(8) + assert.NoError(s.T(), err) + assert.NotNil(s.T(), client.ElevatedUntil) + // Must be in the future (overflow would have produced a past time). + assert.True(s.T(), client.ElevatedUntil.After(before), "elevatedUntil should be in the future, got %v", client.ElevatedUntil) +} + func (s *ClientSuite) withFormData(formData string) { s.ctx.Request = httptest.NewRequest("POST", "/token", strings.NewReader(formData)) s.ctx.Request.Header.Set("Content-Type", "application/x-www-form-urlencoded") diff --git a/api/oidc.go b/api/oidc.go index 2e27d2a20..4acc43d08 100644 --- a/api/oidc.go +++ b/api/oidc.go @@ -264,7 +264,7 @@ func (a *OIDCAPI) handleElevationCallback(w http.ResponseWriter, elevate *pendin http.Error(w, "client not found", http.StatusNotFound) return } - elevatedUntil := time.Now().Add(time.Duration(elevate.DurationSeconds) * time.Second) + elevatedUntil := time.Now().Add(model.ElevationDuration(elevate.DurationSeconds)) if err := a.DB.UpdateClientElevatedUntil(client.ID, &elevatedUntil); err != nil { http.Error(w, fmt.Sprintf("failed to elevate session: %v", err), http.StatusInternalServerError) return diff --git a/model/elevate.go b/model/elevate.go index f0bd577c7..d78370cba 100644 --- a/model/elevate.go +++ b/model/elevate.go @@ -14,3 +14,24 @@ type ElevateRequest struct { } var DefaultElevationDuration = time.Hour + +// maxElevationSeconds caps a requested elevation duration (~100 years). A +// practically "infinite" elevation is intentionally supported, so this is not a +// security limit; its only purpose is to keep the value well below the point +// where time.Duration(seconds) * time.Second overflows int64 nanoseconds +// (~292 years) and would otherwise wrap to a bogus (even past) timestamp. +const maxElevationSeconds = 100 * 365 * 24 * 60 * 60 + +// ElevationDuration converts a requested duration in seconds to a +// time.Duration, clamping the seconds (before multiplying) to a safe range so +// the subsequent multiply cannot overflow. Negative values are preserved so the +// client can still cancel an elevation (the UI sends durationSeconds: -1, which +// must set elevatedUntil in the past rather than grant a fresh elevation). +func ElevationDuration(seconds int) time.Duration { + if seconds > maxElevationSeconds { + seconds = maxElevationSeconds + } else if seconds < -maxElevationSeconds { + seconds = -maxElevationSeconds + } + return time.Duration(seconds) * time.Second +} diff --git a/model/elevate_test.go b/model/elevate_test.go new file mode 100644 index 000000000..5aa6abb07 --- /dev/null +++ b/model/elevate_test.go @@ -0,0 +1,32 @@ +package model + +import ( + "testing" + "time" +) + +func TestElevationDuration(t *testing.T) { + cases := []struct { + name string + seconds int + want time.Duration + }{ + {"normal", 900, 900 * time.Second}, + {"zero stays zero", 0, 0}, + // Negative values must be preserved so the client can cancel an + // elevation (UI sends -1); they must NOT become a positive duration. + {"cancel -1 preserved", -1, -1 * time.Second}, + {"huge clamped to max", 9223372036854775807, maxElevationSeconds * time.Second}, + // Values that would overflow when multiplied but land positive are + // clamped by clamping seconds first. + {"overflow-positive clamped", 20023544073, maxElevationSeconds * time.Second}, + {"large negative clamped", -9223372037, -maxElevationSeconds * time.Second}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := ElevationDuration(c.seconds); got != c.want { + t.Fatalf("ElevationDuration(%d) = %v, want %v", c.seconds, got, c.want) + } + }) + } +}