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

URI record type is incorrectly serialized/deserialized #5443

Closed
Wolfizen opened this Issue Jun 21, 2017 · 2 comments

Comments

Projects
None yet
2 participants
@Wolfizen

Wolfizen commented Jun 21, 2017

  • Program: Authoritative, Recursor
  • Issue type: Bug report

Short description

PowerDNS incorrectly serializes/deserializes URI resource records. I have identified the cause of this issue, and linked a relevant portion of the source code.

Environment

  • Operating system: Ubuntu Trusty (14.04) 64-bit
  • Software version: 4.0.3
  • Software source: PowerDNS repository http://repo.powerdns.com/ubuntu trusty-rec-40 main

Steps to reproduce

  1. Load the PDNS Recursor with a script similar to:
function preresolve(dq)
    dq.rcode = pdns.NOERROR
    dq:addAnswer(pdns.URI, '1 1 "http://example.com"', 300)
    return true
end
  1. Query the recursor with dig @[server] URI example.com

Expected behaviour

The answer section should include a URI record with the same rdata as given to addAnswer:

x.org. 300 IN URI 1 1 "http://example.com"

Actual behaviour

The priority and weight values are wrong, and the URI is truncated by two characters.

; <<>> DiG 9.11.1 <<>> URI x.org @[server]
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 27571
;; flags: qr rd ra; QUERY: 1, ANSWER: 1, AUTHORITY: 0, ADDITIONAL: 1

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 4096
;; QUESTION SECTION:
;x.org.				IN	URI

;; ANSWER SECTION:
x.org.			300	IN	URI	257 26740 "tp://example.com"

;; Query time: 0 msec
;; SERVER: [server]#53([server])
;; WHEN: Wed Jun 21 14:34:29 EDT 2017
;; MSG SIZE  rcvd: 68

Other information

The root cause is that PDNS uses 8-bit integers for the Priority and Weight fields of the URI resource record, yet the standard is to use 16-bit integers. When PDNS computes the network data for this record, it pushes 8 bits for the priority, 8 bits for the weight, and variable-length for the uri. Yet when dig (or any other standard DNS client) attempts to parse this, it pops 16 bits for the priority (consuming both 8-bit numbers), 16 bits for the weight (consuming 2 characters of the uri) and variable-length for the uri (now missing two characters).

I have linked the (I presume) relevant PDNS source and RFC document specifying the URI record format.
RFC Document: https://tools.ietf.org/html/rfc7553#section-4.2
PDNS Source:

pdns/pdns/dnsrecords.cc

Lines 472 to 476 in 749ac14

boilerplate_conv(URI, QType::URI,
conv.xfr8BitInt(d_priority);
conv.xfr8BitInt(d_weight);
conv.xfrText(d_target, true, false);
)

@Habbie

This comment has been minimized.

Show comment
Hide comment
@Habbie

Habbie Jun 22, 2017

Member

Thank you for your extensive report! Please find the fix in #5444

Member

Habbie commented Jun 22, 2017

Thank you for your extensive report! Please find the fix in #5444

Habbie added a commit to Habbie/pdns that referenced this issue Jun 22, 2017

Habbie added a commit to Habbie/pdns that referenced this issue Jun 22, 2017

@Habbie Habbie closed this in fa38400 Jun 22, 2017

Habbie added a commit that referenced this issue Jun 22, 2017

Merge pull request #5444 from Habbie/uri-5443
make URI integers 16 bits, fixes #5443

pieterlexis added a commit that referenced this issue Jun 22, 2017

Merge pull request #5445 from Habbie/auth-4.0.x-uri
auth backport: make URI integers 16 bits, fixes #5443

pieterlexis added a commit that referenced this issue Jun 22, 2017

Merge pull request #5447 from Habbie/rec-4.0.x-uri
rec backport: make URI integers 16 bits, fixes #5443
@Wolfizen

This comment has been minimized.

Show comment
Hide comment
@Wolfizen

Wolfizen Jun 22, 2017

Thank you for the prompt fix! 👍

Wolfizen commented Jun 22, 2017

Thank you for the prompt fix! 👍

mind04 added a commit to mind04/pdns that referenced this issue Jul 11, 2017

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