secure/precis: fix short destination buffer handling in Nickname profile The additional mapping rule for the Nickname profile (RFC 8266) requires collapsing interior sequences of spaces. The Transform implementation was writing an interior space to the destination buffer without first checking if there was enough space for both the space and the next rune. This change ensures that transform.ErrShortDst is returned if the destination buffer is too small, preventing potential out-of-bounds writes or incomplete copies. Thanks to Omkhar Arasaratnam (GitHub: omkhar) for reporting this issue. Fixes CVE-2026-56851 Fixes golang/go#80112 Change-Id: I51c8dcc58a5b8fd1002f2cdd832698651db89a15 Reviewed-on: https://go-review.googlesource.com/c/text/+/793360 LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Damien Neil <dneil@google.com>
diff --git a/secure/precis/enforce_test.go b/secure/precis/enforce_test.go index e091854..ae74218 100644 --- a/secure/precis/enforce_test.go +++ b/secure/precis/enforce_test.go
@@ -104,56 +104,172 @@ // transformation, the end result is identical to one Transform call with // sufficiently long buffers. func TestTransformerShortBuffers(t *testing.T) { - srcUnit := []byte("a\u0300cce\u0301nts") // NFD normalization form. - wantUnit := []byte("àccénts") // NFC normalization form. - src := bytes.Repeat(srcUnit, 16) - want := bytes.Repeat(wantUnit, 16) - const long = 4096 - dst := make([]byte, long) - - // 5, 7, 9, 11, 13, 16 and 17 are all pair-wise co-prime, which means that - // slicing the dst and src buffers into 5, 7, 13 and 17 byte chunks will - // fall at different places inside the repeated srcUnit's and wantUnit's. - if len(srcUnit) != 11 || len(wantUnit) != 9 || len(src) > long || len(want) > long { - t.Fatal("inconsistent lengths") + testCases := []struct { + name string + srcUnit []byte + wantUnit []byte + srcRepeat int + wantRepeat int + srcLen int + wantLen int + wantErr error + tr *Transformer + }{ + { + name: "Freeform", + srcUnit: []byte("a\u0300cce\u0301nts"), // NFD normalization form + wantUnit: []byte("àccénts"), // NFC normalization form + srcRepeat: 16, + wantRepeat: 16, + srcLen: 11, + wantLen: 9, + tr: NewFreeform().NewTransformer(), + }, + { + name: "Nickname1", + srcUnit: []byte("a\u0300cce\u0301nts"), + wantUnit: []byte("àccénts"), + srcRepeat: 16, + wantRepeat: 16, + srcLen: 11, + wantLen: 9, + tr: Nickname.NewTransformer(), + }, + { + name: "Nickname2", + srcUnit: []byte("ﷺ"), // U+FDFA + wantUnit: []byte("صلى الله عليه وسلم"), + srcRepeat: 16, + wantRepeat: 16, + srcLen: 3, + wantLen: 33, + tr: Nickname.NewTransformer(), + }, + { + name: "incomplete 3-byte rune", + srcUnit: []byte("a\xef\xb7"), // input ends in an incomplete 3-byte rune + wantUnit: []byte("a"), + srcRepeat: 1, + wantRepeat: 1, + srcLen: 3, + wantLen: 1, + wantErr: errDisallowedRune, + tr: Nickname.NewTransformer(), + }, + { + name: "bad utf8", + srcUnit: []byte("a\xff"), // \xff is never valid + wantUnit: []byte("a"), + srcRepeat: 1, + wantRepeat: 1, + srcLen: 2, + wantLen: 1, + wantErr: errDisallowedRune, + tr: Nickname.NewTransformer(), + }, + { + name: "Nickname space", + srcUnit: []byte("abcd b"), + wantUnit: []byte("abcd b"), + srcRepeat: 16, + wantRepeat: 16, + srcLen: 6, + wantLen: 6, + tr: Nickname.NewTransformer(), + }, + { + name: "Nickname return nil", + srcUnit: []byte("a b"), + wantUnit: []byte("a b"), + srcRepeat: 10, + wantRepeat: 10, + srcLen: 3, + wantLen: 3, + tr: Nickname.NewTransformer(), + }, + { + name: "Nickname partial", + srcUnit: []byte("ﷺ"), // U+FDFA (3 bytes) + wantUnit: []byte("ﷺ"), + srcRepeat: 16, + wantRepeat: 16, + srcLen: 3, + wantLen: 3, + tr: &Transformer{&nickAdditionalMapping{}}, + }, + { + name: "context boundary", + srcUnit: []byte("\u0628\u200c\u0627"), // JoiningD + ZWNJ + JoiningR (2+3+2=7 bytes) + wantUnit: []byte("\u0628\u200c\u0627"), + srcRepeat: 16, + wantRepeat: 16, + srcLen: 7, + wantLen: 7, + tr: NewFreeform().NewTransformer(), + }, } - tr := NewFreeform().NewTransformer() - for _, deltaD := range []int{5, 7, 13, 17, long} { - loop: - for _, deltaS := range []int{5, 7, 13, 17, long} { - tr.Reset() - d0 := 0 - s0 := 0 - for { - d1 := min(len(dst), d0+deltaD) - s1 := min(len(src), s0+deltaS) - nDst, nSrc, err := tr.Transform(dst[d0:d1:d1], src[s0:s1:s1], s1 == len(src)) - d0 += nDst - s0 += nSrc - if err == nil { - break + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + src := bytes.Repeat(tc.srcUnit, tc.srcRepeat) + want := bytes.Repeat(tc.wantUnit, tc.wantRepeat) + const long = 4096 + dst := make([]byte, long) + + // 5, 7, 9, 11, 13, 16 and 17 are all pair-wise co-prime, which means that + // slicing the dst and src buffers into 5, 7, 13 and 17 byte chunks will + // fall at different places inside the repeated srcUnit's and wantUnit's. + if len(tc.srcUnit) != tc.srcLen || len(tc.wantUnit) != tc.wantLen || len(src) > long || len(want) > long { + t.Fatal("inconsistent lengths") + } + for _, deltaD := range []int{5, 7, 13, 17, long} { + loop: + for _, deltaS := range []int{5, 7, 13, 17, long} { + tc.tr.Reset() + d0 := 0 + s0 := 0 + var nDst, nSrc int + var err error + for { + d1 := min(len(dst), d0+deltaD) + s1 := min(len(src), s0+deltaS) + nDst, nSrc, err = tc.tr.Transform(dst[d0:d1:d1], src[s0:s1:s1], s1 == len(src)) + d0 += nDst + s0 += nSrc + if err == nil && s0 == len(src) { + break + } + if err == nil { + continue + } + if err == transform.ErrShortDst || (err == transform.ErrShortSrc && s1 < len(src)) { + continue + } + if err == tc.wantErr && (s1 == len(src) || tc.wantErr != transform.ErrShortSrc) { + break + } + t.Errorf("deltaD=%d, deltaS=%d: %v", deltaD, deltaS, err) + continue loop + } + if tc.wantErr == nil && s0 != len(src) { + t.Errorf("deltaD=%d, deltaS=%d: s0: got %d, want %d", deltaD, deltaS, s0, len(src)) + continue + } + if d0 != len(want) { + t.Errorf("deltaD=%d, deltaS=%d: d0: got %d, want %d", deltaD, deltaS, d0, len(want)) + continue + } + got := dst[:d0] + if !bytes.Equal(got, want) { + t.Errorf("deltaD=%d, deltaS=%d:\ngot %q\nwant %q", deltaD, deltaS, got, want) + continue + } + if err != tc.wantErr { + t.Errorf("incorrect error: got %t, want %t", err, tc.wantErr) + } } - if err == transform.ErrShortDst || err == transform.ErrShortSrc { - continue - } - t.Errorf("deltaD=%d, deltaS=%d: %v", deltaD, deltaS, err) - continue loop } - if s0 != len(src) { - t.Errorf("deltaD=%d, deltaS=%d: s0: got %d, want %d", deltaD, deltaS, s0, len(src)) - continue - } - if d0 != len(want) { - t.Errorf("deltaD=%d, deltaS=%d: d0: got %d, want %d", deltaD, deltaS, d0, len(want)) - continue - } - got := dst[:d0] - if !bytes.Equal(got, want) { - t.Errorf("deltaD=%d, deltaS=%d:\ngot %q\nwant %q", deltaD, deltaS, got, want) - continue - } - } + }) } }
diff --git a/secure/precis/nickname.go b/secure/precis/nickname.go index 11e0ccb..73b8f0e 100644 --- a/secure/precis/nickname.go +++ b/secure/precis/nickname.go
@@ -44,24 +44,25 @@ // to a single ASCII space character (e.g., "St Peter" is // mapped to "St Peter"). for nSrc < len(src) { - r, size := utf8.DecodeRune(src[nSrc:]) - if size == 0 { // Incomplete UTF-8 encoding - if !atEOF { - return nDst, nSrc, transform.ErrShortSrc - } - size = 1 + if !utf8.FullRune(src[nSrc:]) && !atEOF { + return nDst, nSrc, transform.ErrShortSrc } + r, size := utf8.DecodeRune(src[nSrc:]) if unicode.Is(unicode.Zs, r) { t.prevSpace = true } else { if t.prevSpace && t.notStart { + if nDst >= len(dst) { + return nDst, nSrc, transform.ErrShortDst + } dst[nDst] = ' ' nDst += 1 + t.prevSpace = false } - if size != copy(dst[nDst:], src[nSrc:nSrc+size]) { - nDst += size + if len(dst)-nDst < size { return nDst, nSrc, transform.ErrShortDst } + copy(dst[nDst:], src[nSrc:nSrc+size]) nDst += size t.prevSpace = false t.notStart = true
diff --git a/secure/precis/profile.go b/secure/precis/profile.go index bdd991b..e62e79b 100644 --- a/secure/precis/profile.go +++ b/secure/precis/profile.go
@@ -349,13 +349,13 @@ func (c *checker) span(src []byte, atEOF bool) (n int, err error) { for n < len(src) { e, sz := dpTrie.lookup(src[n:]) - d := categoryTransitions[category(e&catMask)] if sz == 0 { if !atEOF { return n, transform.ErrShortSrc } return n, errDisallowedRune } + d := categoryTransitions[category(e&catMask)] doLookAhead := false if property(e) < c.p.class.validFrom { if d.rule == nil { @@ -389,6 +389,9 @@ n += sz } if m := c.beforeBits >> finalShift; c.beforeBits&m != m || c.termBits != 0 { + if !atEOF { + return n, transform.ErrShortSrc + } err = errContext } return n, err @@ -396,8 +399,9 @@ // TODO: we may get rid of this transform if transform.Chain understands // something like a Spanner interface. -func (c checker) Transform(dst, src []byte, atEOF bool) (nDst, nSrc int, err error) { +func (c *checker) Transform(dst, src []byte, atEOF bool) (nDst, nSrc int, err error) { short := false + if len(dst) < len(src) { src = src[:len(dst)] atEOF = false