Skip to content

Commit de2e6ad

Browse files
acogoluegnesmergify[bot]
acogoluegnes
authored and
mergify[bot]
committed
Reject negative negotiated frame_max
connection.tune's frame_max is parsed via a signed 32-bit read, so a broker can send a value with the high bit set and have it come back negative once negotiated. Unlike negotiatedChannelMax and negotiatedHeartbeat, the negotiated frameMax was never validated afterwards. A negative frameMax reaching Utils.inboundFrameMax defeats the Math.min against maxInboundMessageBodySize, since the negative value is always the smaller one, then trips the same "frame_max <= 0 means no limit" fallback in Utils.framePayloadLimit that frame_max=0 relies on for its legitimate meaning, reintroducing the size-cap bypass without a broker ever needing to negotiate a literal 0. A non-default, positive requestedFrameMax is also enough to reach this on its own, with an otherwise honest broker, since negotiatedMaxValue only launders a negative value back to 0 when one side's value is exactly 0. Reject the negotiated frame max outright when negative, the same way the sibling channel max and heartbeat checks already do. (cherry picked from commit 89ed7ea)
1 parent e7f10bf commit de2e6ad

2 files changed

Lines changed: 68 additions & 0 deletions

File tree

‎src/main/java/com/rabbitmq/client/impl/AMQConnection.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -431,6 +431,11 @@ public void start()
431431
int frameMax =
432432
negotiatedMaxValue(this.requestedFrameMax,
433433
connTune.getFrameMax());
434+
435+
if (frameMax < 0) {
436+
throw new IllegalArgumentException("Negotiated frame max cannot be negative: " + frameMax);
437+
}
438+
434439
this._frameMax = frameMax;
435440

436441
// Inbound payload limit: the smaller of frame_max (less framing

‎src/test/java/com/rabbitmq/client/test/InboundFrameMax.java‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import java.util.concurrent.CountDownLatch;
3232
import java.util.concurrent.TimeUnit;
3333
import java.util.concurrent.atomic.AtomicReference;
34+
import org.junit.jupiter.api.Test;
3435
import org.junit.jupiter.params.ParameterizedTest;
3536
import org.junit.jupiter.params.provider.ValueSource;
3637

@@ -154,6 +155,68 @@ private void doEnforceInboundFrameMaxWithUnlimitedNegotiatedFrameMax(
154155
}
155156
}
156157

158+
// connection.tune's frame_max is parsed as a signed 32-bit value; a broker (or a
159+
// misconfigured client requesting a nonzero frame max) can drive the negotiated value
160+
// negative, which must be rejected rather than silently accepted as a bogus limit.
161+
@Test
162+
void negativeNegotiatedFrameMaxShouldBeRejected() throws Exception {
163+
CountDownLatch serverDone = new CountDownLatch(1);
164+
AtomicReference<Throwable> serverError = new AtomicReference<>();
165+
166+
try (ServerSocket server = new ServerSocket(0, 1, InetAddress.getByName("127.0.0.1"))) {
167+
int port = server.getLocalPort();
168+
Thread peer =
169+
new Thread(
170+
() -> runFakeBrokerWithNegativeFrameMax(server, serverDone, serverError),
171+
"fake-amqp-broker");
172+
peer.setDaemon(true);
173+
peer.start();
174+
175+
ConnectionFactory factory = TestUtils.connectionFactory();
176+
factory.setHost("127.0.0.1");
177+
factory.setPort(port);
178+
// a nonzero requested frame max is needed for negotiatedMaxValue to take the
179+
// Math.min(positive, negative) branch, instead of laundering the broker's negative
180+
// value into 0 ("no limit") the way it would if the client requested 0
181+
factory.setRequestedFrameMax(131_072);
182+
factory.setAutomaticRecoveryEnabled(false);
183+
factory.setHandshakeTimeout(5000);
184+
factory.setConnectionTimeout(5000);
185+
factory.setRequestedHeartbeat(0);
186+
187+
// asserting on the message, not just the type, matters here: on the Netty transport,
188+
// an unguarded negative frame max reaches Netty's frame decoder and trips its own
189+
// IllegalArgumentException ("maxFrameLength ... expected: > 0") for an unrelated
190+
// reason, which would otherwise make this test pass without the fix in place
191+
assertThatThrownBy(() -> factory.newConnection())
192+
.isInstanceOf(IllegalArgumentException.class)
193+
.hasMessageContaining("Negotiated frame max cannot be negative");
194+
}
195+
assertThat(serverDone.await(5, TimeUnit.SECONDS)).isTrue();
196+
assertThat(serverError.get()).isNull();
197+
}
198+
199+
private static void runFakeBrokerWithNegativeFrameMax(
200+
ServerSocket server, CountDownLatch done, AtomicReference<Throwable> error) {
201+
try (Socket socket = server.accept()) {
202+
socket.setSoTimeout(5000);
203+
DataInputStream in = new DataInputStream(socket.getInputStream());
204+
DataOutputStream out = new DataOutputStream(socket.getOutputStream());
205+
206+
byte[] header = new byte[8];
207+
in.readFully(header);
208+
writeMethodFrame(out, startPayload());
209+
readFrame(in);
210+
writeMethodFrame(out, tunePayload(-1));
211+
// the client must reject the negotiated frame max before replying, so no further
212+
// frames are expected
213+
} catch (Throwable t) {
214+
error.set(t);
215+
} finally {
216+
done.countDown();
217+
}
218+
}
219+
157220
private static void runFakeBroker(
158221
ServerSocket server,
159222
CountDownLatch done,

0 commit comments

Comments
 (0)