From bdbd5ac472ee73cdb566ac6348b21f9569b61598 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 20 Aug 2026 16:47:41 +0000 Subject: [PATCH] fix(identity): 1 defect(s) (OC-0195) Bound free-text profile fields by raw byte length before cleanText's quadratic sanitizeToFixpoint pass runs, generalizing OC-0192's guard into cleanTextBounded and applying it to HandlePresenceUpdate's custom_status, SetCustomStatus, and group DM names. --- Server/service/channel.go | 12 ++++++--- Server/service/dm.go | 17 +++++++----- Server/service/profile_fields_test.go | 38 +++++++++++++++++++++++++++ Server/service/user.go | 33 ++++++++++++++++++++--- 4 files changed, 86 insertions(+), 14 deletions(-) diff --git a/Server/service/channel.go b/Server/service/channel.go index 91bf0e1a..f8db8af8 100644 --- a/Server/service/channel.go +++ b/Server/service/channel.go @@ -5,7 +5,6 @@ import ( "fmt" "log/slog" "time" - "unicode/utf8" "github.com/owncord/server/auth" "github.com/owncord/server/db" @@ -179,9 +178,14 @@ func (s *ChannelService) HandlePresenceUpdate(ctx context.Context, userID int64, var cleaned *string if customStatus != nil { - text := cleanText(*customStatus) - if utf8.RuneCountInString(text) > MaxCustomStatusLen { - return nil, fmt.Errorf("%w: custom_status must be at most %d characters", ErrBadRequest, MaxCustomStatusLen) + // OC-0195: bound the raw bytes before cleanText (sanitizeToFixpoint) + // runs — see cleanTextBounded's doc comment (user.go). This path is + // reachable over the WS presence_update frame, whose read limit is + // config.MaxMessageBytes (1 MiB), far larger than any REST body that + // reaches the equivalent guard on SetCustomStatus/UpdateProfile. + text, err := cleanTextBounded(*customStatus, MaxCustomStatusLen, "custom_status") + if err != nil { + return nil, err } cleaned = nullable(text) } diff --git a/Server/service/dm.go b/Server/service/dm.go index 4f47b0c2..6585e9f0 100644 --- a/Server/service/dm.go +++ b/Server/service/dm.go @@ -5,7 +5,6 @@ import ( "fmt" "log/slog" "time" - "unicode/utf8" "github.com/owncord/server/auth" "github.com/owncord/server/db" @@ -231,9 +230,11 @@ func (s *DMService) CreateGroupDM(ctx context.Context, userID int64, recipientID return nil, fmt.Errorf("%w: a group DM holds at most %d users", ErrBadRequest, db.MaxGroupDMParticipants) } - cleanName := cleanText(name) - if utf8.RuneCountInString(cleanName) > MaxGroupDMNameLen { - return nil, fmt.Errorf("%w: name must be at most %d characters", ErrBadRequest, MaxGroupDMNameLen) + // OC-0195 sibling: bound the raw bytes before cleanText (sanitizeToFixpoint) + // runs — see cleanTextBounded's doc comment (user.go). + cleanName, err := cleanTextBounded(name, MaxGroupDMNameLen, "name") + if err != nil { + return nil, err } for _, rid := range unique { @@ -320,9 +321,11 @@ func (s *DMService) RenameGroupDM(ctx context.Context, userID, channelID int64, return nil, fmt.Errorf("%w: only group DMs can be named", ErrBadRequest) } - cleanName := cleanText(name) - if utf8.RuneCountInString(cleanName) > MaxGroupDMNameLen { - return nil, fmt.Errorf("%w: name must be at most %d characters", ErrBadRequest, MaxGroupDMNameLen) + // OC-0195 sibling: bound the raw bytes before cleanText (sanitizeToFixpoint) + // runs — see cleanTextBounded's doc comment (user.go). + cleanName, err := cleanTextBounded(name, MaxGroupDMNameLen, "name") + if err != nil { + return nil, err } if err := s.st.SetDMChannelName(ctx, channelID, cleanName); err != nil { diff --git a/Server/service/profile_fields_test.go b/Server/service/profile_fields_test.go index 74b7492c..91f599a1 100644 --- a/Server/service/profile_fields_test.go +++ b/Server/service/profile_fields_test.go @@ -336,6 +336,44 @@ func TestHandlePresenceUpdate_AcceptsInvisibleAndCarriesCustomStatus(t *testing. } } +// OC-0195: same defect as OC-0192 (TestUpdateProfile_OversizedDisplayNameAndAboutRejectedBeforeSanitizing) +// but reached over presence_update instead of PATCH /users/me. HandlePresenceUpdate +// applied MaxCustomStatusLen to cleanText's *output*, so an adversarial +// nested-entity payload paid the full quadratic sanitizeToFixpoint cost before +// ever being measured. The WS read limit (config.MaxMessageBytes, 1 MiB) admits +// a payload here far larger than PATCH /users/me's body ever could, and this +// runs on the connection's own readPump goroutine. +func TestHandlePresenceUpdate_OversizedCustomStatusRejectedBeforeSanitizing(t *testing.T) { + database := newTestDB(t) + seedUser(t, database, &db.User{ID: 1, Username: "ada", PasswordHash: "h"}) + svc := NewChannelService(database, NewPermissionService(database, permissions.NewChecker(database))) + ctx := context.Background() + + // Adversarial nested-entity payload (16 KB) — see sanitizeToFixpoint's + // doc comment (message.go) for why this shape is quadratic to sanitize. + huge := "&" + strings.Repeat("amp;", 4000) + "lt;" + + start := time.Now() + _, err := svc.HandlePresenceUpdate(ctx, 1, db.StatusOnline, &huge, nil) + elapsed := time.Since(start) + if !errors.Is(err, ErrBadRequest) { + t.Errorf("oversized custom_status err = %v, want ErrBadRequest", err) + } + // A guard that runs before sanitizing rejects in well under a + // millisecond; the pre-fix code spends well over 150ms in + // sanitizeToFixpoint on this payload before the rune-count check ever + // runs. 150ms gives generous margin over noise while staying far below + // the unguarded cost. + if elapsed > 150*time.Millisecond { + t.Errorf("oversized custom_status took %v, want well under 150ms (raw field must be bounded before sanitizing)", elapsed) + } + // The rejected call must not have committed the status either. + u, _ := database.GetUserByID(ctx, 1) + if u.Status == db.StatusOnline { + t.Error("a rejected presence_update must not commit the status") + } +} + func TestHandlePresenceUpdate_RejectsUnknownStatusAndOverlongText(t *testing.T) { database := newTestDB(t) seedUser(t, database, &db.User{ID: 1, Username: "ada", PasswordHash: "h"}) diff --git a/Server/service/user.go b/Server/service/user.go index ee5024e9..9358d4d8 100644 --- a/Server/service/user.go +++ b/Server/service/user.go @@ -114,6 +114,33 @@ func cleanText(v string) string { return strings.TrimSpace(sanitizeToFixpoint(v)) } +// cleanTextBounded is cleanText plus the raw-byte guard OC-0192 established +// for UpdateProfile's DisplayName/About fields, generalized for every other +// free-text field that runs through cleanText: SetCustomStatus, +// HandlePresenceUpdate's custom_status, and group DM names (OC-0195). +// +// cleanText's sanitizeToFixpoint pass is quadratic in input length, so a +// bound applied only to its *output* (a plain rune-count check on the +// cleaned string) still lets an adversarial nested-entity payload pay the +// full sanitize cost first — it can even sanitize down to something well +// under maxRunes and be silently accepted, having spent seconds of CPU to +// get there. The byte-length pre-check runs before cleanText ever does, on +// the untouched input, so the cost of rejecting an oversized value is +// O(len(v)) instead of the sanitizer's cost. *4 is deliberately looser than +// maxRunes — it exists only to keep the sanitizer from ever seeing a +// pathological payload, not to duplicate the real (rune-count) bound, which +// still runs afterward on the cleaned, trimmed value. +func cleanTextBounded(v string, maxRunes int, fieldName string) (string, error) { + if len(v) > maxRunes*4 { + return "", fmt.Errorf("%w: %s must be at most %d characters", ErrBadRequest, fieldName, maxRunes) + } + cleaned := cleanText(v) + if utf8.RuneCountInString(cleaned) > maxRunes { + return "", fmt.Errorf("%w: %s must be at most %d characters", ErrBadRequest, fieldName, maxRunes) + } + return cleaned, nil +} + // resolveOptional picks the column value for one nullable text field: the // sanitized patch when it was supplied, the existing row otherwise. func resolveOptional(patch *string, existing *string) *string { @@ -216,9 +243,9 @@ func (s *UserService) UpdateProfile(ctx context.Context, userID int64, patch Pro // value persists across reconnects and is cleared explicitly on logout, which // is why it is stored rather than held on the connection. func (s *UserService) SetCustomStatus(ctx context.Context, userID int64, text string) error { - cleaned := cleanText(text) - if utf8.RuneCountInString(cleaned) > MaxCustomStatusLen { - return fmt.Errorf("%w: custom_status must be at most %d characters", ErrBadRequest, MaxCustomStatusLen) + cleaned, err := cleanTextBounded(text, MaxCustomStatusLen, "custom_status") + if err != nil { + return err } if err := s.st.UpdateUserCustomStatus(ctx, userID, nullable(cleaned)); err != nil { return fmt.Errorf("%w: failed to update custom status: %v", ErrInternal, err)