Fix counting of escape sequences when splitting TXT strings

[janik-cloudflare]

endingToTxtSlice, used by TXT, SPF and a few other types, parses a string such as "hello world" from an RR's content in a zone file. These strings are limited to 255 characters, and endingToTxtSlice automatically splits them if they're longer than that. However, it didn't count the length correctly: escape sequences such as \\ or \123 were counted as multiple characters (2 and 4 respectively in these examples), but they should only count as one character because they represent a single byte in wire format (which is where this 255 character limit comes from). This commit fixes that.

[janik-cloudflare]

A more detailed explanation of the problem: https://github.com/miekg/dns/issues/1384#issuecomment-1946017184

[janik-cloudflare]

Apologies, found a bug! Fix and more tests coming today.

Edit: PR updated.

[miekg]

I think this is OK (my gawd... txt records)... I see a escapedStringOffset without looking too closely, don't we already have a bunch of those type of functions? Is a new one really needed or can something older be re-used?

[janik-cloudflare]

Thanks for looking! I had indeed found one, escapedNameLen, but, while it's similar, it's also different enough to make it difficult to reuse for our purposes. Looking at callers of isDDD again now, I also don't see anything else that could easily be reused.

[janik-cloudflare]

FYI: we've been running this in production for 3+ weeks with no issues

[catenacyber]

Seems to have triggered https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=68124 cc @miekg

[miekg]

hmm, thanks.

After some clicking this is the testcase:

 hinFo ÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿ\      

[janik-cloudflare]

Yikes, thanks @catenacyber. I don't have access to the report but I'll try to figure out what's happening here.

[janik-cloudflare]

@miekg I'm using slightly modified versions of Fuzz and FuzzNewRR (surely the latter is the problematic one), but I'm not able to replicate the issue. I've tried some variations of the test case, such as removing the spaces at the start of each of the two lines, removing the six spaces at the end of the first line, in case those were added by mistake, but no luck. I also tried the second line with and without a trailing \n. Parsing fails if I remove the initial spaces, but I imagine the report isn't about a simple error (fuzz function returning code 0) but about a panic.

I've also looked over HINFO's parse() in case this is a type-specific issue, but it doesn't look like it.

I think I need some help here. If the test case is available as a binary file that'd be great. Confirmation that it is indeed FuzzNewRR that's failing would be helpful too. Also, is there a stack trace or panic message available?

[catenacyber]

Here is the base64-encoded file

IGhpbkZvIP//////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////XA==

FuzzNewRR is the target

panic: runtime error: slice bounds out of range [:256] with length 255
--
  |  
  | goroutine 17 [running, locked to thread]:
  | github.com/miekg/dns.endingToTxtSlice(0x0?, {0x755ec9, 0x10})
  | github.com/miekg/dns/scan_rr.go:60 +0xc78
  | github.com/miekg/dns.(*HINFO).parse(0x10c0000dc000, 0x10c00007c6c0?, {0x0?, 0x0?})
  | github.com/miekg/dns/scan_rr.go:201 +0x4b
  | github.com/miekg/dns.(*ZoneParser).Next(0x10c0000d2090)
  | github.com/miekg/dns/scan.go:672 +0x29b7
  | github.com/miekg/dns.ReadRR({0x810638?, 0x10c0000ce000?}, {0x0?, 0x797180?})
  | github.com/miekg/dns/scan.go:128 +0xa6
  | github.com/miekg/dns.NewRR({0x10c0000c8000, 0x106})
  | github.com/miekg/dns/scan.go:113 +0x13f
  | github.com/miekg/dns.FuzzNewRR({0x612000000940?, 0x0?, 0x5cc301?})
  | github.com/miekg/dns/fuzz.go:29 +0xdb
  | main.LLVMFuzzerTestOneInput(...)
  | ./main.1703470873.go:21

[janik-cloudflare]

Thanks a ton! The problem seems to be within escapedStringOffset, in particular the return i + 1 within if offset >= byteOffset. The function is getting confused by the trailing slash and returning an OOB offset, which should never be allowed to happen.

It should be easy enough to fix by adding a bounds check there but I'll see if I can come up with something better that's easier to follow.

Smaller test case: escapedStringOffset("aa\\", 3) returns 4.

[janik-cloudflare]

Haven't tested it, but I think escapedNameLen might suffer from a similar issue where an invalid message could be produced if the last character is a backslash. That one is probably less serious because I doubt it would panic and, regardless, a backslash shouldn't be able to get there in the first place unless someone is modifying the structs directly. Perhaps another argument for not trying to preserve zone file syntax such as escape sequences in parsed data structures though?

[janik-cloudflare]

Here's my proposed fix: #1557

[miekg]

Looks ok. Not near a computer for a couple of days..

Are you willing to accept an invite to become maintainer and able to merge?

/Miek

On Fri, 19 Apr 2024, 15:10 Janik Rabe, @.***> wrote:

Here's my proposed fix: #1557 https://github.com/miekg/dns/pull/1557

— Reply to this email directly, view it on GitHub https://github.com/miekg/dns/pull/1540#issuecomment-2066546102, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACWIW3LQBKBHHSO4XBE2MLY6EJTZAVCNFSM6AAAAABEBXPUECVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRWGU2DMMJQGI . You are receiving this because you were mentioned.Message ID: @.***>

[janik-cloudflare]

Thank you Miek! It would be an honor!

[miekg]

Ack. Ack

On mobile, but will push buttons next week. Thanks all, appreciate all the help

On Fri, 26 Apr 2024, 09:24 Janik Rabe, @.***> wrote:

Thank you Miek! It would be an honor!

— Reply to this email directly, view it on GitHub https://github.com/miekg/dns/pull/1540#issuecomment-2078889710, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACWIW34NQOYKLH2CHF6J33Y7IFKXAVCNFSM6AAAAABEBXPUECVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZYHA4DSNZRGA . You are receiving this because you were mentioned.Message ID: @.***>

[miekg]

invite finally sent