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

Return error for empty target #1627

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Return error for empty target #1627

wants to merge 1 commit into from

Conversation

taoso
Copy link

@taoso taoso commented Jan 24, 2025

try to fix #1626

@taoso taoso requested review from miekg and tmthrgd as code owners January 24, 2025 13:28
@miekg
Copy link
Owner

miekg commented Feb 22, 2025

thanks this looks a lot better

defaults.go Outdated
@@ -185,6 +185,10 @@ func IsDomainName(s string) (labels int, ok bool) {

const lenmsg = 256

if s == "\n" {
Copy link
Owner

Choose a reason for hiding this comment

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

technically dns is 8-bit clean, and \n is sort of a valid character in a dns name, so this corner case must be dealt with in the zone parser and not here

Copy link
Author

Choose a reason for hiding this comment

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

@miekg after some dig into the parser code, it seems impossible to deal this case in the zone parser.

Because all the rr depends on the toAbsoluteName() need to deal this case. But different rr has different rdata format.

For example,

  • the CNAME has the rdata format of example.org.
  • the MX has the rdata format of 50 mail
  • the SVR has the rdata format of 0 5 5060 sipserver.example.com.
  • the HTTPS has the rdata format of HTTPS 1 . port=8002

You cannot detect the bad target without knowing the rdata format.

Fortunately, all target in the rdata need to use the toAbsoluteName() method to uniform the domain. It's seems appropriate to add the check logic in the toAbsoluteName() method.

Copy link
Owner

Choose a reason for hiding this comment

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

Indeed, but it may be possible to detect "no data" seen when a comment token appears? It's not the newline you care about but the fact nothing is specified until you see a comment token.

Don't know if that's easy to do or not, but somewhere in that area you have to look for a solution

Copy link
Author

Choose a reason for hiding this comment

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

the fact nothing is specified until you see a comment token.

This is only true for the CNAME case. For the rr with rdate containing both target and other information like MX or SVR, it fails.

For example, in bad.example.org. MX 10 ; bad mx, the target is after the priority of 10. If we want to deal this case, it seems we need to add some logic in the case zExpectRdata: branch. However, in the MX example, its rdata is not empty, but still invalid. The parser main loop cannot detect the invalid target because it does not know the rdata format.

If checking target is not accepted in the toAbsoluteName() method, it seems no choose by do the validation in the parse() method of every related rr separately. There are more that 30 different rr types need to be updated.

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.

Bad target domain parsing.
2 participants