diff --git a/admin.go b/admin.go index aed63e6..669088b 100644 --- a/admin.go +++ b/admin.go @@ -880,17 +880,30 @@ func (b *Barnard) addManualBan(text string) { b.AddOutputLine("Admin: ban address must be CIDR, for example 192.0.2.1/32") return } - minutes, err := strconv.Atoi(parts[1]) + duration, err := manualBanDuration(parts[1]) if err != nil { - b.AddOutputLine("Admin: ban minutes must be a number") + b.AddOutputLine("Admin: " + err.Error()) return } reason := strings.Join(parts[2:], " ") - b.adminBanList.Add(ip, network.Mask, reason, time.Duration(minutes)*time.Minute) + b.adminBanList.Add(ip, network.Mask, reason, duration) b.Client.Send(b.adminBanList) b.AddOutputLine("Admin: manual ban sent") } +// manualBanDuration validates user input before it reaches the unsigned +// protocol duration field. +func manualBanDuration(text string) (time.Duration, error) { + minutes, err := strconv.Atoi(text) + if err != nil { + return 0, fmt.Errorf("ban minutes must be a number") + } + if minutes < 0 { + return 0, fmt.Errorf("ban minutes must not be negative") + } + return time.Duration(minutes) * time.Minute, nil +} + func (b *Barnard) unbanIndex(index int) { if index < 0 || index >= len(b.adminBanList) { b.AddOutputLine("Admin: ban index out of range") diff --git a/admin_test.go b/admin_test.go index 44682fb..59d0c80 100644 --- a/admin_test.go +++ b/admin_test.go @@ -65,6 +65,18 @@ func TestPermissionList(t *testing.T) { } } +// Regression: negative manual-ban minutes were cast to an unsigned protocol +// duration, turning a rejected short ban into an extremely long one. +func TestManualBanDurationRejectsNegativeMinutes(t *testing.T) { + if _, err := manualBanDuration("-1"); err == nil { + t.Fatal("negative duration was accepted") + } + got, err := manualBanDuration("15") + if err != nil || got != 15*60*1000000000 { + t.Fatalf("got %v, %v", got, err) + } +} + func TestAdminEscapeInputs(t *testing.T) { if !isAdminEscapeKey(uiterm.KeyEsc) { t.Fatal("expected escape key to close admin menu") diff --git a/fix.txt b/fix.txt index 018675a..930318f 100644 --- a/fix.txt +++ b/fix.txt @@ -266,7 +266,7 @@ Priority 3: configuration, UI, and binding hardening Both helpers panic on exec failure. Return/log an error or remove unused helpers; a missing optional beep binary must not terminate Barnard. -35. Admin manual-ban duration accepts negative values +[x] 35. Admin manual-ban duration accepts negative values Files: admin.go, gumble/gumble/bans.go Negative minutes become a negative duration, then are cast to uint32 seconds for the protocol, creating a huge ban duration. Reject negative