diff --git a/src/main/java/org/xbill/DNS/TSIG.java b/src/main/java/org/xbill/DNS/TSIG.java index aabd364b..9644b623 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) @@ -369,6 +383,37 @@ 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); + } + + /** + * 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 +421,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 +429,7 @@ public TSIGRecord generate( fudge = Duration.ofSeconds(fudgeOption); } - if (old != null && signing) { + if (old != null && addOldSignatureFirst && signing) { hmacAddSignature(hmac, old); } @@ -571,42 +609,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 +694,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 +715,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 +740,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 +837,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); } } } 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; + } + } }