diff --git a/src/main/java/org/apache/commons/net/tftp/TFTPAckPacket.java b/src/main/java/org/apache/commons/net/tftp/TFTPAckPacket.java index 2e1ca54ce..6596f2e1b 100644 --- a/src/main/java/org/apache/commons/net/tftp/TFTPAckPacket.java +++ b/src/main/java/org/apache/commons/net/tftp/TFTPAckPacket.java @@ -52,9 +52,7 @@ public final class TFTPAckPacket extends TFTPPacket { data = datagram.getData(); - if (getType() != data[1]) { - throw new TFTPPacketException("TFTP operator code does not match type."); - } + checkType(data); this.blockNumber = (data[2] & 0xff) << 8 | data[3] & 0xff; } diff --git a/src/main/java/org/apache/commons/net/tftp/TFTPDataPacket.java b/src/main/java/org/apache/commons/net/tftp/TFTPDataPacket.java index 8f97dcb97..07acf827e 100644 --- a/src/main/java/org/apache/commons/net/tftp/TFTPDataPacket.java +++ b/src/main/java/org/apache/commons/net/tftp/TFTPDataPacket.java @@ -66,9 +66,7 @@ public final class TFTPDataPacket extends TFTPPacket { this.data = datagram.getData(); this.offset = 4; - if (getType() != this.data[1]) { - throw new TFTPPacketException("TFTP operator code does not match type."); - } + checkType(data); this.blockNumber = (this.data[2] & 0xff) << 8 | this.data[3] & 0xff; diff --git a/src/main/java/org/apache/commons/net/tftp/TFTPErrorPacket.java b/src/main/java/org/apache/commons/net/tftp/TFTPErrorPacket.java index c25866278..3b568c1c2 100644 --- a/src/main/java/org/apache/commons/net/tftp/TFTPErrorPacket.java +++ b/src/main/java/org/apache/commons/net/tftp/TFTPErrorPacket.java @@ -92,9 +92,7 @@ public final class TFTPErrorPacket extends TFTPPacket { data = datagram.getData(); length = datagram.getLength(); - if (getType() != data[1]) { - throw new TFTPPacketException("TFTP operator code does not match type."); - } + checkType(data); error = (data[2] & 0xff) << 8 | data[3] & 0xff; diff --git a/src/main/java/org/apache/commons/net/tftp/TFTPPacket.java b/src/main/java/org/apache/commons/net/tftp/TFTPPacket.java index c930f4278..2ad76be0f 100644 --- a/src/main/java/org/apache/commons/net/tftp/TFTPPacket.java +++ b/src/main/java/org/apache/commons/net/tftp/TFTPPacket.java @@ -111,7 +111,7 @@ public static final TFTPPacket newTFTPPacket(final DatagramPacket datagram) thro packet = new TFTPErrorPacket(datagram); break; default: - throw new TFTPPacketException("Bad packet. Invalid TFTP operator code."); + throw new TFTPPacketException("Bad packet. Invalid TFTP operator code."); } return packet; } @@ -138,6 +138,12 @@ public static final TFTPPacket newTFTPPacket(final DatagramPacket datagram) thro this.port = port; } + void checkType(final byte[] data) throws TFTPPacketException { + if (getType() != data[1]) { + throw new TFTPPacketException("TFTP operator code does not match type."); + } + } + /** * Gets the address of the host where the packet is going to be sent or where it came from. * diff --git a/src/main/java/org/apache/commons/net/tftp/TFTPRequestPacket.java b/src/main/java/org/apache/commons/net/tftp/TFTPRequestPacket.java index 9a7383b4f..f3dec654d 100644 --- a/src/main/java/org/apache/commons/net/tftp/TFTPRequestPacket.java +++ b/src/main/java/org/apache/commons/net/tftp/TFTPRequestPacket.java @@ -90,70 +90,57 @@ public abstract class TFTPRequestPacket extends TFTPPacket { */ TFTPRequestPacket(final int type, final DatagramPacket datagram) throws TFTPPacketException { super(type, datagram.getAddress(), datagram.getPort()); - final byte[] data = datagram.getData(); - - if (getType() != data[1]) { - throw new TFTPPacketException("TFTP operator code does not match type."); - } - + final int dataLen = datagram.getLength(); + checkType(data); final StringBuilder buffer = new StringBuilder(); - int index = 2; - final int length = datagram.getLength(); - - while (index < length && data[index] != 0) { + while (isChar(data, index)) { buffer.append((char) data[index]); ++index; } - this.fileName = buffer.toString(); - - if (index >= length) { + if (index >= dataLen) { throw new TFTPPacketException("Bad file name and mode format."); } - buffer.setLength(0); ++index; // need to advance beyond the end of string marker - while (index < length && data[index] != 0) { + while (isChar(data, index)) { buffer.append((char) data[index]); ++index; } - final String modeString = buffer.toString().toLowerCase(Locale.ENGLISH); - final int modeStringsLength = modeStrings.length; - + final int modeStringsLen = modeStrings.length; int mode = 0; int modeIndex; - for (modeIndex = 0; modeIndex < modeStringsLength; modeIndex++) { + for (modeIndex = 0; modeIndex < modeStringsLen; modeIndex++) { if (modeString.equals(modeStrings[modeIndex])) { mode = modeIndex; break; } } - this.mode = mode; - - if (modeIndex >= modeStringsLength) { + if (modeIndex >= modeStringsLen) { throw new TFTPPacketException("Unrecognized TFTP transfer mode: " + modeString); // May just want to default to binary mode instead of throwing // exception. - // _mode = TFTP.OCTET_MODE; + // mode = TFTP.OCTET_MODE; } - ++index; - while (index < length) { + while (index < dataLen) { int start = index; - for (; data[index] != 0; ++index) { - if (index >= length) { + while (isChar(data, index)) { + index++; + if (index >= dataLen) { throw new TFTPPacketException("Invalid option format"); } } final String option = new String(data, start, index - start, StandardCharsets.US_ASCII); ++index; start = index; - for (; data[index] != 0; ++index) { - if (index >= length) { + while (isChar(data, index)) { + index++; + if (index >= dataLen) { throw new TFTPPacketException("Invalid option format"); } } @@ -208,6 +195,10 @@ private void handleOptions(final byte[] data, final int fileLength, final int mo } } + private boolean isChar(final byte[] data, int index) { + return index < data.length && data[index] != 0; + } + /** * Creates a UDP datagram containing all the TFTP request packet data in the proper format. This is a method exposed to the programmer in case he wants to * implement his own TFTP client instead of using the {@link org.apache.commons.net.tftp.TFTPClient} class. Under normal circumstances, you should not have @@ -217,28 +208,21 @@ private void handleOptions(final byte[] data, final int fileLength, final int mo */ @Override public final DatagramPacket newDatagram() { - final int fileLength; - final int modeLength; - final byte[] data; - - fileLength = fileName.length(); - modeLength = modeBytes[mode].length; - + final int fileLength = fileName.length(); + final int modeLength = modeBytes[mode].length; int optionsLength = 0; for (final Map.Entry entry : options.entrySet()) { optionsLength += entry.getKey().length() + 1 + entry.getValue().length() + 1; } - data = new byte[fileLength + modeLength + 3 + optionsLength]; + final byte[] data = new byte[fileLength + modeLength + 3 + optionsLength]; data[0] = 0; data[1] = (byte) type; System.arraycopy(fileName.getBytes(Charset.defaultCharset()), 0, data, 2, fileLength); data[fileLength + 2] = 0; System.arraycopy(modeBytes[mode], 0, data, fileLength + 3, modeLength); - if (optionsLength > 0) { handleOptions(data, fileLength, modeLength); } - return new DatagramPacket(data, data.length, address, port); } @@ -252,25 +236,18 @@ public final DatagramPacket newDatagram() { */ @Override final DatagramPacket newDatagram(final DatagramPacket datagram, final byte[] data) { - final int fileLength; - final int modeLength; - - fileLength = fileName.length(); - modeLength = modeBytes[mode].length; - + final int fileLength = fileName.length(); + final int modeLength = modeBytes[mode].length; data[0] = 0; data[1] = (byte) type; System.arraycopy(fileName.getBytes(Charset.defaultCharset()), 0, data, 2, fileLength); data[fileLength + 2] = 0; System.arraycopy(modeBytes[mode], 0, data, fileLength + 3, modeLength); - handleOptions(data, fileLength, modeLength); - datagram.setAddress(address); datagram.setPort(port); datagram.setData(data); datagram.setLength(fileLength + modeLength + 3); - return datagram; } } diff --git a/src/test/java/org/apache/commons/net/tftp/TFTPAckPacketTest.java b/src/test/java/org/apache/commons/net/tftp/TFTPAckPacketTest.java index b4eaedd30..f131d0701 100644 --- a/src/test/java/org/apache/commons/net/tftp/TFTPAckPacketTest.java +++ b/src/test/java/org/apache/commons/net/tftp/TFTPAckPacketTest.java @@ -19,15 +19,22 @@ import static org.junit.jupiter.api.Assertions.assertNotNull; +import java.net.DatagramPacket; import java.net.InetAddress; import java.net.UnknownHostException; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.function.Executable; /** * Tests {@link TFTPAckPacket}. */ -class TFTPAckPacketTest { +class TFTPAckPacketTest extends TFTPPacketTest { + + @Override + protected Executable getDatagramPacketCtor(final DatagramPacket packet) { + return () -> new TFTPAckPacket(packet); + } @Test void testNewDatagram() throws UnknownHostException { diff --git a/src/test/java/org/apache/commons/net/tftp/TFTPDataPacketTest.java b/src/test/java/org/apache/commons/net/tftp/TFTPDataPacketTest.java index 76c758a3a..12e777a0f 100644 --- a/src/test/java/org/apache/commons/net/tftp/TFTPDataPacketTest.java +++ b/src/test/java/org/apache/commons/net/tftp/TFTPDataPacketTest.java @@ -19,15 +19,22 @@ import static org.junit.jupiter.api.Assertions.assertNotNull; +import java.net.DatagramPacket; import java.net.InetAddress; import java.net.UnknownHostException; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.function.Executable; /** * Tests {@link TFTPDataPacket}. */ -class TFTPDataPacketTest { +class TFTPDataPacketTest extends TFTPPacketTest { + + @Override + protected Executable getDatagramPacketCtor(final DatagramPacket packet) { + return () -> new TFTPDataPacket(packet); + } @Test void testNewDatagram() throws UnknownHostException { diff --git a/src/test/java/org/apache/commons/net/tftp/TFTPErrorPacketTest.java b/src/test/java/org/apache/commons/net/tftp/TFTPErrorPacketTest.java index 6183cb7de..6b339fc06 100644 --- a/src/test/java/org/apache/commons/net/tftp/TFTPErrorPacketTest.java +++ b/src/test/java/org/apache/commons/net/tftp/TFTPErrorPacketTest.java @@ -19,15 +19,22 @@ import static org.junit.jupiter.api.Assertions.assertNotNull; +import java.net.DatagramPacket; import java.net.InetAddress; import java.net.UnknownHostException; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.function.Executable; /** * Tests {@link TFTPErrorPacket}. */ -class TFTPErrorPacketTest { +class TFTPErrorPacketTest extends TFTPPacketTest { + + @Override + protected Executable getDatagramPacketCtor(final DatagramPacket packet) { + return () -> new TFTPErrorPacket(packet); + } @Test void testNewDatagram() throws UnknownHostException { @@ -38,4 +45,5 @@ void testNewDatagram() throws UnknownHostException { void testToString() throws UnknownHostException { assertNotNull(new TFTPErrorPacket(InetAddress.getLocalHost(), 0, 0, "").toString()); } + } diff --git a/src/test/java/org/apache/commons/net/tftp/TFTPPacketTest.java b/src/test/java/org/apache/commons/net/tftp/TFTPPacketTest.java new file mode 100644 index 000000000..4f52a5fb8 --- /dev/null +++ b/src/test/java/org/apache/commons/net/tftp/TFTPPacketTest.java @@ -0,0 +1,46 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.commons.net.tftp; + +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.net.DatagramPacket; +import java.net.InetAddress; +import java.net.UnknownHostException; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.function.Executable; + +abstract class TFTPPacketTest { + + protected abstract Executable getDatagramPacketCtor(DatagramPacket packet); + + @Test + public void testConstructorBadType() throws UnknownHostException { + // Create a DatagramPacket with invalid TFTP packet type (not ACK) + final InetAddress address = InetAddress.getLocalHost(); + final byte[] data = new byte[4]; + data[0] = 0; // TFTP opcode 0 (invalid) + data[1] = 0; // TFTP opcode 0 (invalid) + data[2] = 0; // Block number high byte + data[3] = 1; // Block number low byte + final DatagramPacket packet = new DatagramPacket(data, data.length, address, 69); + assertThrows(TFTPPacketException.class, () -> TFTPPacket.newTFTPPacket(packet)); + assertThrows(TFTPPacketException.class, getDatagramPacketCtor(packet)); + } +} diff --git a/src/test/java/org/apache/commons/net/tftp/TFTPReadRequestPacketTest.java b/src/test/java/org/apache/commons/net/tftp/TFTPReadRequestPacketTest.java index c41e7264c..93094574a 100644 --- a/src/test/java/org/apache/commons/net/tftp/TFTPReadRequestPacketTest.java +++ b/src/test/java/org/apache/commons/net/tftp/TFTPReadRequestPacketTest.java @@ -19,15 +19,22 @@ import static org.junit.jupiter.api.Assertions.assertNotNull; +import java.net.DatagramPacket; import java.net.InetAddress; import java.net.UnknownHostException; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.function.Executable; /** * Tests {@link TFTPReadRequestPacket}. */ -class TFTPReadRequestPacketTest { +class TFTPReadRequestPacketTest extends TFTPPacketTest { + + @Override + protected Executable getDatagramPacketCtor(final DatagramPacket packet) { + return () -> new TFTPReadRequestPacket(packet); + } @Test void testToString() throws UnknownHostException { diff --git a/src/test/java/org/apache/commons/net/tftp/TFTPRequestPacketOptionBoundsTest.java b/src/test/java/org/apache/commons/net/tftp/TFTPRequestPacketOptionBoundsTest.java new file mode 100644 index 000000000..8a57f0893 --- /dev/null +++ b/src/test/java/org/apache/commons/net/tftp/TFTPRequestPacketOptionBoundsTest.java @@ -0,0 +1,75 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.commons.net.tftp; + +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertThrowsExactly; + +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.net.DatagramPacket; +import java.net.InetAddress; +import java.nio.charset.StandardCharsets; +import java.util.Arrays; + +import org.junit.jupiter.api.Test; + +/** + * Tests {@link TFTPRequestPacket}. + */ +class TFTPRequestPacketOptionBoundsTest { + + /** + * RRQ for "f" in octet mode, one option whose value has no terminating NUL. + */ + private static byte[] newReadRequest() throws IOException { + final ByteArrayOutputStream out = new ByteArrayOutputStream(); + out.write(0); + out.write(TFTPPacket.READ_REQUEST); + out.write("f".getBytes(StandardCharsets.US_ASCII)); + out.write(0); + out.write("octet".getBytes(StandardCharsets.US_ASCII)); + out.write(0); + out.write("blksize".getBytes(StandardCharsets.US_ASCII)); + out.write(0); + out.write("1024".getBytes(StandardCharsets.US_ASCII)); + return out.toByteArray(); + } + + private static String parse(final byte[] buf, final int len) throws TFTPPacketException { + final DatagramPacket packet = new DatagramPacket(buf, len, InetAddress.getLoopbackAddress(), 69); + return "OK " + ((TFTPRequestPacket) TFTPPacket.newTFTPPacket(packet)).getOptions(); + } + + @Test + void testParseIsIndependentOfBytesBeyondGetLength() throws Exception { + final byte[] request = newReadRequest(); + final byte[] zeroed = Arrays.copyOf(request, request.length + 16); + final byte[] stale = Arrays.copyOf(request, request.length + 16); + Arrays.fill(stale, request.length, stale.length, (byte) 'S'); + assertThrowsExactly(TFTPPacketException.class, () -> parse(zeroed, request.length)); + assertThrowsExactly(TFTPPacketException.class, () -> parse(stale, request.length)); + } + + @Test + void testUnterminatedOptionAtEndOfBufferThrowsDeclaredException() throws Exception { + final byte[] request = newReadRequest(); + final DatagramPacket packet = new DatagramPacket(request, request.length, InetAddress.getLoopbackAddress(), 69); + assertThrows(TFTPPacketException.class, () -> TFTPPacket.newTFTPPacket(packet)); + } +} diff --git a/src/test/java/org/apache/commons/net/tftp/TFTPWriteRequestPacketTest.java b/src/test/java/org/apache/commons/net/tftp/TFTPWriteRequestPacketTest.java index 50796719d..014cb386f 100644 --- a/src/test/java/org/apache/commons/net/tftp/TFTPWriteRequestPacketTest.java +++ b/src/test/java/org/apache/commons/net/tftp/TFTPWriteRequestPacketTest.java @@ -19,19 +19,25 @@ import static org.junit.jupiter.api.Assertions.assertNotNull; +import java.net.DatagramPacket; import java.net.InetAddress; import java.net.UnknownHostException; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.function.Executable; /** - * Tests {@link TFTPReadRequestPacket}. + * Tests {@link TFTPWriteRequestPacket}. */ -class TFTPWriteRequestPacketTest { +class TFTPWriteRequestPacketTest extends TFTPPacketTest { + + @Override + protected Executable getDatagramPacketCtor(final DatagramPacket packet) { + return () -> new TFTPWriteRequestPacket(packet); + } @Test void testToString() throws UnknownHostException { - assertNotNull(new TFTPReadRequestPacket(InetAddress.getLocalHost(), 0, "", 0).toString()); + assertNotNull(new TFTPWriteRequestPacket(InetAddress.getLocalHost(), 0, "", 0).toString()); } - }