gnodet-bot commented on code in PR #28846:
URL: https://github.com/apache/camel/pull/28846#discussion_r4440883019
##########
components/camel-syslog/src/main/java/org/apache/camel/component/syslog/netty/Rfc5425FrameDecoder.java:
##########
@@ -33,33 +32,29 @@ public class Rfc5425FrameDecoder extends
ByteToMessageDecoder implements Channel
protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object>
out) throws Exception {
if (currentFramelength == null) {
// find index of the first space, it should be after the length
field
- int index = indexOf(in, Unpooled.wrappedBuffer(new byte[] { ' '
}));
-
- // Read part until the first space, if we have found one
- StringBuilder lengthbuffer = new StringBuilder();
- if (index > -1) {
- ByteBuf byteBuf = in.readBytes(index);
- byte[] dest = new byte[byteBuf.readableBytes()];
- byteBuf.readBytes(dest);
- lengthbuffer.append(new String(dest));
+ int spaceIndex = in.indexOf(in.readerIndex(), in.writerIndex(),
(byte) ' ');
+ if (spaceIndex < 0) {
+ // The length delimiter has not arrived yet, wait for more
data without modifying reader index
+ return;
}
- int length;
- try {
- // add one because we have to take in account the space after
- // the length field
- length = Integer.parseInt(lengthbuffer.toString()) + 1;
- } catch (NumberFormatException e) {
- length = -1;
+ int lengthDigits = spaceIndex - in.readerIndex();
+ if (lengthDigits == 0) {
+ throw new IllegalArgumentException("Invalid RFC 5425 frame:
empty length field");
}
- // We have not found the length field, reset the buffer so we can
- // retry next time
- if (length < 0) {
- in.resetReaderIndex();
- return;
+ int length = 0;
+ for (int i = 0; i < lengthDigits; i++) {
+ byte b = in.getByte(in.readerIndex() + i);
+ if (b < '0' || b > '9') {
+ throw new IllegalArgumentException("Invalid RFC 5425
frame: non-digit character in length field");
+ }
+ length = length * 10 + (b - '0');
Review Comment:
⚠️ **Security: silent integer overflow on adversarial input**
The loop accumulates digits without bounds checking. A crafted length field
like `2147483648` (or any value ≥ 2³¹) silently wraps the `int`, producing a
negative or small `currentFramelength`. Downstream,
`in.readBytes(lengthToRead)` will then extract the wrong number of bytes or
throw unexpectedly.
The old `Integer.parseInt` path raised `NumberFormatException` for
out-of-range values. This new path is a regression for overflow. Add a guard
after accumulating:
```suggestion
length = length * 10 + (b - '0');
if (length < 0) {
throw new IllegalArgumentException("Invalid RFC 5425
frame: length field overflows int");
}
```
##########
components/camel-syslog/src/test/java/org/apache/camel/component/syslog/netty/Rfc5425FrameDecoderTest.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * 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
+ *
+ * http://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.camel.component.syslog.netty;
+
+import java.nio.charset.StandardCharsets;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.Unpooled;
+import io.netty.channel.embedded.EmbeddedChannel;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+class Rfc5425FrameDecoderTest {
+
+ @Test
+ void testFragmentedLengthFieldDoesNotDuplicateFrames() {
+ EmbeddedChannel channel = new EmbeddedChannel(new
Rfc5425FrameDecoder());
+
+ // Write first message and partial length of next message: "3 one5"
+ assertTrue(channel.writeInbound(Unpooled.copiedBuffer("3 one5",
StandardCharsets.UTF_8)));
+
+ // Exactly one frame (" one") should be produced
+ ByteBuf frame1 = channel.readInbound();
+ assertNotNull(frame1);
+ assertEquals(" one", frame1.toString(StandardCharsets.UTF_8));
+ frame1.release();
+
+ // No duplicate frame should be produced
+ assertNull(channel.readInbound());
+
+ // Write remainder of second message: " three"
+ assertTrue(channel.writeInbound(Unpooled.copiedBuffer(" three",
StandardCharsets.UTF_8)));
+
+ // Second frame (" three") should be produced
+ ByteBuf frame2 = channel.readInbound();
+ assertNotNull(frame2);
+ assertEquals(" three", frame2.toString(StandardCharsets.UTF_8));
+ frame2.release();
+
+ assertNull(channel.readInbound());
+ assertFalse(channel.finish());
+ }
+
+ @Test
+ void testFragmentedLengthDigitsAcrossPackets() {
+ EmbeddedChannel channel = new EmbeddedChannel(new
Rfc5425FrameDecoder());
+
+ // First packet contains only partial length digit: "1"
+ assertFalse(channel.writeInbound(Unpooled.copiedBuffer("1",
StandardCharsets.UTF_8)));
+ assertNull(channel.readInbound());
+
+ // Second packet contains remaining length digit, space, and body: "1
hello world"
+ assertTrue(channel.writeInbound(Unpooled.copiedBuffer("1 hello world",
StandardCharsets.UTF_8)));
+
+ ByteBuf frame = channel.readInbound();
+ assertNotNull(frame);
+ assertEquals(" hello world", frame.toString(StandardCharsets.UTF_8));
+ frame.release();
+
+ assertNull(channel.readInbound());
+ assertFalse(channel.finish());
+ }
Review Comment:
💡 **Missing tests for the new error paths**
The two new tests cover fragmentation — good. But the code also introduces
two new `IllegalArgumentException` paths (`empty length field`, `non-digit
character in length field`) that have no test coverage. Consider adding:
```java
@Test
void testEmptyLengthFieldThrows() {
EmbeddedChannel channel = new EmbeddedChannel(new Rfc5425FrameDecoder());
assertThrows(IllegalArgumentException.class,
() -> channel.writeInbound(Unpooled.copiedBuffer(" hello",
StandardCharsets.UTF_8)));
}
@Test
void testNonDigitInLengthFieldThrows() {
EmbeddedChannel channel = new EmbeddedChannel(new Rfc5425FrameDecoder());
assertThrows(IllegalArgumentException.class,
() -> channel.writeInbound(Unpooled.copiedBuffer("abc hello",
StandardCharsets.UTF_8)));
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
