Support inspecting fragmented DNS message for FQDN policy #4732
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/test-e2e |
tnqn
force-pushed
the
fix-fragmented-dns
branch
from
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. |
tnqn
force-pushed
the
fix-fragmented-dns
branch
from
4118098
to
17d239f
Compare
|
/test-all |
Dyanngg
reviewed
Mar 21, 2023
antoninbas
previously approved these changes
Mar 21, 2023
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>
tnqn
force-pushed
the
fix-fragmented-dns
branch
from
17d239f
to
342abe9
Compare
|
/skip-all which have passed before the last comment update. |
hongliangl
reviewed
Mar 22, 2023
| }, | ||
| } | ||
|
|
||
| // Pack msg and then unpack into partialMsg |
There was a problem hiding this comment.
Suggested change
| // 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.
Suggested change
| // 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.
Suggested change
| // 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.
Suggested change
| // 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. |
Comment on lines
+119
to
+121
| // 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.
Suggested change
| // 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.
Suggested change
| // 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.
Suggested change
| // 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.
Suggested change
| // 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. |
Closed
jainpulkit22
pushed a commit
to urharshitha/antrea
that referenced
this pull request
Apr 28, 2023
…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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.