-
Notifications
You must be signed in to change notification settings - Fork 366
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
Support inspecting fragmented DNS message for FQDN policy #4732
Conversation
/test-e2e |
6d07836
to
4118098
Compare
@GraysonWu ACNPFQDNPolicyTCP test failed because of fragmented DNS message, so I assume it could happen in production envrionments. I think better to fix it before we declare the support of DNS over TCP officially.
The same test has passed stably with the patch.
|
4118098
to
17d239f
Compare
/test-all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall, thanks Quan for adding this
A DNS message sent over TCP will be fragmented if its length is greater than MTU, in which case FQDN policy wouldn't work as expected because the used DNS library expects the entire message. However, it's been observed that DNS message was fragmented frequently in tests, which may be applicable to production environments as well. On the other hand, we don't really need the entire message to support FQDN policy, and the first fragment normally already contains the information we need, the question and answer sections. This patch forks and modifies the message Unpack function to be able to handle fragmented messages. Signed-off-by: Quan Tian <qtian@vmware.com>
17d239f
to
342abe9
Compare
/skip-all which have passed before the last comment update. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
}, | ||
} | ||
|
||
// Pack msg and then unpack into partialMsg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Pack msg and then unpack into partialMsg | |
// Pack msg and unpack into partialMsg. |
// If we cannot unpack the whole array, then it will return nil | ||
func unpackRRslice(l int, msg []byte, off int) (dst1 []godns.RR, off1 int, err error) { | ||
var r godns.RR | ||
// Don't pre-allocate, l may be under attacker control |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Don't pre-allocate, l may be under attacker control | |
// Don't pre-allocate, 'l' may be under attacker control. |
} | ||
|
||
// unpackRRslice unpacks msg[off:] into an []RR. | ||
// If we cannot unpack the whole array, then it will return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// If we cannot unpack the whole array, then it will return nil | |
// If we cannot unpack the whole array, then it will return nil. |
} | ||
|
||
dns.Answer, off, err = unpackRRslice(int(dh.Ancount), msg, off) | ||
// The header counts might have been wrong so we need to update it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// The header counts might have been wrong so we need to update it | |
// The header counts might have been wrong so we need to update it. |
// Qdcount, Ancount, Nscount, Arcount can't be trusted, as they are | ||
// attacker controlled. This means we can't use them to pre-allocate | ||
// slices. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Qdcount, Ancount, Nscount, Arcount can't be trusted, as they are | |
// attacker controlled. This means we can't use them to pre-allocate | |
// slices. | |
// Qdcount, Ancount, Nscount, Arcount can't be trusted, as they are | |
// under the control of attackers. This means that we can't use them to pre-allocate | |
// slices. |
// header. This can still be useful to the caller. 9.9.9.9 sends these | ||
// when responding with REFUSED for instance. | ||
if off == len(msg) { | ||
// reset sections before returning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// reset sections before returning | |
// Reset sections before returning. |
// the additional section. | ||
func unpackPartially(dns *godns.Msg, dh godns.Header, msg []byte, off int) (err error) { | ||
// If we are at the end of the message we should return *just* the | ||
// header. This can still be useful to the caller. 9.9.9.9 sends these |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// header. This can still be useful to the caller. 9.9.9.9 sends these | |
// header. This can be still useful to the caller. 9.9.9.9 sends these |
off = len(msg) | ||
break | ||
} | ||
// If offset does not increase anymore, l is a lie |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// If offset does not increase anymore, l is a lie | |
// If offset does not increase anymore, 'l' is a lie. |
@hongliangl the code and comments are copied from third party as the directory indicates. I don't feel it's worth to update them as commented to introduce unncessary diff. |
If so, it's not worth to update them, just keep them. |
…4732) A DNS message sent over TCP will be fragmented if its length is greater than MTU, in which case FQDN policy wouldn't work as expected because the used DNS library expects the entire message. However, it's been observed that DNS message was fragmented frequently in tests, which may be applicable to production environments as well. On the other hand, we don't really need the entire message to support FQDN policy, and the first fragment normally already contains the information we need, the question and answer sections. This patch forks and modifies the message Unpack function to be able to handle fragmented messages. Signed-off-by: Quan Tian <qtian@vmware.com>
A DNS message sent over TCP will be fragmented if its length is greater than MTU, in which case FQDN policy wouldn't work as expected because the used DNS library expects the entire message. However, it's been observed that DNS message was fragmented frequently in tests, which may be applicable to production environments as well. On the other hand, we don't really need the entire message to support FQDN policy, and the first fragment normally already contains the information we need, the question and answer sections.
This patch forks and modifies the message Unpack function to be able to handle fragmented messages.