From 5b8d9da0371d582052bdbb594970b377a466e854 Mon Sep 17 00:00:00 2001 From: Frank Hill Date: Tue, 24 Oct 2023 15:56:48 -0400 Subject: [PATCH 1/3] Issue 295, addresses bug with TSIG verification of large responses. --- src/main/java/org/xbill/DNS/TSIG.java | 85 ++++++++++++++++++++------- 1 file changed, 65 insertions(+), 20 deletions(-) diff --git a/src/main/java/org/xbill/DNS/TSIG.java b/src/main/java/org/xbill/DNS/TSIG.java index aabd364b..e5156e83 100644 --- a/src/main/java/org/xbill/DNS/TSIG.java +++ b/src/main/java/org/xbill/DNS/TSIG.java @@ -132,7 +132,21 @@ public static String nameToAlgorithm(Name name) { private final Name name; private final SecretKey macKey; private final String macAlgorithm; - private final Mac sharedHmac; + + // TODO: This change of mine is incompatible with 62d1222e3088f106f05032cef7697da324d699d0 and needs to be resolved. + // As written, that changeset was based on the assumption that the hmac state did not need to persist across + // calls to TSIG.verify(). Unfortunately, this is false. + // + // a few possibilities offhand: + // 1. Give up on the thread safety promise for the TSIG instance. + // That's what I've done here (just because it is simplest), but it could break client assumptions. + // 2. Have the StreamVerifier instance manage the hmac lifecycle and pass it as a parameter to TSIG.verify(). + // This seems best to me, but its necessary to think through backward compatibility issues for the API. + // 3. Use some sort of ThreadLocal cheese. + // That may cause more trouble than its worth given all DnsJava's nice async support. I don't know. + // + // I need a bit of guidance here on how to proceed. + private Mac sharedHmac; /** * Verifies the data (computes the secure hash and compares it to the input) @@ -571,42 +585,68 @@ public int verify(Message m, byte[] messageBytes, TSIGRecord requestTSIG) { * @see Rcode * @since 3.2 */ + // TODO: requestTSIG is now a bit misnamed. It is, indeed, the request TSIG on the first iteration. + // But in later iterations where the response is larger than 100 messages in length, + // it is just the "lastTSIG". public int verify(Message m, byte[] messageBytes, TSIGRecord requestTSIG, boolean fullSignature) { m.tsigState = Message.TSIG_FAILED; TSIGRecord tsig = m.getTSIG(); - if (tsig == null) { - return Rcode.FORMERR; - } - if (!tsig.getName().equals(name) || !tsig.getAlgorithm().equals(alg)) { - log.debug( + if (fullSignature) { + if (!tsig.getName().equals(name) || !tsig.getAlgorithm().equals(alg)) { + log.debug( "BADKEY failure on message id {}, expected: {}/{}, actual: {}/{}", m.getHeader().getID(), name, alg, tsig.getName(), tsig.getAlgorithm()); - return Rcode.BADKEY; + return Rcode.BADKEY; + } + + // TODO: note thread local comment on line 136 + sharedHmac = initHmac(); + + // add the query tsig to the HMAC + if (requestTSIG != null && tsig.getError() != Rcode.BADKEY && tsig.getError() != Rcode.BADSIG) { + hmacAddSignature(sharedHmac, requestTSIG); + } } - Mac hmac = initHmac(); - if (requestTSIG != null && tsig.getError() != Rcode.BADKEY && tsig.getError() != Rcode.BADSIG) { - hmacAddSignature(hmac, requestTSIG); + if (tsig != null) { + m.getHeader().decCount(Section.ADDITIONAL); } - m.getHeader().decCount(Section.ADDITIONAL); byte[] header = m.getHeader().toWire(); - m.getHeader().incCount(Section.ADDITIONAL); + + if (tsig != null) { + m.getHeader().incCount(Section.ADDITIONAL); + } + if (log.isTraceEnabled()) { log.trace(hexdump.dump("TSIG-HMAC header", header)); } - hmac.update(header); + sharedHmac.update(header); + + int len; + if (tsig != null) { + len = m.tsigstart - header.length; + } else { + len = messageBytes.length - header.length; + } - int len = m.tsigstart - header.length; if (log.isTraceEnabled()) { log.trace(hexdump.dump("TSIG-HMAC message after header", messageBytes, header.length, len)); } - hmac.update(messageBytes, header.length, len); + + sharedHmac.update(messageBytes, header.length, len); + + // TODO: style question whether to prefer "return early" or to have a single return at the end. + // return early for the intermediate messages between tsigs + if (tsig == null) { + m.tsigState = Message.TSIG_INTERMEDIATE; + return Rcode.NOERROR; + } DNSOutput out = new DNSOutput(); if (fullSignature) { @@ -630,10 +670,10 @@ public int verify(Message m, byte[] messageBytes, TSIGRecord requestTSIG, boolea if (log.isTraceEnabled()) { log.trace(hexdump.dump("TSIG-HMAC variables", tsigVariables)); } - hmac.update(tsigVariables); + sharedHmac.update(tsigVariables); byte[] signature = tsig.getSignature(); - int digestLength = hmac.getMacLength(); + int digestLength = sharedHmac.getMacLength(); // rfc4635#section-3.1, 4.: // "MAC size" field is less than the larger of 10 (octets) and half @@ -651,7 +691,7 @@ public int verify(Message m, byte[] messageBytes, TSIGRecord requestTSIG, boolea signature.length); return Rcode.BADSIG; } else { - byte[] expectedSignature = hmac.doFinal(); + byte[] expectedSignature = sharedHmac.doFinal(); // note that doFinal also resets the hmac if (!verify(expectedSignature, signature)) { if (log.isDebugEnabled()) { log.debug( @@ -676,6 +716,12 @@ public int verify(Message m, byte[] messageBytes, TSIGRecord requestTSIG, boolea return Rcode.BADTIME; } + // TODO: this leaves the hmac "dirty" at the end of the last message verification. + // But I don't see a way around it without adding more parameters to TSIG.verify(). + // FWIW, it was left dirty in the original working implementation. + // Add the current TSIG details to the HMAC for a possible next iteration + hmacAddSignature(sharedHmac, tsig); + m.tsigState = Message.TSIG_VERIFIED; return Rcode.NOERROR; } @@ -767,8 +813,7 @@ public int verify(Message m, byte[] b) { return Rcode.FORMERR; } else { log.trace("Intermediate message {} without signature", nresponses); - m.tsigState = Message.TSIG_INTERMEDIATE; - return Rcode.NOERROR; + return key.verify(m, b, lastTSIG, false); } } } From 120c1f214e17ac13fb5dea801dcc939ecabadd35 Mon Sep 17 00:00:00 2001 From: Nick Guichon Date: Wed, 25 Oct 2023 17:40:25 -0400 Subject: [PATCH 2/3] Test for Issue 295 added as well as a related change to give more control over tsig generate --- src/main/java/org/xbill/DNS/TSIG.java | 46 ++++++-- src/test/java/org/xbill/DNS/TSIGTest.java | 122 +++++++++++++++++++--- 2 files changed, 146 insertions(+), 22 deletions(-) diff --git a/src/main/java/org/xbill/DNS/TSIG.java b/src/main/java/org/xbill/DNS/TSIG.java index e5156e83..62383dde 100644 --- a/src/main/java/org/xbill/DNS/TSIG.java +++ b/src/main/java/org/xbill/DNS/TSIG.java @@ -383,6 +383,43 @@ public TSIGRecord generate(Message m, byte[] b, int error, TSIGRecord old) { */ public TSIGRecord generate( Message m, byte[] b, int error, TSIGRecord old, boolean fullSignature) { + Mac hmac = null; + boolean signing = false; + + if (error == Rcode.NOERROR || error == Rcode.BADTIME || error == Rcode.BADTRUNC) { + signing = true; + hmac = initHmac(); + } + + return generate(m, b, error, old, hmac, true, signing, fullSignature); + } + + // TODO - We had to introduce this to implement interleaved tsig requests for the test of StreamVerifier. + // A couple possiblities going forward for this: + // 1. Make the ability to send multiple messages with a shared hmac a full feature + // 2. Keep this test-only refactor of TSIG.generate + // 3. Duplicate this method with this feature in the test class as to not to expose/maintain it as part of the public API + /** + * Generates a TSIG record with a specific error for a message that has been rendered. This version is useful + * if you want to send multiple Messages without TSIGs in between. + * + * + * @param m The message + * @param b The rendered message + * @param error The error + * @param old If this message is a response, the TSIG from the request + * @param hmac Hmac to use with this generate, will be cleared if signing completes + * @param addOldSignatureFirst Makes a call to hmacAddSignature before signing. This should be false and hmacAddSignature + * should be called manually if you are attemping to send multiple Messages without TSIGs + * @param fullSignature {@code true} if this {@link TSIGRecord} is the to be added to the first of + * many messages in a TCP connection and all TSIG variables (rfc2845, 3.4.2.) should be + * included in the signature. {@code false} for subsequent messages with reduced TSIG + * variables set (rfc2845, 4.4.). + * @return The TSIG record to be added to the message + * @since 3.2 + */ + public TSIGRecord generate( + Message m, byte[] b, int error, TSIGRecord old, Mac hmac, boolean addOldSignatureFirst, boolean signing, boolean fullSignature) { Instant timeSigned; if (error == Rcode.BADTIME) { timeSigned = old.getTimeSigned(); @@ -390,13 +427,6 @@ public TSIGRecord generate( timeSigned = clock.instant(); } - boolean signing = false; - Mac hmac = null; - if (error == Rcode.NOERROR || error == Rcode.BADTIME || error == Rcode.BADTRUNC) { - signing = true; - hmac = initHmac(); - } - Duration fudge; int fudgeOption = Options.intValue("tsigfudge"); if (fudgeOption < 0 || fudgeOption > 0x7FFF) { @@ -405,7 +435,7 @@ public TSIGRecord generate( fudge = Duration.ofSeconds(fudgeOption); } - if (old != null && signing) { + if (old != null && addOldSignatureFirst && signing) { hmacAddSignature(hmac, old); } diff --git a/src/test/java/org/xbill/DNS/TSIGTest.java b/src/test/java/org/xbill/DNS/TSIGTest.java index d4e52b0e..f01d7b3f 100644 --- a/src/test/java/org/xbill/DNS/TSIGTest.java +++ b/src/test/java/org/xbill/DNS/TSIGTest.java @@ -1,29 +1,30 @@ // SPDX-License-Identifier: BSD-3-Clause 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.assertNotEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyInt; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.mockito.MockedStatic; +import org.mockito.Mockito; +import org.xbill.DNS.utils.base64; +import javax.crypto.Mac; +import javax.crypto.spec.SecretKeySpec; import java.io.IOException; import java.net.InetAddress; import java.net.InetSocketAddress; +import java.security.InvalidKeyException; +import java.security.NoSuchAlgorithmException; import java.time.Duration; import java.time.Instant; +import java.util.LinkedList; import java.util.List; import java.util.concurrent.CompletableFuture; import java.util.concurrent.Executor; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.ValueSource; -import org.mockito.MockedStatic; -import org.mockito.Mockito; -import org.xbill.DNS.utils.base64; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyInt; class TSIGTest { @Test @@ -325,4 +326,97 @@ void testTSIGMessageClone() throws IOException { assertNotNull(cloneBytes); assertNotEquals(0, cloneBytes.length); } + + @Test + void testTSIGStreamVerifier() throws IOException, NoSuchAlgorithmException, InvalidKeyException { + MockMessageClient client = new MockMessageClient(new TSIG(TSIG.HMAC_SHA256, "example.", "12345678")); + MockMessageServer server = new MockMessageServer(new TSIG(TSIG.HMAC_SHA256, "example.", "12345678")); + + byte[] query = client.createQuery(); + List response = server.handleQuery(query, 100, 6); + client.validateResponse(query, response); + } + + private static class MockMessageClient { + private final TSIG key; + + MockMessageClient(TSIG key) { + this.key = key; + } + + byte[] createQuery() throws TextParseException { + Name qname = Name.fromString("www.example."); + Record question = Record.newRecord(qname, Type.A, DClass.IN); + Message query = Message.newQuery(question); + query.setTSIG(key); + return query.toWire(Message.MAXLENGTH); + } + + public void validateResponse(byte[] query, List response) throws IOException { + Message queryMessage = new Message(query); + TSIG.StreamVerifier verifier = new TSIG.StreamVerifier(key, queryMessage.getTSIG()); + + for (byte[] resBytes : response) { + Message resMessage = new Message(resBytes); + assertEquals(Rcode.NOERROR, verifier.verify(resMessage, resBytes)); + } + } + } + + private static class MockMessageServer { + private final TSIG key; + + MockMessageServer(TSIG key) { + this.key = key; + } + + List handleQuery(byte[] queryMessageBytes, int responseMessageCount, int signEvery) throws IOException, NoSuchAlgorithmException, InvalidKeyException { + Message parsedQueryMessage = new Message(queryMessageBytes); + assertNotNull(parsedQueryMessage.getTSIG()); + + List responseMessageList = new LinkedList<>(); + TSIGRecord lastTsigRecord = parsedQueryMessage.getTSIG(); + + // Create an HMAC that is shared by messages between each TSIGRecord + Mac sharedHmac = Mac.getInstance("HmacSHA256"); + sharedHmac.init(new SecretKeySpec(base64.fromString("12345678"), "HmacSHA256")); + + for (int i = 0; i < responseMessageCount; i++) { + Message response = new Message(parsedQueryMessage.getHeader().getID()); + response.getHeader().setFlag(Flags.QR); + response.addRecord(parsedQueryMessage.getQuestion(), Section.QUESTION); + Record answer = Record.fromString(parsedQueryMessage.getQuestion().getName(), Type.A, DClass.IN, 300, "1.2.3." + i, null); + response.addRecord(answer, Section.ANSWER); + + boolean isNthMessage = i % signEvery == 0; + boolean isLastMessage = i == responseMessageCount - 1; + boolean isFirstMessage = i == 0; + if (isFirstMessage || isNthMessage || isLastMessage) { + byte[] unsignedResponseBytes = response.toWire(); + + // Create TSIGRecord for the latest message, the trick here is that prior messages without a TSIG have already + // been added to the sharedHmac + lastTsigRecord = key.generate(response, unsignedResponseBytes, Rcode.NOERROR, lastTsigRecord, sharedHmac, isFirstMessage, true, isFirstMessage); + response.addRecord(lastTsigRecord, Section.ADDITIONAL); + response.tsigState = Message.TSIG_SIGNED; + + // Store message as a "response" + byte[] signedResponseBytes = response.toWire(Message.MAXLENGTH); + responseMessageList.add(signedResponseBytes); + + // The call to generate above called doFinal and cleared sharedHmac, starting a new collection of signatures + // and the first thing that needs to be put in it is the previous signature. + byte[] signatureSize = DNSOutput.toU16(lastTsigRecord.getSignature().length); + sharedHmac.update(signatureSize); + sharedHmac.update(lastTsigRecord.getSignature()); + } else { + byte[] responseBytes = response.toWire(Message.MAXLENGTH); + sharedHmac.update(responseBytes); + responseMessageList.add(responseBytes); + } + } + + return responseMessageList; + } + } } From bd160e5bbda1481ccf6485eb715ccd1d1e586273 Mon Sep 17 00:00:00 2001 From: Frank Hill Date: Thu, 26 Oct 2023 19:08:01 -0400 Subject: [PATCH 3/3] Issue 295, removed direct references to shared_hmac. --- src/main/java/org/xbill/DNS/TSIG.java | 57 +++++++++++++-------------- 1 file changed, 27 insertions(+), 30 deletions(-) diff --git a/src/main/java/org/xbill/DNS/TSIG.java b/src/main/java/org/xbill/DNS/TSIG.java index 62383dde..1f189383 100644 --- a/src/main/java/org/xbill/DNS/TSIG.java +++ b/src/main/java/org/xbill/DNS/TSIG.java @@ -133,20 +133,7 @@ public static String nameToAlgorithm(Name name) { private final SecretKey macKey; private final String macAlgorithm; - // TODO: This change of mine is incompatible with 62d1222e3088f106f05032cef7697da324d699d0 and needs to be resolved. - // As written, that changeset was based on the assumption that the hmac state did not need to persist across - // calls to TSIG.verify(). Unfortunately, this is false. - // - // a few possibilities offhand: - // 1. Give up on the thread safety promise for the TSIG instance. - // That's what I've done here (just because it is simplest), but it could break client assumptions. - // 2. Have the StreamVerifier instance manage the hmac lifecycle and pass it as a parameter to TSIG.verify(). - // This seems best to me, but its necessary to think through backward compatibility issues for the API. - // 3. Use some sort of ThreadLocal cheese. - // That may cause more trouble than its worth given all DnsJava's nice async support. I don't know. - // - // I need a bit of guidance here on how to proceed. - private Mac sharedHmac; + private final Mac sharedHmac; /** * Verifies the data (computes the secure hash and compares it to the input) @@ -615,10 +602,19 @@ public int verify(Message m, byte[] messageBytes, TSIGRecord requestTSIG) { * @see Rcode * @since 3.2 */ - // TODO: requestTSIG is now a bit misnamed. It is, indeed, the request TSIG on the first iteration. + public int verify(Message m, byte[] messageBytes, TSIGRecord requestTSIG, boolean fullSignature) { + Mac hmac = initHmac(); + int rval = verify(m, messageBytes, requestTSIG, fullSignature, hmac); + hmac.reset(); // TODO: this should help maintain exact backward compatibility for certain weird sharedHmac cases + return rval; + } + + // TODO: introducing this method means we dont have to use the sharedHmac directly + // TODO: requestTSIG is a bit misnamed here. It is, indeed, the request TSIG on the first iteration. // But in later iterations where the response is larger than 100 messages in length, // it is just the "lastTSIG". - public int verify(Message m, byte[] messageBytes, TSIGRecord requestTSIG, boolean fullSignature) { + // TODO: unsure if this should be public or private + private int verify(Message m, byte[] messageBytes, TSIGRecord requestTSIG, boolean fullSignature, Mac hmac) { m.tsigState = Message.TSIG_FAILED; TSIGRecord tsig = m.getTSIG(); @@ -634,12 +630,9 @@ public int verify(Message m, byte[] messageBytes, TSIGRecord requestTSIG, boolea return Rcode.BADKEY; } - // TODO: note thread local comment on line 136 - sharedHmac = initHmac(); - // add the query tsig to the HMAC if (requestTSIG != null && tsig.getError() != Rcode.BADKEY && tsig.getError() != Rcode.BADSIG) { - hmacAddSignature(sharedHmac, requestTSIG); + hmacAddSignature(hmac, requestTSIG); } } @@ -656,7 +649,7 @@ public int verify(Message m, byte[] messageBytes, TSIGRecord requestTSIG, boolea if (log.isTraceEnabled()) { log.trace(hexdump.dump("TSIG-HMAC header", header)); } - sharedHmac.update(header); + hmac.update(header); int len; if (tsig != null) { @@ -669,7 +662,7 @@ public int verify(Message m, byte[] messageBytes, TSIGRecord requestTSIG, boolea log.trace(hexdump.dump("TSIG-HMAC message after header", messageBytes, header.length, len)); } - sharedHmac.update(messageBytes, header.length, len); + hmac.update(messageBytes, header.length, len); // TODO: style question whether to prefer "return early" or to have a single return at the end. // return early for the intermediate messages between tsigs @@ -700,10 +693,10 @@ public int verify(Message m, byte[] messageBytes, TSIGRecord requestTSIG, boolea if (log.isTraceEnabled()) { log.trace(hexdump.dump("TSIG-HMAC variables", tsigVariables)); } - sharedHmac.update(tsigVariables); + hmac.update(tsigVariables); byte[] signature = tsig.getSignature(); - int digestLength = sharedHmac.getMacLength(); + int digestLength = hmac.getMacLength(); // rfc4635#section-3.1, 4.: // "MAC size" field is less than the larger of 10 (octets) and half @@ -721,7 +714,7 @@ public int verify(Message m, byte[] messageBytes, TSIGRecord requestTSIG, boolea signature.length); return Rcode.BADSIG; } else { - byte[] expectedSignature = sharedHmac.doFinal(); // note that doFinal also resets the hmac + byte[] expectedSignature = hmac.doFinal(); // note that doFinal also resets the hmac if (!verify(expectedSignature, signature)) { if (log.isDebugEnabled()) { log.debug( @@ -748,9 +741,9 @@ public int verify(Message m, byte[] messageBytes, TSIGRecord requestTSIG, boolea // TODO: this leaves the hmac "dirty" at the end of the last message verification. // But I don't see a way around it without adding more parameters to TSIG.verify(). - // FWIW, it was left dirty in the original working implementation. + // Need to think if this could cause problems for users of the sharedHmac feature. // Add the current TSIG details to the HMAC for a possible next iteration - hmacAddSignature(sharedHmac, tsig); + hmacAddSignature(hmac, tsig); m.tsigState = Message.TSIG_VERIFIED; return Rcode.NOERROR; @@ -799,6 +792,8 @@ public static class StreamVerifier { /** A helper class for verifying multiple message responses. */ private final TSIG key; + // TODO: moving the MAC into StreamVerifier means a TSIG instance can remain stateless/thread safe + private final Mac mac; private int nresponses; private int lastsigned; private TSIGRecord lastTSIG; @@ -808,6 +803,8 @@ public StreamVerifier(TSIG tsig, TSIGRecord queryTsig) { key = tsig; nresponses = 0; lastTSIG = queryTsig; + mac = tsig.initHmac(); + mac.reset(); // TODO: unsure if certain weird sharedHmac cases might require this } /** @@ -825,13 +822,13 @@ public int verify(Message m, byte[] b) { nresponses++; if (nresponses == 1) { - int result = key.verify(m, b, lastTSIG); + int result = key.verify(m, b, lastTSIG, true, mac); lastTSIG = tsig; return result; } if (tsig != null) { - int result = key.verify(m, b, lastTSIG, false); + int result = key.verify(m, b, lastTSIG, false, mac); lastsigned = nresponses; lastTSIG = tsig; return result; @@ -843,7 +840,7 @@ public int verify(Message m, byte[] b) { return Rcode.FORMERR; } else { log.trace("Intermediate message {} without signature", nresponses); - return key.verify(m, b, lastTSIG, false); + return key.verify(m, b, lastTSIG, false, mac); } } }