Add validation for WireFormatInfo#2080
Conversation
The max number of properties in the map for the WireFormatInfo command was lowered to 64 and a new max value buffer size was added to validate buffers created during unmarshalling of the properties map. The max buffer allowed has been set to 512 bytes. This also adds a maxDepth check for handling nested collections inside the map. It is set to 0 which will block list/map from being used as a value for a property.
cshannon
force-pushed
the
wfi-buffer-validation
branch
from
9575d3f to
f2c842b
Compare
|
|
||
| public static String readUTF8(DataInput dataIn) throws IOException { | ||
| int utflen = dataIn.readInt(); | ||
| return readUTF8(dataIn, Integer.MAX_VALUE); |
There was a problem hiding this comment.
This one is a bit dangerous so should check on who is calling it. I might actually pass a value of dataIn.available() unless there is a case where this can be used on blocking reads like a read from a socket input stream.
There was a problem hiding this comment.
That is a good point, I agree this could be looked into for improvement as well. This is a utility class and method so I would need to look where else it could be used.
This particular method wasn't touched and is used at all by WireFormatInfo so I don't think it needs to be changed now. It could be used in the future by various use cases, also there's a chance (and likely) the stream could be a compressed stream which would impact the available() call as well. So I can look into it as a separate follow on.
|
|
||
| public static Object unmarshalPrimitive(DataInputStream in) throws IOException { | ||
| return unmarshalPrimitive(in, false); | ||
| return unmarshalPrimitive(in, false, Integer.MAX_VALUE, Integer.MAX_VALUE, Integer.MAX_VALUE, 0); |
There was a problem hiding this comment.
Another case where I'd consider if the passed size limit could be the stream.available()
|
|
||
| public static Map<String, Object> unmarshalPrimitiveMap(DataInputStream in) throws IOException { | ||
| return unmarshalPrimitiveMap(in, Integer.MAX_VALUE); | ||
| return unmarshalPrimitiveMap(in, Integer.MAX_VALUE, Integer.MAX_VALUE, Integer.MAX_VALUE); |
There was a problem hiding this comment.
If possible consider capping the max size from these methods to in.available() to that if the encoded size is bogus you will fail early vs large allocation attempt. Unsure if this is possible if these are read via a socket input stream which is expected to block on reads waiting more bytes
There was a problem hiding this comment.
Same as other 2 comments to close the loop, I'll investigate as a follow on and look at other places using the util.
The max number of properties in the map for the WireFormatInfo command was lowered to 64 and a new max value buffer size was added to validate buffers created during unmarshalling of the properties map. The max buffer allowed has been set to 512 bytes. This also adds a maxDepth check for handling nested collections inside the map. It is set to 0 which will block list/map from being used as a value for a property. (cherry picked from commit 6fc46e5)
The max number of properties in the map for the WireFormatInfo command was lowered to 64 and a new max value buffer size was added to validate buffers created during unmarshalling of the properties map. The max buffer allowed has been set to 512 bytes. This also adds a maxDepth check for handling nested collections inside the map. It is set to 0 which will block list/map from being used as a value for a property. (cherry picked from commit 6fc46e5)
2 participants
The max number of properties in the map for the WireFormatInfo command was lowered to 64 and a new max value buffer size was added to validate buffers created during unmarshalling of the properties map. The max buffer allowed has been set to 512 bytes.
This also adds a maxDepth check for handling nested collections inside the map. It is set to 0 which will block list/map from being used as a value for a property.