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

DNS protocol #291

Closed
wants to merge 1 commit into from
Closed

Conversation

kdrenard
Copy link
Contributor

@kdrenard kdrenard commented Jun 21, 2023

Account for answer and authority RRs in dns message before processing additional records. Also, account for multiple additional records when processing OPT RRs.
Do not ignore answers and authority records when marshaling through DNS packet.

additional records.  Also, account for multiple additional records
when processing OPT RRs.
@jelu
Copy link
Member

jelu commented Jun 27, 2023

@kdrenard can you describe the packets you got that hit this? like what OPT record was before the EDNS0?

@kdrenard
Copy link
Contributor Author

In a response from an an authoritative server, were there is (at least) one question, some set of answers and potentially additional records (e.g. glue) in the answer along with an additional record of type OPT that contains zero or more EDNS0 options in RDATA.

try: "dig @h.root-servers.net +nsid IN NS aaa." to generate a response packet

@jelu
Copy link
Member

jelu commented Jun 27, 2023

an and ns records are actually not a problem with the old code, unless they contain an OPT record themselves (is that possible?), but multiple OPTs in ar is.

Nice and easy combo EDNS0 size + nsid, will make creating PCAP for testing easy.

@kdrenard
Copy link
Contributor Author

If I recall correctly, the AN and NS counts were never pulled from the DNS header (those lines were commented out), so there was no way to skip past them. It was just assumed that if an OPT was present, it would immediately follow the question(s), and it would be the first AR. Simple queries with only an OPT were just fine.

@jelu
Copy link
Member

jelu commented Jun 28, 2023

If I recall correctly, the AN and NS counts were never pulled from the DNS header (those lines were commented out), so there was no way to skip past them.

Ah yes, I read the diff in the PR wrong.

Why did you increase loop detection to 5 btw?

@kdrenard
Copy link
Contributor Author

Why did you increase loop detection to 5 btw?

There were a few compressions that had at least 3 references (I believe they were in ARs for glue records) that were getting errors. The value 5 was a bit arbitrary, but I think at least 4.

@jelu
Copy link
Member

jelu commented Jun 28, 2023

Okay, I'm gonna fix all this in different PR's, break up the different "bugs". Will leave this open until all has been fixed.

Thanks @kdrenard !

jelu added a commit to jelu/dsc that referenced this pull request Jun 28, 2023
- `dns_protocol`: Issue DNS-OARC#291: Implement proper loop detection during decompression
@jelu jelu changed the title Account for answer and authority RRs in dns message before processing additional records. Also, account for multiple additional records when processing OPT RRs. DNS protocol Jun 28, 2023
jelu added a commit to jelu/dsc that referenced this pull request Jun 29, 2023
- `dns_protocol`: Fix parsing of additional resource records by walking answers and authorities, related to issue DNS-OARC#291
@jelu jelu mentioned this pull request Jun 29, 2023
jelu added a commit to jelu/dsc that referenced this pull request Jun 29, 2023
- `dns_protocol`: Fix parsing of additional resource records by walking answers and authorities, related to issue DNS-OARC#291
jelu added a commit to jelu/dsc that referenced this pull request Jun 29, 2023
- `dns_protocol`: Issue DNS-OARC#291: Implement proper loop detection during decompression
jelu added a commit to jelu/dsc that referenced this pull request Jun 29, 2023
- Issue DNS-OARC#291:
  - `dns_protocol`: fix EDNS0 parsing, check that it's root domain and haven't found EDNS before
  - Add test with multiple resource records, EDNS0 and nsid
- Fix test 285
jelu added a commit to jelu/dsc that referenced this pull request Jun 29, 2023
- Issue DNS-OARC#291:
  - `dns_protocol`: fix EDNS0 parsing, check that it's root domain and haven't found EDNS before
  - Add test with multiple resource records, EDNS0 and nsid
- Fix test 285
@jelu
Copy link
Member

jelu commented Jun 29, 2023

Replaced by #293, #292, #294.

@jelu jelu closed this Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants