Skip to content
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

Error when retrieving binary data in TXT records #100

Open
maelp opened this issue Aug 10, 2021 · 9 comments
Open

Error when retrieving binary data in TXT records #100

maelp opened this issue Aug 10, 2021 · 9 comments

Comments

@maelp
Copy link

maelp commented Aug 10, 2021

The RFC1035 indicates that TXT records are "" which are to be interpreted as binary data of length at most 255, prefixed with a length character.

If I'm packing a TXT record containing a null byte, it is not correctly parsed by the library and the parsed output contains an empty buffer

@saghul
Copy link
Contributor

saghul commented Aug 10, 2021

Do you know if c-area is capa of parsing those? Do you have a sample record I could query?

@maelp
Copy link
Author

maelp commented Aug 10, 2021

I'm not sure whether c-ares is doing it as it should or not, but I guess it should be supposed to because the RFC indicates that's how it should be done

For instance I'm trying to use this byte string as a TXT record (I'm using dnslib to pack the data, which works as expected when I inspect the packet, and aiodns to receive and parse the packet, which shows an empty record):

b'\x00\x08\xa7\xe4\xca\x88\x06\x12\x02\x12'

and (perhaps because the first byte is null) it outputs as an empty string

@saghul
Copy link
Contributor

saghul commented Aug 10, 2021

The bug is most likely here: https://github.com/saghul/pycares/blob/4e6e36f839255ebef05e0682b98cbee1533805ce/src/pycares/__init__.py#L749-L764 -- the txt reply structures do have a length field I'm not using, I'm relying on strlen really.

A patch would be most welcome!

@maelp
Copy link
Author

maelp commented Aug 10, 2021

I guess it is this indeed https://github.com/saghul/pycares/blob/4e6e36f839255ebef05e0682b98cbee1533805ce/src/pycares/utils.py#L21

it should not necessarily force a decoding as ascii, it should be a "bytes" response (but this might break existing code as it means that the result of the resp.text is "bytes" rather than "str", and it is to each user to know how to interpret it, although this would be the correct way to do it)

@saghul
Copy link
Contributor

saghul commented Aug 10, 2021

We are already returning bytes sometimes, and str just when the response is strictly ascii. Can't we keep that behavior?

@maelp
Copy link
Author

maelp commented Aug 10, 2021

So I guess the real "error" is there, trying to cast to a _ffi.string when it should stay as a byte array (but not sure how to do this using the lib?) https://github.com/saghul/pycares/blob/4e6e36f839255ebef05e0682b98cbee1533805ce/src/pycares/__init__.py#L763

@maelp
Copy link
Author

maelp commented Aug 10, 2021

The correct way seems to be

self.text = bytes(_ffi.buffer(txt.txt, txt.length))

@maelp
Copy link
Author

maelp commented Aug 10, 2021

This PR fixes the bug saghul/pycares#160

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants
@maelp @saghul and others