Skip to content

Commit

Permalink
Fix possible out-of-bounds read in endingToTxtSlice caused by escaped…
Browse files Browse the repository at this point in the history
…StringOffset

If the input had a trailing backslash (normally the start of an escape
sequence) with nothing following it, `escapedStringOffset` would return
the length of the input, plus one (!), as the result index, causing an
out-of-bounds read and panic in `endingToTxtSlice`.

Consistent with, e.g., commit 2230854,
I've decided to make this an error since it definitely indicates that
the string isn't valid.

Credit to OSS-Fuzz -- thank you!
  • Loading branch information
janik-cloudflare committed Apr 19, 2024
1 parent 280787b commit 5fd1579
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 17 deletions.
18 changes: 12 additions & 6 deletions scan_rr.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,10 @@ func endingToTxtSlice(c *zlexer, errstr string) ([]string, *ParseError) {
sx := []string{}
p := 0
for {
i := escapedStringOffset(l.token[p:], 255)
i, ok := escapedStringOffset(l.token[p:], 255)
if !ok {
return nil, &ParseError{err: errstr, lex: l}
}
if i != -1 && p+i != len(l.token) {
sx = append(sx, l.token[p:p+i])
} else {
Expand Down Expand Up @@ -1919,10 +1922,10 @@ func (rr *APL) parse(c *zlexer, o string) *ParseError {

// escapedStringOffset finds the offset within a string (which may contain escape
// sequences) that corresponds to a certain byte offset. If the input offset is
// out of bounds, -1 is returned.
func escapedStringOffset(s string, desiredByteOffset int) int {
// out of bounds, -1 is returned (which is *not* considered an error).
func escapedStringOffset(s string, desiredByteOffset int) (int, bool) {
if desiredByteOffset == 0 {
return 0
return 0, true
}

currentByteOffset, i := 0, 0
Expand All @@ -1937,15 +1940,18 @@ func escapedStringOffset(s string, desiredByteOffset int) int {
} else if isDDD(s[i+1:]) {
// Skip backslash and DDD.
i += 4
} else if len(s[i+1:]) < 1 {
// No character following the backslash; that's an error.
return 0, false
} else {
// Skip backslash and following byte.
i += 2
}

if currentByteOffset >= desiredByteOffset {
return i
return i, true
}
}

return -1
return -1, true
}
34 changes: 23 additions & 11 deletions scan_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -433,25 +433,37 @@ func TestEscapedStringOffset(t *testing.T) {
input string
inputOffset int
expectedOffset int
expectedOK bool
}{
{"simple string with no escape sequences", 20, 20},
{"simple string with no escape sequences", 500, -1},
{`\;\088\\\;\120\\`, 0, 0},
{`\;\088\\\;\120\\`, 1, 2},
{`\;\088\\\;\120\\`, 2, 6},
{`\;\088\\\;\120\\`, 3, 8},
{`\;\088\\\;\120\\`, 4, 10},
{`\;\088\\\;\120\\`, 5, 14},
{`\;\088\\\;\120\\`, 6, 16},
{`\;\088\\\;\120\\`, 7, -1},
{"simple string with no escape sequences", 20, 20, true},
{"simple string with no escape sequences", 500, -1, true},
{`\;\088\\\;\120\\`, 0, 0, true},
{`\;\088\\\;\120\\`, 1, 2, true},
{`\;\088\\\;\120\\`, 2, 6, true},
{`\;\088\\\;\120\\`, 3, 8, true},
{`\;\088\\\;\120\\`, 4, 10, true},
{`\;\088\\\;\120\\`, 5, 14, true},
{`\;\088\\\;\120\\`, 6, 16, true},
{`\;\088\\\;\120\\`, 7, -1, true},
{`\`, 3, 0, false},
{`a\`, 3, 0, false},
{`aa\`, 3, 0, false},
{`aaa\`, 3, 3, true},
{`aaaa\`, 3, 3, true},
}
for i, test := range cases {
outputOffset := escapedStringOffset(test.input, test.inputOffset)
outputOffset, outputOK := escapedStringOffset(test.input, test.inputOffset)
if outputOffset != test.expectedOffset {
t.Errorf(
"Test %d (input %#q offset %d) returned offset %d but expected %d",
i, test.input, test.inputOffset, outputOffset, test.expectedOffset,
)
}
if outputOK != test.expectedOK {
t.Errorf(
"Test %d (input %#q offset %d) returned ok=%t but expected %t",
i, test.input, test.inputOffset, outputOK, test.expectedOK,
)
}
}
}

0 comments on commit 5fd1579

Please sign in to comment.