diff --git a/src/main/java/org/xbill/DNS/TSIG.java b/src/main/java/org/xbill/DNS/TSIG.java index aabd364b..1f189383 100644 --- a/src/main/java/org/xbill/DNS/TSIG.java +++ b/src/main/java/org/xbill/DNS/TSIG.java @@ -132,6 +132,7 @@ public static String nameToAlgorithm(Name name) { private final Name name; private final SecretKey macKey; private final String macAlgorithm; + private final Mac sharedHmac; /** @@ -369,6 +370,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(); @@ -376,13 +414,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) { @@ -391,7 +422,7 @@ public TSIGRecord generate( fudge = Duration.ofSeconds(fudgeOption); } - if (old != null && signing) { + if (old != null && addOldSignatureFirst && signing) { hmacAddSignature(hmac, old); } @@ -572,42 +603,74 @@ public int verify(Message m, byte[] messageBytes, TSIGRecord requestTSIG) { * @since 3.2 */ 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". + // 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(); - 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; + } + + // add the query tsig to the HMAC + if (requestTSIG != null && tsig.getError() != Rcode.BADKEY && tsig.getError() != Rcode.BADSIG) { + hmacAddSignature(hmac, 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); - int len = m.tsigstart - header.length; + int len; + if (tsig != null) { + len = m.tsigstart - header.length; + } else { + len = messageBytes.length - header.length; + } + if (log.isTraceEnabled()) { log.trace(hexdump.dump("TSIG-HMAC message after header", 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 + if (tsig == null) { + m.tsigState = Message.TSIG_INTERMEDIATE; + return Rcode.NOERROR; + } + DNSOutput out = new DNSOutput(); if (fullSignature) { tsig.getName().toWireCanonical(out); @@ -651,7 +714,7 @@ public int verify(Message m, byte[] messageBytes, TSIGRecord requestTSIG, boolea signature.length); return Rcode.BADSIG; } else { - byte[] expectedSignature = hmac.doFinal(); + byte[] expectedSignature = hmac.doFinal(); // note that doFinal also resets the hmac if (!verify(expectedSignature, signature)) { if (log.isDebugEnabled()) { log.debug( @@ -676,6 +739,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(). + // 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(hmac, tsig); + m.tsigState = Message.TSIG_VERIFIED; return Rcode.NOERROR; } @@ -723,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; @@ -732,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 } /** @@ -749,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; @@ -767,8 +840,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, mac); } } } 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; + } + } }