Skip to content

Commit 1822f95

Browse files
Adding protection against malformed entries on mapPersister.
Say someone says it has 30K entries while the persister can only generate 8 I'm allowing 100 entries (which should be way beyond necessary to support newer versions).
1 parent b382916 commit 1822f95

3 files changed

Lines changed: 22 additions & 0 deletions

File tree

artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPMessageMetadataPersister.java

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,17 @@ public static AMQPMessageMetadataPersister getInstance() {
5050
protected static final short KEY_EXTRA_PROPERTIES = 8;
5151
protected static final short KEY_IS_REENCODED = 9; // used on large messages only
5252

53+
// This persister cannot generate more elements than defined keys.
54+
// Currently, KEY_IS_REENCODED is the last defined key (9), with KEY_MESSAGE_ID being the first (1).
55+
// Reading beyond 9 elements indicates either invalid/malicious data or a message from a future version.
56+
// We set a reasonable limit to support future versions while protecting the decoder from malformed data.
57+
protected static final short MAX_ELEMENTS = 100;
58+
59+
@Override
60+
protected int getMaxAllowedElements() {
61+
return MAX_ELEMENTS;
62+
}
63+
5364
/** This is the minimum size this codec will generate, as these will always be persisted */
5465
private static final int BASIC_SIZE = headerSize() +
5566
payloadSizeLong() + // messageID

artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/util/AbstractMapPersister.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,8 @@ public static Datatypes fromId(byte id) {
5858
}
5959
}
6060

61+
protected abstract int getMaxAllowedElements();
62+
6163
protected abstract void onMapReadInteger(short key, int value, T decodingObject);
6264
protected abstract void onMapReadByte(short key, byte value, T decodingObject);
6365
protected abstract void onMapReadBoolean(short key, boolean value, T decodingObject);
@@ -152,6 +154,10 @@ public void decode(ActiveMQBuffer buffer, T decodingObject) {
152154
checkReadableBytes(buffer, DataConstants.SIZE_SHORT, endPosition);
153155
int entries = buffer.readUnsignedShort();
154156

157+
if (entries > getMaxAllowedElements()) {
158+
throw new IllegalStateException("Invalid entries size " + entries + " beyond max allowed elements of " + getMaxAllowedElements());
159+
}
160+
155161
for (int i = 0; i < entries; i++) {
156162
// This will check that each entry is valid. If entries is set to an invalid value through malicious data
157163
// this check here would interrupt any further parsing

artemis-protocols/artemis-amqp-protocol/src/test/java/org/apache/activemq/artemis/protocol/amqp/util/MapPersisterTest.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,11 @@ private void calculatePayloadSize() {
6666
encodeSize += headerSize();
6767
}
6868

69+
@Override
70+
protected int getMaxAllowedElements() {
71+
return Short.MAX_VALUE;
72+
}
73+
6974
public int getEncodeSize() {
7075
if (encodeSize == 0) {
7176
calculatePayloadSize();

0 commit comments

Comments
 (0)