Skip to content

Fix incorrect name encoding/decoding in DNS records#5064

Closed
trustin wants to merge 1 commit into
4.1from
fix_dns_record_codec
Closed

Fix incorrect name encoding/decoding in DNS records#5064
trustin wants to merge 1 commit into
4.1from
fix_dns_record_codec

Conversation

@trustin
Copy link
Copy Markdown
Member

@trustin trustin commented Apr 1, 2016

Motivation:

  • The decoded name should always end with a dot (.), but we currently
    strip it, which is incorrect.
    • (O) 0 -> "."
    • (X) 0 -> ""
    • (O) 5 netty 2 io 0 -> "netty.io."
    • (X) 5 netty 2 io 0 -> "netty.io"
  • The encoded name should end with a null-label, which is a label whose
    length is 0, but we currently append an extra NUL, causing FORMERR(1)
    on a strict DNS server:
    • (O) . -> 0
    • (X) . -> 0 0
    • (O) netty.io. -> 5 netty 2 io 0
    • (X) netty.io. -> 5 netty 2 io 0 0

Modifications:

  • Make sure to append '.' when decoding a name.
  • Improve index checks so that the decoder can raise
    CorruptFrameException instead of IIOBE
  • Do not encode extra NUL
  • Add more tests

Result:

Robustness and correctness

@trustin trustin added the defect label Apr 1, 2016
@trustin trustin added this to the 4.1.0.Final milestone Apr 1, 2016
@trustin trustin force-pushed the fix_dns_record_codec branch from 1a92f78 to 2351661 Compare April 1, 2016 11:57
normanmaurer
normanmaurer reviewed Apr 1, 2016
View reviewed changes
Comment thread codec-dns/src/main/java/io/netty/handler/codec/dns/DefaultDnsRecordDecoder.java Outdated
Copy link
Copy Markdown
Member

@normanmaurer normanmaurer Apr 1, 2016

Choose a reason for hiding this comment

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

@trustin store "." in a static field and reuse ?

@trustin trustin force-pushed the fix_dns_record_codec branch from 2351661 to ae384c3 Compare April 1, 2016 12:02
normanmaurer
normanmaurer reviewed Apr 1, 2016
View reviewed changes
Comment thread resolver-dns/src/test/java/io/netty/resolver/dns/DnsNameResolverTest.java Outdated
Copy link
Copy Markdown
Member

@normanmaurer normanmaurer Apr 1, 2016

Choose a reason for hiding this comment

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

remove this test ?

@trustin trustin force-pushed the fix_dns_record_codec branch from ae384c3 to 5a82ddf Compare April 1, 2016 12:03
normanmaurer
normanmaurer reviewed Apr 1, 2016
View reviewed changes
Comment thread codec-dns/src/main/java/io/netty/handler/codec/dns/DefaultDnsRecordEncoder.java Outdated
Copy link
Copy Markdown
Member

@normanmaurer normanmaurer Apr 1, 2016

Choose a reason for hiding this comment

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

Is the comment still accurate ?

@trustin trustin force-pushed the fix_dns_record_codec branch 4 times, most recently from 40a46bd to 970ec5b Compare April 1, 2016 12:12
@trustin
Fix incorrect name encoding/decoding in DNS records
ab30af7 
Motivation:

- The decoded name should always end with a dot (.), but we currently
  strip it, which is incorrect.
  - (O) 0 -> "."
  - (X) 0 -> ""
  - (O) 5 netty 2 io 0 -> "netty.io."
  - (X) 5 netty 2 io 0 -> "netty.io"
- The encoded name should end with a null-label, which is a label whose
  length is 0, but we currently append an extra NUL, causing FORMERR(1)
  on a strict DNS server:
  - (O) . -> 0
  - (X) . -> 0 0
  - (O) netty.io. -> 5 netty 2 io 0
  - (X) netty.io. -> 5 netty 2 io 0 0

Modifications:

- Make sure to append '.' when decoding a name.
- Improve index checks so that the decoder can raise
  CorruptFrameException instead of IIOBE
- Do not encode extra NUL
- Add more tests

Result:

Robustness and correctness
@trustin trustin force-pushed the fix_dns_record_codec branch from 970ec5b to ab30af7 Compare April 1, 2016 12:15
@normanmaurer
Copy link
Copy Markdown
Member

normanmaurer commented Apr 1, 2016

@rkapsi can you have a look as well ?

@trustin
Copy link
Copy Markdown
Member Author

trustin commented Apr 1, 2016

For the record, @normanmaurer and I had different interpretation on the encoding of a name. He thinks 5 netty 2 io 0 0 is correct while I think 5 netty 2 io 0 is correct. I guess we'd better capture the packet sent by dig to get the verdict. @normanmaurer, could you do this for me? I'm on Windows this weekend. ;-)

@Scottmitch
Copy link
Copy Markdown
Member

Scottmitch commented Apr 1, 2016

The spec is a bit confusing ... I previously thought the same as @normanmaurer .... but it looks like we were wrong. 5 netty 2 io 0 it is!

dig netty.io:

0000   07 6e 01 20 00 01 00 00 00 00 00 01 05 6e 65 74  .n. .........net
0010   74 79 02 69 6f 00 00 01 00 01 00 00 29 10 00 00  ty.io.......)...
0020   00 00 00 00 00                                   .....

the important part:

0000   05 6e 65 74 74 79 02 69 6f 00                    .netty.io.

this is followed by the type 00 01.

@normanmaurer
Copy link
Copy Markdown
Member

normanmaurer commented Apr 1, 2016

Cherry-picked into 4.1 as 4b38b72

@normanmaurer
Copy link
Copy Markdown
Member

normanmaurer commented Apr 1, 2016

@trustin Thanks!

@normanmaurer normanmaurer deleted the fix_dns_record_codec branch April 1, 2016 20:19
slandelle added a commit to AsyncHttpClient/async-http-client that referenced this pull request Apr 1, 2016
@slandelle
Fix incorrect name encoding/decoding in DNS records, backport netty/n…
01df3bc 
…etty#5064
@trustin
Copy link
Copy Markdown
Member Author

trustin commented Apr 2, 2016

@Scottmitch @normanmaurer Thanks for a quick confirmation and cherry-pick! Will release CR6 today because I need it badly.

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

Reviewers

No reviews

Assignees

@normanmaurer normanmaurer

Labels

defect

Projects

None yet

Milestone

4.1.0.CR6

Development

Successfully merging this pull request may close these issues.

3 participants

@trustin @normanmaurer @Scottmitch