[Solved] Fix Potential Vulnerability in writeString() function in RmiSerializer.java

Troubleshooting General Help
1

Hi jMonkeyEngine Development Team,

I identified a potential vulnerable due to improper handling of character encoding and formatting in a clone function writeString() in jmonkeyengine/jme3-networking/src/main/java/com/jme3/network/rmi/RmiSerializer.java. This issue was originally reported and resolved in the original repository via this commit UNDERTOW-1302 Fix response splitting flaw if headers contain non-asci… · undertow-io/undertow@85d4478 · GitHub.

Please review at your convenience. Thank you!

CVSS Rating
Score: 5.3
Severity: Medium

Benefits of the Fix

  1. Proper Encoding: Ensures compatibility with non-ASCII characters and prevents encoding issues.
  2. Input Sanitization: Prevents formatting issues and log injection attacks by replacing newline and carriage return characters.
  3. Safe Logging: Ensures that logged strings are sanitized, reducing the risk of log manipulation.

Code Fix

private void writeString(ByteBuffer buffer, String string) throws IOException {
    // Sanitize input to replace newline and carriage return characters
    string = string.replace('\n', ' ').replace('\r', ' ');

    // Encode the string using UTF-8
    byte[] encodedBytes = string.getBytes(StandardCharsets.UTF_8);
    int length = encodedBytes.length;

    // Validate the length of the encoded string
    if (length > 255) {
        logger.log(Level.WARNING, "The string length exceeds the limit! {0} > 255", length);
        buffer.put((byte) 0); // Write zero-length byte
        return;
    }

    // Write the length and the encoded bytes to the buffer
    buffer.put((byte) length);
    buffer.put(encodedBytes);
}

References:

  • CWE-172: Encoding Error
  • CWE-117: Improper Output Neutralization for Logs
1 Like
2

This link: UNDERTOW-1302 Fix response splitting flaw if headers contain non-asci… · undertow-io/undertow@85d4478 · GitHub

…doesn’t appear related to encoding but is adding a corrupter to the code. Casting a char to a byte shouldn’t actually be doing anything here unless you have some \r or \n ‘characters’ that have the high-order bits set for some reason… but then they wouldn’t have been \r or \n and this code is turning the original characters to garbage because of a coincidence that it happens to share 8 bits with \r or \n.

image
image909×347 14.8 KB

This isn’t really how to convert characters to bytes in a reversible way.

As for RmiSerializer, similarly, the breakage is more serious. It needs to be writing the actual character values and not chomping them into bytes. The string that is received on the other end is never going to be the same as the string written with the code the way it is. And your fix doesn’t really fix this either… though it’s close. The reverse would have to be done on the read side to turn it back into characters for the string.

Really, RmiSerializer should probably be deprecated. There is a separate service.rmi layer that does a bunch of things better (though is slightly more limiting to better fit what SpiderMonkey is actually doing). The service.rmi layer defers to StringSerializer for strings… which is already doing the right thing:

3 Likes