paulc / dnslib

A Python library to encode/decode DNS wire-format packets
https://github.com/paulc/dnslib
BSD 2-Clause "Simplified" License
295 stars 84 forks source link

0.9.21 Raises TypeError instead of DNSError when failing to parse HTTPS records #43

Closed robinlandstrom closed 1 year ago

robinlandstrom commented 1 year ago

Hi Paul and thanks for a great library!

It seems like the change in dns.py https://github.com/paulc/dnslib/commit/ce070170f989a994fded1b973537e1fbb3a7c45c breaks the error reporting on parse errors on HTTPS records. hexlify return bytes and hex() that was used earlier returns a string.

Here is some code to reproduce, possibly there are some issues with the HTTPS parsing in general since this packet is parsed okay in Wireshark.

Anything I can do to help? Not that familiar with the HTTPS record type thou..

Python 3.10.6 (main, Aug  3 2022, 17:39:45) [GCC 12.1.1 20220730] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import dnslib
>>> import binascii
>>> dnslib.DNSRecord.parse(binascii.unhexlify("93088410000100020000000107646973636f726403636f6d0000410001c00c004100010000012c002b0001000001000c0268330568332d323902683200040014a29f80e9a29f87e8a29f88e8a29f89e8a29f8ae8c00c002e00010000012c005f00410d020000012c632834e5632575c586c907646973636f726403636f6d0044d488ce4a5b9085289c671f0296b2b06cffaca28880c57643befd43d6de433d84ae078b282fc2cdd744f3bea2f201042a7a0d6f3e17ebd887b082bbe30dfda100002904d0000080000000"))
Traceback (most recent call last):
  File "/home/lanrob1702/.cache/pypoetry/virtualenvs/nmsg-processor-l4-TU0Er-py3.10/lib/python3.10/site-packages/dnslib/dns.py", line 1837, in parse
    v = bytearray(buffer.get(n))
  File "/home/lanrob1702/.cache/pypoetry/virtualenvs/nmsg-processor-l4-TU0Er-py3.10/lib/python3.10/site-packages/dnslib/buffer.py", line 63, in get
    raise BufferError("Not enough bytes [offset=%d,remaining=%d,requested=%d]" %
dnslib.buffer.BufferError: Not enough bytes [offset=138,remaining=64,requested=40039]

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/lanrob1702/.cache/pypoetry/virtualenvs/nmsg-processor-l4-TU0Er-py3.10/lib/python3.10/site-packages/dnslib/dns.py", line 109, in parse
    rr.append(RR.parse(buffer))
  File "/home/lanrob1702/.cache/pypoetry/virtualenvs/nmsg-processor-l4-TU0Er-py3.10/lib/python3.10/site-packages/dnslib/dns.py", line 814, in parse
    rdata = RDMAP.get(QTYPE.get(rtype),RD).parse(
  File "/home/lanrob1702/.cache/pypoetry/virtualenvs/nmsg-processor-l4-TU0Er-py3.10/lib/python3.10/site-packages/dnslib/dns.py", line 1841, in parse
    raise DNSError("Error unpacking HTTPS: " + str(e) + binascii.hexlify(buffer.data[buffer.offset:]))
TypeError: can only concatenate str (not "bytes") to str

image

felixonmars commented 1 year ago

I am getting the same error running the self tests.

paulc commented 1 year ago

Thanks for spotting this - I suspect that the code path is only checked if the fuzz testing generates an invalid record which triggers the exception so I haven't seen this happen when I run run_tests.sh.

In the short term I've made a simple fix that just generates the right exception but haven't done any digging into the HTTPS record processing - the code was kindly donated by someone and I'm not familiar enough with the HTTPS record to work out what is happening (I've never come across one).

I've released 0.9.22 which includes the temporary fix (should be available on PyPi)

I will leave the issue open however and see if I get some time to look at the HTTPS records.

robinlandstrom commented 1 year ago

Took a look at this and found / fixed one issue in the parsing where the HTTPS parser ignores the length parameter. PR https://github.com/paulc/dnslib/pull/44

paulc commented 1 year ago

Merged - thanks (haven't released new version to PyPi yet - see next issue)