Skip to content

Fix validation of TSIG signed responses #249

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.

Sign up for GitHub

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

Merged
ibauersachs merged 2 commits into from
Mar 19, 2022
Merged

Fix validation of TSIG signed responses #249

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
116cc88
Fix validation of signed responses
ibauersachs Mar 11, 2022
a0b3470
Cleanup of jnamed
ibauersachs Mar 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/org/xbill/DNS/DohResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -640,7 +640,7 @@ private void verifyTSIG(Message query, Message response, byte[] b, TSIG tsig) {
return;
}

int error = tsig.verify(response, b, query.getTSIG());
int error = tsig.verify(response, b, query.getGeneratedTSIG());
log.debug(
"TSIG verify for query {}, {}/{}: {}",
query.getHeader().getID(),
Expand Down
39 changes: 32 additions & 7 deletions src/main/java/org/xbill/DNS/Message.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ public class Message implements Cloneable {
private List<Record>[] sections;
private int size;
private TSIG tsigkey;
private TSIGRecord generatedTsig;
private TSIGRecord querytsig;
private int tsigerror;
private Resolver resolver;
Expand Down Expand Up @@ -274,7 +275,7 @@ public boolean findRRset(Name name, int type) {
*/
public Record getQuestion() {
List<Record> l = sections[Section.QUESTION];
if (l == null || l.size() == 0) {
if (l == null || l.isEmpty()) {
return null;
}
return l.get(0);
Expand All @@ -300,6 +301,16 @@ public TSIGRecord getTSIG() {
return (TSIGRecord) rec;
}

/**
* Gets the generated {@link TSIGRecord}. Only valid if the messages has been converted to wire
* format with {@link #toWire(int)} before.
*
* @return A generated TSIG record or {@code null}.
*/
TSIGRecord getGeneratedTSIG() {
return generatedTsig;
}

/**
* Was this message signed by a TSIG?
*
Expand All @@ -325,9 +336,9 @@ public boolean isVerified() {
* @see Section
*/
public OPTRecord getOPT() {
for (Record record : getSection(Section.ADDITIONAL)) {
if (record instanceof OPTRecord) {
return (OPTRecord) record;
for (Record r : getSection(Section.ADDITIONAL)) {
if (r instanceof OPTRecord) {
return (OPTRecord) r;
}
}
return null;
Expand Down Expand Up @@ -516,6 +527,7 @@ private void toWire(DNSOutput out, int maxLength) {
TSIGRecord tsigrec = tsigkey.generate(this, out.toByteArray(), tsigerror, querytsig);

tsigrec.toWire(out, Section.ADDITIONAL, c);
generatedTsig = tsigrec;
out.writeU16At(additionalCount + 1, startpos + 10);
}
}
Expand All @@ -536,9 +548,9 @@ 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 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.
* fit, and will also sign the message with the TSIG key set by a call to {@link #setTSIG(TSIG,
* int, TSIGRecord)}. This method 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.
*
* <p>Do NOT use this method in conjunction with {@link TSIG#apply(Message, TSIGRecord)}, it
* produces inconsistent results! Use {@link #setTSIG(TSIG, int, TSIGRecord)} instead.
Expand All @@ -556,6 +568,16 @@ public byte[] toWire(int maxLength) {
return out.toByteArray();
}

/**
* Sets the TSIG key to sign a message.
*
* @param key The TSIG key.
* @since 3.5.1
*/
public void setTSIG(TSIG key) {
setTSIG(key, Rcode.NOERROR, null);
}

/**
* Sets the TSIG key and other necessary information to sign a message.
*
Expand Down Expand Up @@ -668,6 +690,9 @@ public Message clone() {
if (querytsig != null) {
m.querytsig = (TSIGRecord) querytsig.cloneRecord();
}
if (generatedTsig != null) {
m.generatedTsig = (TSIGRecord) generatedTsig.cloneRecord();
}
return m;
}

Expand Down
16 changes: 9 additions & 7 deletions src/main/java/org/xbill/DNS/SimpleResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ public class SimpleResolver implements Resolver {

private InetSocketAddress address;
private InetSocketAddress localAddress;
private boolean useTCP, ignoreTruncation;
private boolean useTCP;
private boolean ignoreTruncation;
private OPTRecord queryOPT = new OPTRecord(DEFAULT_EDNS_PAYLOADSIZE, 0, 0, 0);
private TSIG tsig;
private Duration timeoutValue = Duration.ofSeconds(10);
Expand Down Expand Up @@ -267,12 +268,13 @@ private Message parseMessage(byte[] b) throws WireParseException {
}
}

private void verifyTSIG(Message query, Message response, byte[] b, TSIG tsig) {
private void verifyTSIG(Message query, Message response, byte[] b) {
if (tsig == null) {
return;
}
int error = tsig.verify(response, b, query.getTSIG());
log.debug("TSIG verify: {}", Rcode.TSIGstring(error));
int error = tsig.verify(response, b, query.getGeneratedTSIG());
log.debug(
"TSIG verify on message id {}: {}", query.getHeader().getID(), Rcode.TSIGstring(error));
}

private void applyEDNS(Message query) {
Expand Down Expand Up @@ -431,7 +433,7 @@ CompletableFuture<Message> sendAsync(Message query, boolean forceTcp, Executor e
return f;
}

verifyTSIG(query, response, in, tsig);
verifyTSIG(query, response, in);
if (!tcp && !ignoreTruncation && response.getHeader().getFlag(Flags.TC)) {
if (log.isTraceEnabled()) {
log.trace(
Expand Down Expand Up @@ -466,8 +468,8 @@ private Message sendAXFR(Message query) throws IOException {
response.getHeader().setFlag(Flags.AA);
response.getHeader().setFlag(Flags.QR);
response.addRecord(query.getQuestion(), Section.QUESTION);
for (Record record : records) {
response.addRecord(record, Section.ANSWER);
for (Record r : records) {
response.addRecord(r, Section.ANSWER);
}
return response;
}
Expand Down
35 changes: 18 additions & 17 deletions src/main/java/org/xbill/DNS/TSIG.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ public TSIG(Name algorithm, String name, String key) {
* @param key The shared key's data represented as a base64 encoded string.
* @throws IllegalArgumentException The key name is an invalid name
* @throws IllegalArgumentException The key data is improperly encoded
* @see RFC8945
* @see <a href="https://datatracker.ietf.org/doc/html/rfc8945">RFC8945</a>
*/
public TSIG(String algorithm, String name, String key) {
this(algorithmToName(algorithm), name, key);
Expand Down Expand Up @@ -543,15 +543,15 @@ public byte verify(Message m, byte[] b, int length, TSIGRecord old) {
* routine, Message.isVerified() may be called on this message.
*
* @param m The message to verify
* @param b An array containing the message in unparsed form. This is necessary since TSIG signs
* the message in wire format, and we can't recreate the exact wire format (with the same name
* compression).
* @param old If this message is a response, the TSIG from the request
* @param messageBytes An array containing the message in unparsed form. This is necessary since
* TSIG signs the message in wire format, and we can't recreate the exact wire format (with
* the same name compression).
* @param requestTSIG If this message is a response, the TSIG from the request
* @return The result of the verification (as an Rcode)
* @see Rcode
*/
public int verify(Message m, byte[] b, TSIGRecord old) {
return verify(m, b, old, true);
public int verify(Message m, byte[] messageBytes, TSIGRecord requestTSIG) {
return verify(m, messageBytes, requestTSIG, true);
}

/**
Expand All @@ -560,18 +560,18 @@ public int verify(Message m, byte[] b, TSIGRecord old) {
* routine, Message.isVerified() may be called on this message.
*
* @param m The message to verify
* @param b An array containing the message in unparsed form. This is necessary since TSIG signs
* the message in wire format, and we can't recreate the exact wire format (with the same name
* compression).
* @param old If this message is a response, the TSIG from the request
* @param messageBytes An array containing the message in unparsed form. This is necessary since
* TSIG signs the message in wire format, and we can't recreate the exact wire format (with
* the same name compression).
* @param requestTSIG If this message is a response, the TSIG from the request
* @param fullSignature {@code true} if this message is the first of many 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 result of the verification (as an Rcode)
* @see Rcode
* @since 3.2
*/
public int verify(Message m, byte[] b, TSIGRecord old, boolean fullSignature) {
public int verify(Message m, byte[] messageBytes, TSIGRecord requestTSIG, boolean fullSignature) {
m.tsigState = Message.TSIG_FAILED;
TSIGRecord tsig = m.getTSIG();
if (tsig == null) {
Expand All @@ -580,7 +580,8 @@ public int verify(Message m, byte[] b, TSIGRecord old, boolean fullSignature) {

if (!tsig.getName().equals(name) || !tsig.getAlgorithm().equals(alg)) {
log.debug(
"BADKEY failure, expected: {}/{}, actual: {}/{}",
"BADKEY failure on message id {}, expected: {}/{}, actual: {}/{}",
m.getHeader().getID(),
name,
alg,
tsig.getName(),
Expand All @@ -589,8 +590,8 @@ public int verify(Message m, byte[] b, TSIGRecord old, boolean fullSignature) {
}

Mac hmac = initHmac();
if (old != null && tsig.getError() != Rcode.BADKEY && tsig.getError() != Rcode.BADSIG) {
hmacAddSignature(hmac, old);
if (requestTSIG != null && tsig.getError() != Rcode.BADKEY && tsig.getError() != Rcode.BADSIG) {
hmacAddSignature(hmac, requestTSIG);
}

m.getHeader().decCount(Section.ADDITIONAL);
Expand All @@ -603,9 +604,9 @@ public int verify(Message m, byte[] b, TSIGRecord old, boolean fullSignature) {

int len = m.tsigstart - header.length;
if (log.isTraceEnabled()) {
log.trace(hexdump.dump("TSIG-HMAC message after header", b, header.length, len));
log.trace(hexdump.dump("TSIG-HMAC message after header", messageBytes, header.length, len));
}
hmac.update(b, header.length, len);
hmac.update(messageBytes, header.length, len);

DNSOutput out = new DNSOutput();
if (fullSignature) {
Expand Down
47 changes: 17 additions & 30 deletions src/main/java/org/xbill/DNS/tools/jnamed.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@ public jnamed(String conffile) throws IOException, ZoneTransferException {
FileInputStream fs;
InputStreamReader isr;
BufferedReader br;
List<Integer> ports = new ArrayList<Integer>();
List<InetAddress> addresses = new ArrayList<InetAddress>();
List<Integer> ports = new ArrayList<>();
List<InetAddress> addresses = new ArrayList<>();
try {
fs = new FileInputStream(conffile);
isr = new InputStreamReader(fs);
Expand All @@ -76,9 +76,9 @@ public jnamed(String conffile) throws IOException, ZoneTransferException {
}

try {
caches = new HashMap<Integer, Cache>();
caches = new HashMap<>();
znames = new HashMap<>();
TSIGs = new HashMap<Name, TSIG>();
TSIGs = new HashMap<>();

String line;
while ((line = br.readLine()) != null) {
Expand Down Expand Up @@ -127,21 +127,20 @@ public jnamed(String conffile) throws IOException, ZoneTransferException {
}
}

if (ports.size() == 0) {
if (ports.isEmpty()) {
ports.add(53);
}

if (addresses.size() == 0) {
if (addresses.isEmpty()) {
addresses.add(Address.getByAddress("0.0.0.0"));
}

for (Object address : addresses) {
InetAddress addr = (InetAddress) address;
for (Object o : ports) {
int port = (Integer) o;
addUDP(addr, port);
addTCP(addr, port);
System.out.println("jnamed: listening on " + addrport(addr, port));
for (InetAddress address : addresses) {
for (Integer o : ports) {
int port = o;
addUDP(address, port);
addTCP(address, port);
System.out.println("jnamed: listening on " + addrport(address, port));
}
}
System.out.println("jnamed: running");
Expand Down Expand Up @@ -172,12 +171,7 @@ public void addTSIG(String algstr, String namestr, String key) throws IOExceptio
}

public Cache getCache(int dclass) {
Cache c = caches.get(dclass);
if (c == null) {
c = new Cache(dclass);
caches.put(dclass, c);
}
return c;
return caches.computeIfAbsent(dclass, Cache::new);
}

public Zone findBestZone(Name name) {
Expand All @@ -197,7 +191,7 @@ public Zone findBestZone(Name name) {
return null;
}

public <T extends Record> RRset findExactMatch(Name name, int type, int dclass, boolean glue) {
public RRset findExactMatch(Name name, int type, int dclass, boolean glue) {
Zone zone = findBestZone(name);
if (zone != null) {
return zone.findExactMatch(name, type);
Expand All @@ -217,8 +211,7 @@ public <T extends Record> RRset findExactMatch(Name name, int type, int dclass,
}
}

<T extends Record> void addRRset(
Name name, Message response, RRset rrset, int section, int flags) {
void addRRset(Name name, Message response, RRset rrset, int section, int flags) {
for (int s = 1; s <= section; s++) {
if (response.findRRset(name, rrset.getType(), s)) {
return;
Expand Down Expand Up @@ -403,6 +396,7 @@ byte[] doAXFR(Name name, Message query, TSIG tsig, TSIGRecord qtsig, Socket s) {
try {
s.close();
} catch (IOException ex) {
// ignore
}
return null;
}
Expand All @@ -414,7 +408,6 @@ byte[] doAXFR(Name name, Message query, TSIG tsig, TSIGRecord qtsig, Socket s) {
*/
byte[] generateReply(Message query, byte[] in, Socket s) {
Header header;
boolean badversion;
int maxLength;
int flags = 0;

Expand Down Expand Up @@ -515,13 +508,12 @@ public byte[] errorMessage(Message query, int rcode) {
}

public void TCPclient(Socket s) {
try {
try (InputStream is = s.getInputStream()) {
int inLength;
DataInputStream dataIn;
DataOutputStream dataOut;
byte[] in;

InputStream is = s.getInputStream();
dataIn = new DataInputStream(is);
inLength = dataIn.readUnsignedShort();
in = new byte[inLength];
Expand All @@ -544,11 +536,6 @@ public void TCPclient(Socket s) {
} catch (IOException e) {
System.out.println(
"TCPclient(" + addrport(s.getLocalAddress(), s.getLocalPort()) + "): " + e);
} finally {
try {
s.close();
} catch (IOException e) {
}
}
}

Expand Down
Loading