New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix counting of escape sequences when splitting TXT strings #1540
Conversation
A more detailed explanation of the problem: #1384 (comment) |
Apologies, found a bug! Fix and more tests coming today. Edit: PR updated. |
`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.
d3869e0
to
00d0072
Compare
I think this is OK (my gawd... txt records)... I see a |
Thanks for looking! I had indeed found one, |
FYI: we've been running this in production for 3+ weeks with no issues |
Seems to have triggered https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=68124 cc @miekg |
hmm, thanks. After some clicking this is the testcase:
|
Yikes, thanks @catenacyber. I don't have access to the report but I'll try to figure out what's happening here. |
@miekg I'm using slightly modified versions of I've also looked over HINFO's 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? |
Here is the base64-encoded file
FuzzNewRR is the target
|
Thanks a ton! The problem seems to be within 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: |
Haven't tested it, but I think |
Here's my proposed fix: #1557 |
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 <#1557>
—
Reply to this email directly, view it on GitHub
<#1540 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACWIW3LQBKBHHSO4XBE2MLY6EJTZAVCNFSM6AAAAABEBXPUECVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRWGU2DMMJQGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Thank you Miek! It would be an honor! |
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
<#1540 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACWIW34NQOYKLH2CHF6J33Y7IFKXAVCNFSM6AAAAABEBXPUECVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZYHA4DSNZRGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
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, andendingToTxtSlice
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.