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

dnsdist: addXPF does not account for trailing data #6896

Closed
gibson042 opened this Issue Aug 29, 2018 · 6 comments

Comments

Projects
None yet
2 participants
@gibson042
Contributor

gibson042 commented Aug 29, 2018

  • Program: dnsdist
  • Issue type: Bug report

Short description

addXPF always places XPF records at the end of the packet, after any data in the packet following the DNS message and causing such data to be misinterpreted as a (quite likely malformed) additional-section RR.

Environment

  • Operating system: all
  • Software version: 1.3.2
  • Software source: PowerDNS repository

Steps to reproduce

Establish an environment like that of test_XPF.

  1. Configure dnsdist to add XPF, e.g. newServer{address="127.0.0.1:53", addXPF=65422}.
  2. Make a normal query (e.g., dns.message.make_query('example.com.', 'A', 'IN')) and verify that XPF is added by dnsdist (e.g., assertEquals(msg.additional[-1].rdtype, 65422)).
  3. Make a query with trailing data (e.g., dns.message.make_query('example.com.', 'A', 'IN').to_wire() + b'\x00\xff\x42\x00\x01\x00\x00\x00\x00\x00\x00') and observe that addXPF results in misinterpretation of the original trailing data as a record (e.g., assertEquals(msg.additional[-1].rdtype, 0xff42)).

Expected behaviour

addXPF always adds the record at the end of the DNS message before any trailing data, so steps 2 and 3 both result in dnsdist sending a DNS message in which the last additional-section record is XPF.

Actual behaviour

addXPF adds the record after inbound trailing data, so step 3 resulting in trailing data becoming an additional-section record and XPF became trailing data.

Other information

addXPF should probably use getDNSPacketLength to determine the proper manipulation position.

@rgacogne

This comment has been minimized.

Member

rgacogne commented Aug 29, 2018

Determining the length of the packet to know if there is any trailing data has a non-negligible performance impact, and I'm pretty sure we don't want to pay that price by default.
We might consider adding an option to do what you suggest, but then should we discard the trailing data or try to preserve it?

@gibson042

This comment has been minimized.

Contributor

gibson042 commented Aug 29, 2018

Detecting and handling trailing data is a necessary cost of adding records to the end of a DNS message—you have to know where the end is, and that's under the control of clients. An option to assume no trailing data because of a guarding addAction(TrailingDataRule(), DropAction()) or something might make sense, but I think adding a new field to DNSQuestion to just avoid paying the parsing cost twice would be better.

As for what to do with trailing data, I hope it will be preserved. I'm working on a PR for #6846 that would allow for direct manipulation so capabilities like addXPF don't need to be opinionated:

newServer{address="127.0.0.1:53", addXPF=65422}
addLuaAction(AllRule(), removeTrailingData)
@rgacogne

This comment has been minimized.

Member

rgacogne commented Aug 30, 2018

Detecting and handling trailing data is a necessary cost of adding records to the end of a DNS message—you have to know where the end is, and that's under the control of clients.

Allow me to disagree on this. dnsdist has always tried to do as little parsing as possible by default, and IMHO this is clearly a 'garbage in, garbage out' case.

@gibson042

This comment has been minimized.

Contributor

gibson042 commented Aug 31, 2018

No, this is an injection vulnerability because the "garbage in" is actually whatever data a client want to trick the servers behind dnsdist into processing as a DNS record. When trying to append data records to the end of a DNS message, you cannot blindly assume that it coincides with the end of the packet.

@rgacogne

This comment has been minimized.

Member

rgacogne commented Aug 31, 2018

Oh, right, you do have a valid point! This could indeed be used to hide a valid record (XPF or something else) from dnsdist that would be visible by the backend because of the increased arcount. I'll fix it, thanks for the heads-up and sorry I didn't understand your point earlier.

@rgacogne

This comment has been minimized.

Member

rgacogne commented Nov 8, 2018

This issue has been fixed in 1.3.3.

@rgacogne rgacogne closed this Nov 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment