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

TSIG fails to verify when EDNS Expire is set #1292

Closed
dmavrommatis opened this issue Sep 3, 2021 · 2 comments
Closed

TSIG fails to verify when EDNS Expire is set #1292

dmavrommatis opened this issue Sep 3, 2021 · 2 comments

Comments

@dmavrommatis
Copy link
Contributor

I have set up a Bind server as secondary to my DNS server that runs miekg/dns library and I was seeing bad signature errors. After some digging and packet sniffing I found out that the reason the verification fails is because the Bind server is sending an additional option for EDNS Expire.

I have setup a simple playground environment to mimic this behavior:

package main

import (
	"flag"
	"fmt"
	"log"
	"os"
	"os/signal"
	"strconv"
	"syscall"

	"github.com/miekg/dns"
)

type mockServer struct {
}

func (ms *mockServer) ServeDNS(writer dns.ResponseWriter, query *dns.Msg) {

	raw, _ := query.Pack()
	err := dns.TsigVerify(raw, "Ch/iN6YuFKTX6xI7Fm00HkcPaD/Pk2ilKc6gOyt8/kE=", "", false)
	if err != nil {
		fmt.Printf("error: %s", err)
	} else {
		fmt.Printf("signature verified")
	}

	err = writer.WriteMsg(query)
	if err != nil {
		return
	}
}

func main() {
	port := flag.Int("port", 8053, "port to run on")

	go func() {
		srv := &dns.Server{Addr: ":" + strconv.Itoa(*port), Net: "udp", Handler: &mockServer{}}
		if err := srv.ListenAndServe(); err != nil {
			log.Fatalf("Failed to set udp listener %s\n", err.Error())
		}
	}()

	go func() {
		srv := &dns.Server{Addr: ":" + strconv.Itoa(*port), Net: "tcp", Handler: &mockServer{}}
		if err := srv.ListenAndServe(); err != nil {
			log.Fatalf("Failed to set tcp listener %s\n", err.Error())
		}
	}()

	sig := make(chan os.Signal)
	signal.Notify(sig, syscall.SIGINT, syscall.SIGTERM)
	s := <-sig
	log.Fatalf("Signal (%v) received, stopping\n", s)
}

If you do a DNS query without the EDNS expiry option the query is verified correctly:

dig @localhost -p 8053 +nocookie -y hmac-sha256:key2.:Ch/iN6YuFKTX6xI7Fm00HkcPaD/Pk2ilKc6gOyt8/kE= foo.invalid SOA

signature verified

but if you do a DNS with the EDNS expiry it fails verifying the signature

dig @localhost -p 8053 +nocookie +ednsopt=9 -y hmac-sha256:key2.:Ch/iN6YuFKTX6xI7Fm00HkcPaD/Pk2ilKc6gOyt8/kE= foo.invalid SOA

error: dns: bad signature

I tried with other EDNS options and the verification is working fine. Is this a bug or am I missing something regarding the Expiry option?

@dmavrommatis
Copy link
Contributor Author

dmavrommatis commented Sep 4, 2021

Doing some further research it seems that the option data field of EDNS_EXPIRY can be missing.
image

But when I see the wire format of the library it seems that it has the data initialized.
image

original packet is 121bytes while the one parse by the library 124bytes.

Hex dump from wireshark for OPT

0x0 0x0 0x29 0x10 0x0 0x0 0x0 0x0 0x0 0x0 0x4 0x0 0x9 0x0 0x0

Hex dump from intellij for OPT

0x0 0x0 0x29 0x10 0x0 0x0 0x0 0x0 0x0 0x0 0x8 0x0 0x9 0x0 0x4 0x0 0x0 0x0 0x0

@dmavrommatis
Copy link
Contributor Author

dmavrommatis commented Sep 4, 2021

seems that the error is in the unpack method. The []byte is correct when the message is received but when it unpacks it to dns.Msg it bumps the rdlength from 4 to 8 (byte 39 in picture)!
image

dmavrommatis added a commit to dmavrommatis/dns that referenced this issue Sep 6, 2021
…iekg#1292)

As per [RFC7134](https://datatracker.ietf.org/doc/html/rfc7314#section-2) the Expire
Option in queries should be zero-length. In the current implementation the field is
uint32 which always instatiates 4bytes for that field when packing to wire format.
For that reason we change the field to []uint8 so it can support 0-length and 4-byte
length option data.
dmavrommatis added a commit to dmavrommatis/dns that referenced this issue Sep 13, 2021
…iekg#1292)

As per [RFC7134](https://datatracker.ietf.org/doc/html/rfc7314#section-2) the Expire
Option in queries should be zero-length. In the current implementation the field is
uint32 which always instatiates 4bytes for that field when packing to wire format.
For that reason we change the field to []uint8 so it can support 0-length and 4-byte
length option data.
dmavrommatis added a commit to dmavrommatis/dns that referenced this issue Sep 13, 2021
…iekg#1292)

As per [RFC7134](https://datatracker.ietf.org/doc/html/rfc7314#section-2) the Expire
Option in queries should be zero-length. In the current implementation the field is
uint32 which always instatiates 4bytes for that field when packing to wire format.
For that reason we change the field to []uint8 so it can support 0-length and 4-byte
length option data.
@miekg miekg closed this as completed in d48e92a Mar 12, 2022
aanm pushed a commit to cilium/dns that referenced this issue Jul 29, 2022
…ekg#1292) (miekg#1293)

* Change EDNS_EXPIRE field to support zero length option data (Resolves miekg#1292)

As per [RFC7134](https://datatracker.ietf.org/doc/html/rfc7314#section-2) the Expire
Option in queries should be zero-length. In the current implementation the field is
uint32 which always instatiates 4bytes for that field when packing to wire format.
For that reason we change the field to []uint8 so it can support 0-length and 4-byte
length option data.

* addressed comments

* addressed comments

* make change backwards compatible

* add comment for Empty field
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

No branches or pull requests

1 participant