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

EDNS(0) padding according to RFC7830 for IETF 101 Hackathon #6

Closed
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
2 participants
@loganaden

loganaden commented Mar 5, 2018

No description provided.

@shuque

This comment has been minimized.

Owner

shuque commented Apr 21, 2018

Thanks for the pull request. I have a few comments:

  • Your code appears to be padding the size of the OPT RR itself to a multiple of 64 octets. The padding option is used to hide the size of the DNS query (which is not contained in the OPT RR), and I believe the intent of the RFC is to pad the entire DNS message (not just the OPT RR).

  • Your code to produce the padding payload excerpted here:

    for i in range (0,padlen):
    optdata+=struct.pack('!H', 0)

    The above inadvertently creates a padding that is double the size of "padlen". You should either use struct.pack('B', 0) or probably the simpler: optdata = b'\x00' * padlen.

  • The padding policies draft recommends a pad size of 128, rather than 64.

  • RFC 7830 also says that implementations must not use this option if the transport is not encrypted. Hence you probably need an additional check to see if TLS is being used. Although, it might be reasonable for a debugging tool to optionally allow padding when unencrypted (perhaps with another command line option like "+pad_unencrypted").

Let me know if I misunderstood any part of your code.

Shumon.

@shuque shuque changed the base branch from master to develop Apr 29, 2018

@shuque

This comment has been minimized.

Owner

shuque commented Apr 29, 2018

I've had another quick look at this.

Adding the padding option is bit more involved than other EDNS options. We need to calculate the entire length of the query message before the padding option has been added, and then calculate how much to pad. Since the query can optionally contain a TSIG RR (which needs to be the last record in the Additional section, and which contains a signature computed over the entire message including padding), we also need to pre-compute the expected length of the TSIG RR and include that in the message length before perform the padding calculation.

I'm working on refactoring the code a bit to make this easier to do.

@shuque

This comment has been minimized.

Owner

shuque commented Apr 30, 2018

While re-organizing the OPT RR code (mainly to convert it into a Python class), I've added EDNS padding support also. Basically, it calculates how large the entire DNS message is before adding any padding option, and also including the expected length of any TSIG record that follows the OPT RR. And then pads the size to the closest multiple of 128 by default. +padding=N can be used to change the blocksize.

Here are the changes if you're interested in taking a look:

5585d7a...3d92553

Sorry, I couldn't take your patch as you wrote it. I look forward to your next pull request.

Shumon.

@loganaden

This comment has been minimized.

loganaden commented May 1, 2018

@shuque shuque closed this May 4, 2018

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