BitoAgent commented on code in PR #13786:
URL: https://github.com/apache/dubbo/pull/13786#discussion_r1573855686
##########
dubbo-rpc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/h12/grpc/GrpcHttp2ServerTransportListener.java:
##########
@@ -145,39 +146,31 @@ public void onMessage(InputStream inputStream) {
private class DetermineMethodDescriptorListener implements
StreamingDecoder.FragmentListener {
- @Override
- public void onFragmentMessage(InputStream rawMessage) {}
-
@Override
public void onClose() {
getStreamingDecoder().close();
}
@Override
- public void onFragmentMessage(InputStream dataHeader, InputStream
rawMessage) {
+ public void onFragmentMessage(InputStream rawMessage) {
try {
- ByteArrayOutputStream merged =
- new ByteArrayOutputStream(dataHeader.available() +
rawMessage.available());
- StreamUtils.copy(dataHeader, merged);
- byte[] data = StreamUtils.readBytes(rawMessage);
-
RpcInvocationBuildContext context = getContext();
if (null == context.getMethodDescriptor()) {
-
context.setMethodDescriptor(DescriptorUtils.findTripleMethodDescriptor(
- context.getServiceDescriptor(),
context.getMethodName(), data));
+ byte[] data = StreamUtils.readBytes(rawMessage);
+ MethodDescriptor methodDescriptor =
DescriptorUtils.findTripleMethodDescriptor(
+ context.getServiceDescriptor(),
context.getMethodName(), data);
+ context.setMethodDescriptor(methodDescriptor);
setHttpMessageListener(GrpcHttp2ServerTransportListener.super.buildHttpMessageListener());
// replace decoder
GrpcCompositeCodec grpcCompositeCodec =
(GrpcCompositeCodec) context.getHttpMessageDecoder();
- MethodMetadata methodMetadata =
context.getMethodMetadata();
-
grpcCompositeCodec.setDecodeTypes(methodMetadata.getActualRequestTypes());
- grpcCompositeCodec.setEncodeTypes(new Class[]
{methodMetadata.getActualResponseType()});
+ grpcCompositeCodec.loadPackableMethod(methodDescriptor);
getServerChannelObserver().setResponseEncoder(grpcCompositeCodec);
+ rawMessage = new ByteArrayInputStream(data);
}
- merged.write(data);
- getHttpMessageListener().onMessage(new
ByteArrayInputStream(merged.toByteArray()));
+ getStreamingDecoder().invokeListener(rawMessage);
} catch (IOException e) {
throw new DecodeException(e);
}
Review Comment:
**Security Issue**: The code does not sanitize or validate input before
processing it, which may lead to injection vulnerabilities, such as SQL
Injection, Command Injection, or similar when the input is used in a
security-sensitive context. <br> **Fix**: Implement input validation and
sanitization to ensure the rawMessage data is safe to process. Use a whitelist
approach where only known good data is accepted. <br> **Code Suggestion**:
```
InputStream validatedStream = sanitizeInputStream(rawMessage);
if (validateInputStream(validatedStream)) {
byte[] data = StreamUtils.readBytes(validatedStream);
MethodDescriptor methodDescriptor =
DescriptorUtils.findTripleMethodDescriptor(context.getServiceDescriptor(),
context.getMethodName(), data);
context.setMethodDescriptor(methodDescriptor);
}
```
##########
dubbo-remoting/dubbo-remoting-http12/src/main/java/org/apache/dubbo/remoting/http12/message/LengthFieldStreamingDecoder.java:
##########
@@ -130,16 +127,12 @@ private void deliver() {
}
private void processHeader() throws IOException {
- ByteArrayOutputStream bos = new
ByteArrayOutputStream(lengthFieldOffset + lengthFieldLength);
byte[] offsetData = new byte[lengthFieldOffset];
int ignore = accumulate.read(offsetData);
- bos.write(offsetData);
processOffset(new ByteArrayInputStream(offsetData), lengthFieldOffset);
byte[] lengthBytes = new byte[lengthFieldLength];
ignore = accumulate.read(lengthBytes);
- bos.write(lengthBytes);
requiredLength = bytesToInt(lengthBytes);
- this.dataHeader = new ByteArrayInputStream(bos.toByteArray());
// Continue reading the frame body.
state = DecodeState.PAYLOAD;
Review Comment:
**Security Issue**: The implementation of the method 'processHeader' does
not properly validate the length of the data being processed. This can lead to
buffer overflow vulnerabilities if the data exceeds expected bounds. <br>
**Fix**: Implement length checks for 'offsetData' and 'lengthBytes' to ensure
they do not exceed predefined safe limits. If the data exceeds these limits,
throw an IOException or a custom exception that can be properly handled. <br>
**Code Suggestion**:
```
private void processHeader() throws IOException {
byte[] offsetData = new byte[lengthFieldOffset];
int ignore = accumulate.read(offsetData);
if(offsetData.length > MAX_OFFSET_DATA_LENGTH) throw new
IOException("Offset data exceeds allowed limit.");
processOffset(new ByteArrayInputStream(offsetData), lengthFieldOffset);
byte[] lengthBytes = new byte[lengthFieldLength];
ignore = accumulate.read(lengthBytes);
if(lengthBytes.length > MAX_LENGTH_BYTES) throw new IOException("Length
bytes exceed allowed limit.");
requiredLength = bytesToInt(lengthBytes);
}
```
--
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: notifications-unsubscr...@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@dubbo.apache.org
For additional commands, e-mail: notifications-h...@dubbo.apache.org
