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

dnsdist: Expose trailing data #6967

Merged
merged 14 commits into from Jan 18, 2019
Merged

Conversation

gibson042
Copy link
Contributor

Short description

Expose data following a DNS message to Lua for both reading and writing.
Fixes #6846.

Incorporates #6897 (diff from that base).

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good, a couple nits.

regression-tests.dnsdist/test_Trailing.py Show resolved Hide resolved
dq.len = length + data.size();
uint8_t* tail = message + length - 1;
for(const auto& pair : data) {
*(tail + pair.first) = pair.second;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm probably paranoid but I'd prefer the use of a counter instead of using the indexes passed by Lua.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was also wondering about that... but is the order of data from Lua tables guaranteed? And if not, should I just guard the write like e.g.

int size = data.size();
for(const auto& pair : data) {
  if(pair.first < 1 || pair.first > size) {
    /* reject sparse input */
    return false;
  }
  *(tail + pair.first) = pair.second;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be fair if the order is not right I'd consider it a bug in the wrapper. Still I'd like some sort of check, yes, but we need to be careful because we already increased dq.len at that point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what kind of protection would you like to see?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rgacogne would it make more sense to follow what #6068 is doing with EDNS option data and expose the trailing data as a string rather than a uint8 list? It would work as long as 0x00 is allowed and strings are sufficiently manipulable in Lua, and would resolve the ordering concern.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry it took me so long to answer this comment :-/
Yes, we have been using binary-safe strings in dnsdist for this kind of things so far, so IMHO it would be a better choice. Again, thanks a lot for contributing.

@gibson042
Copy link
Contributor Author

@rgacogne I updated this PR to expose the data as a string rather than an array of bytes, and added tests for non-ASCII octets in the trailing data (both inbound and outbound). This should be ready for final review.

Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good, just a couple nits but nothing huge. I really like your construction, by the way:

for method in ("sendUDPQuery", "sendTCPQuery"):
            sender = getattr(self, method)

except dns.message.TrailingJunk as e:
if trailingDataResponse is False:
raise
print("UDP query with trailing data, synthesizing response")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code looks quite confusing to me. ignoreTrailing will only be True if trailingDataResponse is True, not if it is a numerical value like a response code, which means we would then pass True in the synthesize parameter to _getResponse(), which feels wrong.
Since as far as I can tell we never pass set trailingDataResponse to True, perhaps we could just remove ignoreTrailing altogether?

Copy link
Contributor Author

@gibson042 gibson042 Oct 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing structure was already a bit confusing, and I was trying to preserve it as much as possible (which unfortunately didn't help). But the idea is to extend the existing ignoreTrailing parameter into trailingDataResponse as follows:

  • False: reject queries with trailing data (preexisting behavior)
  • True: ignore trailing data after queries (preexisting behavior)
  • anything else: interpret as RCODE to be sent in response to queries with trailing data (new behavior, used by TestTrailingDataToBackend in regression-tests.dnsdist/test_Trailing.py)

We will never pass synthesize=True to _getResponse, because forceRcode is only set to trailingDataResponse when dns.message.from_wire raises dns.message.TrailingJunk, which cannot happen when it is ignoring trailing data.

I have added comments and safety-enforcing code.

char* message = reinterpret_cast<char*>(dq.dh);
const uint16_t messageLen = getDNSPacketLength(message, dq.len);
const uint16_t tailLen = tail.size();
if(messageLen + tailLen > dq.size) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big deal but it looks like messageLen + tailLen could overflow before being promoted. Perhaps something like:

Suggested change
if(messageLen + tailLen > dq.size) {
if(tailLen > (dq.size - messageLen)) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! Updated.

Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM!

@gibson042 gibson042 changed the title Expose trailing data dnsdist: Expose trailing data Oct 19, 2018
@gibson042
Copy link
Contributor Author

@rgacogne Why was dnsdist 1.3.3 released including DNSQuestion:getEDNSOptions() but not these additions?

@rgacogne
Copy link
Member

rgacogne commented Nov 9, 2018

Why was dnsdist 1.3.3 released including DNSQuestion:getEDNSOptions() but not these additions?

The main reason is that we are having a hard time deciding whether we want to make it possible to use trailing data within dnsdist. I personally don't like the idea too much, but if people are going to do it anyway I'd prefer the feature to be merged and be done with it.

@gibson042
Copy link
Contributor Author

if people are going to do it anyway I'd prefer the feature to be merged and be done with it.

I really wish you would do that. Remember that this feature also provides a means of protecting downstream servers from trailing data:

function removeTrailingData(dq)
    local success = dq:setTrailingData("")
    if not success then
        return DNSAction.ServFail, ""
    end
    return DNSAction.None, ""
end

@rgacogne rgacogne merged commit b604ee2 into PowerDNS:master Jan 18, 2019
@rgacogne
Copy link
Member

Sorry it took so long, thanks a lot for this pull request!

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

Successfully merging this pull request may close these issues.

None yet

2 participants