From f81d86bf343c7b05e3db985de0efd78df0e29a95 Mon Sep 17 00:00:00 2001 From: Ingo Bauersachs Date: Wed, 6 May 2020 22:20:00 +0200 Subject: [PATCH 1/3] Fix TSIG fudge seconds rr string representation --- src/main/java/org/xbill/DNS/TSIGRecord.java | 2 +- .../java/org/xbill/DNS/TSIGRecordTest.java | 28 +++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 src/test/java/org/xbill/DNS/TSIGRecordTest.java diff --git a/src/main/java/org/xbill/DNS/TSIGRecord.java b/src/main/java/org/xbill/DNS/TSIGRecord.java index 257cf55e8..313d5b160 100644 --- a/src/main/java/org/xbill/DNS/TSIGRecord.java +++ b/src/main/java/org/xbill/DNS/TSIGRecord.java @@ -145,7 +145,7 @@ String rrToString() { sb.append(timeSigned.getEpochSecond()); sb.append(" "); - sb.append(fudge); + sb.append((int) fudge.getSeconds()); sb.append(" "); sb.append(signature.length); if (Options.check("multiline")) { diff --git a/src/test/java/org/xbill/DNS/TSIGRecordTest.java b/src/test/java/org/xbill/DNS/TSIGRecordTest.java new file mode 100644 index 000000000..2306b42fc --- /dev/null +++ b/src/test/java/org/xbill/DNS/TSIGRecordTest.java @@ -0,0 +1,28 @@ +// SPDX-License-Identifier: BSD-2-Clause +package org.xbill.DNS; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import java.time.Duration; +import java.time.Instant; +import org.junit.jupiter.api.Test; + +public class TSIGRecordTest { + @Test + void testTsigToStringFudge() { + TSIGRecord r = + new TSIGRecord( + Name.root, + DClass.IN, + 60, + TSIG.HMAC_MD5, + Instant.ofEpochSecond(1), + Duration.ofSeconds(5), + new byte[16], + 1, + 0, + null); + assertEquals( + "HMAC-MD5.SIG-ALG.REG.INT. 1 5 16 AAAAAAAAAAAAAAAAAAAAAA== NOERROR 0", r.rrToString()); + } +} From 0d58ae2ad88f0ef8ee7d5469974e74ddea62757a Mon Sep 17 00:00:00 2001 From: Ingo Bauersachs Date: Wed, 6 May 2020 22:23:51 +0200 Subject: [PATCH 2/3] Do not apply EDNS/TSIG for AXFR queries --- src/main/java/org/xbill/DNS/SimpleResolver.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/xbill/DNS/SimpleResolver.java b/src/main/java/org/xbill/DNS/SimpleResolver.java index ab8104db2..08a83150d 100644 --- a/src/main/java/org/xbill/DNS/SimpleResolver.java +++ b/src/main/java/org/xbill/DNS/SimpleResolver.java @@ -240,12 +240,6 @@ private int maxUDPSize(Message query) { */ @Override public CompletionStage sendAsync(Message query) { - Message ednsTsigQuery = query.clone(); - applyEDNS(ednsTsigQuery); - if (tsig != null) { - tsig.apply(ednsTsigQuery, null); - } - if (query.getHeader().getOpcode() == Opcode.QUERY) { Record question = query.getQuestion(); if (question != null && question.getType() == Type.AXFR) { @@ -263,6 +257,12 @@ public CompletionStage sendAsync(Message query) { } } + Message ednsTsigQuery = query.clone(); + applyEDNS(ednsTsigQuery); + if (tsig != null) { + tsig.apply(ednsTsigQuery, null); + } + return sendAsync(ednsTsigQuery, useTCP); } From 1c635cc5bb638698c16eea0ec7b5f2f2dcecd232 Mon Sep 17 00:00:00 2001 From: Ingo Bauersachs Date: Wed, 6 May 2020 23:35:55 +0200 Subject: [PATCH 3/3] Fix order of OPT and TSIG records in messages Closes #100 --- src/main/java/org/xbill/DNS/Message.java | 21 ++++-- .../java/org/xbill/DNS/SimpleResolver.java | 4 +- src/test/java/org/xbill/DNS/TSIGTest.java | 73 +++++++++++++++++++ 3 files changed, 91 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/xbill/DNS/Message.java b/src/main/java/org/xbill/DNS/Message.java index ecbab21a2..687efe40e 100644 --- a/src/main/java/org/xbill/DNS/Message.java +++ b/src/main/java/org/xbill/DNS/Message.java @@ -115,6 +115,9 @@ public static Message newUpdate(Name zone) { if (i == Section.ADDITIONAL) { if (rec.getType() == Type.TSIG) { tsigstart = pos; + if (j != count - 1) { + throw new WireParseException("TSIG is not the last record in the message"); + } } if (rec.getType() == Type.SIG) { SIGRecord sig = (SIGRecord) rec; @@ -519,7 +522,12 @@ private void toWire(DNSOutput out, int maxLength) { } } - /** Returns an array containing the wire format representation of the Message. */ + /** + * Returns an array containing the wire format representation of the {@link Message}, but does not + * do any additional processing (e.g. OPT/TSIG records, truncation). + * + *

Do NOT use this to actually transmit a message, use {@link #toWire(int)} instead. + */ public byte[] toWire() { DNSOutput out = new DNSOutput(); toWire(out); @@ -531,12 +539,15 @@ public byte[] toWire() { * Returns an array containing the wire format representation of the Message with the specified * maximum length. This will generate a truncated message (with the TC bit) if the message doesn't * fit, and will also sign the message with the TSIG key set by a call to setTSIG(). This method - * may return null if the message could not be rendered at all; this could happen if maxLength is - * smaller than a DNS header, for example. + * may return an empty byte array if the message could not be rendered at all; this could happen + * if maxLength is smaller than a DNS header, for example. + * + *

Do NOT use this method in conjunction with {@link TSIG#apply(Message, TSIGRecord)}, it + * produces inconsistent results! Use {@link #setTSIG(TSIG, int, TSIGRecord)} instead. * * @param maxLength The maximum length of the message. - * @return The wire format of the message, or null if the message could not be rendered into the - * specified length. + * @return The wire format of the message, or an empty array if the message could not be rendered + * into the specified length. * @see Flags * @see TSIG */ diff --git a/src/main/java/org/xbill/DNS/SimpleResolver.java b/src/main/java/org/xbill/DNS/SimpleResolver.java index 08a83150d..c34069fe6 100644 --- a/src/main/java/org/xbill/DNS/SimpleResolver.java +++ b/src/main/java/org/xbill/DNS/SimpleResolver.java @@ -260,13 +260,13 @@ public CompletionStage sendAsync(Message query) { Message ednsTsigQuery = query.clone(); applyEDNS(ednsTsigQuery); if (tsig != null) { - tsig.apply(ednsTsigQuery, null); + ednsTsigQuery.setTSIG(tsig, Rcode.NOERROR, null); } return sendAsync(ednsTsigQuery, useTCP); } - private CompletableFuture sendAsync(Message query, boolean forceTcp) { + CompletableFuture sendAsync(Message query, boolean forceTcp) { int qid = query.getHeader().getID(); byte[] out = query.toWire(Message.MAXLENGTH); int udpSize = maxUDPSize(query); diff --git a/src/test/java/org/xbill/DNS/TSIGTest.java b/src/test/java/org/xbill/DNS/TSIGTest.java index 170dfea55..76083355e 100644 --- a/src/test/java/org/xbill/DNS/TSIGTest.java +++ b/src/test/java/org/xbill/DNS/TSIGTest.java @@ -1,10 +1,13 @@ package org.xbill.DNS; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import java.io.IOException; +import java.util.List; +import java.util.concurrent.CompletableFuture; import org.junit.jupiter.api.Test; class TSIGTest { @@ -25,6 +28,76 @@ void TSIG_query() throws IOException { assertTrue(parsed.isSigned()); } + @Test + void TSIG_queryIsLastAddMessageRecord() throws IOException { + TSIG key = new TSIG(TSIG.HMAC_SHA256, "example.", "12345678"); + + Name qname = Name.fromString("www.example."); + Record rec = Record.newRecord(qname, Type.A, DClass.IN); + OPTRecord opt = new OPTRecord(SimpleResolver.DEFAULT_EDNS_PAYLOADSIZE, 0, 0, 0); + Message msg = Message.newQuery(rec); + msg.setTSIG(key, Rcode.NOERROR, null); + msg.addRecord(opt, Section.ADDITIONAL); + byte[] bytes = msg.toWire(512); + assertEquals(bytes[11], 2); // additional RR count, lower byte + + Message parsed = new Message(bytes); + List additionalSection = parsed.getSection(Section.ADDITIONAL); + assertEquals(Type.string(Type.OPT), Type.string(additionalSection.get(0).getType())); + assertEquals(Type.string(Type.TSIG), Type.string(additionalSection.get(1).getType())); + int result = key.verify(parsed, bytes, null); + assertEquals(result, Rcode.NOERROR); + assertTrue(parsed.isSigned()); + } + + @Test + void TSIG_queryAndTsigApplyMisbehaves() throws IOException { + Name qname = Name.fromString("www.example.com."); + Record rec = Record.newRecord(qname, Type.A, DClass.IN); + OPTRecord opt = new OPTRecord(SimpleResolver.DEFAULT_EDNS_PAYLOADSIZE, 0, 0, 0); + Message msg = Message.newQuery(rec); + msg.addRecord(opt, Section.ADDITIONAL); + assertFalse(msg.isSigned()); + + TSIG key = new TSIG(TSIG.HMAC_SHA256, "example.", "12345678"); + key.apply(msg, null); // additional RR count, lower byte + byte[] bytes = msg.toWire(Message.MAXLENGTH); + + assertThrows(WireParseException.class, () -> new Message(bytes), "Expected TSIG error"); + } + + @Test + void TSIG_queryIsLastResolver() throws IOException { + Name qname = Name.fromString("www.example.com."); + Record rec = Record.newRecord(qname, Type.A, DClass.IN); + Message msg = Message.newQuery(rec); + + TSIG key = new TSIG(TSIG.HMAC_SHA256, "example.", "12345678"); + SimpleResolver res = + new SimpleResolver("127.0.0.1") { + @Override + CompletableFuture sendAsync(Message query, boolean forceTcp) { + byte[] out = query.toWire(Message.MAXLENGTH); + try { + return CompletableFuture.completedFuture(new Message(out)); + } catch (IOException e) { + CompletableFuture f = new CompletableFuture<>(); + f.completeExceptionally(e); + return f; + } + } + }; + res.setTSIGKey(key); + Message parsed = res.send(msg); + + List additionalSection = parsed.getSection(Section.ADDITIONAL); + assertEquals(Type.string(Type.OPT), Type.string(additionalSection.get(0).getType())); + assertEquals(Type.string(Type.TSIG), Type.string(additionalSection.get(1).getType())); + int result = key.verify(parsed, parsed.toWire(), null); + assertEquals(result, Rcode.NOERROR); + assertTrue(parsed.isSigned()); + } + @Test void TSIG_response() throws IOException { TSIG key = new TSIG(TSIG.HMAC_SHA256, "example.", "12345678");