Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Filter characters before byte conversion #416

Merged
merged 4 commits into from
Aug 22, 2023
Merged

Filter characters before byte conversion #416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
17d51b5
Filter characters before byte conversion
vismayku Aug 17, 2023
c61ba53
Filter characters before byte conversion
vismayku Aug 17, 2023
b0884d7
Filter characters before byte conversion
vismayku Aug 17, 2023
e391e58
Filter characters before byte conversion
vismayku Aug 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions httpcore5/src/main/java/org/apache/hc/core5/http/impl/nio/SessionOutputBufferImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@
import org.apache.hc.core5.util.Args;
import org.apache.hc.core5.util.CharArrayBuffer;

import static org.apache.hc.core5.util.TextUtils.filterIfRequired;

class SessionOutputBufferImpl extends ExpandableBuffer implements SessionOutputBuffer {

private static final byte[] CRLF = new byte[] {Chars.CR, Chars.LF};
Expand Down Expand Up @@ -171,12 +173,14 @@ public void writeLine(final CharArrayBuffer lineBuffer) throws CharacterCodingEx
final int off = buffer().position();
final int arrayOffset = buffer().arrayOffset();
for (int i = 0; i < len; i++) {
b[arrayOffset + off + i] = (byte) lineBuffer.charAt(i);
final int c = lineBuffer.charAt(i);
b[arrayOffset + off + i] = filterIfRequired(c);
}
buffer().position(off + len);
} else {
for (int i = 0; i < lineBuffer.length(); i++) {
buffer().put((byte) lineBuffer.charAt(i));
final int c = lineBuffer.charAt(i);
buffer().put(filterIfRequired(c));
}
}
} else {
Expand Down
10 changes: 3 additions & 7 deletions httpcore5/src/main/java/org/apache/hc/core5/util/ByteArrayBuffer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@
import java.io.Serializable;
import java.nio.ByteBuffer;

import static org.apache.hc.core5.util.TextUtils.filterIfRequired;

/**
* A resizable byte array.
*
Expand Down Expand Up @@ -139,13 +141,7 @@ public void append(final char[] b, final int off, final int len) {

for (int i1 = off, i2 = oldlen; i2 < newlen; i1++, i2++) {
final int c = b[i1];
if ((c >= 0x20 && c <= 0x7E) || // Visible ASCII
(c >= 0xA0 && c <= 0xFF) || // Visible ISO-8859-1
c == 0x09) { // TAB
this.array[i2] = (byte) c;
} else {
this.array[i2] = '?';
}
this.array[i2] = filterIfRequired(c);
}
this.len = newlen;
}
Expand Down
17 changes: 17 additions & 0 deletions httpcore5/src/main/java/org/apache/hc/core5/util/TextUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
*/

package org.apache.hc.core5.util;
import org.apache.hc.core5.annotation.Internal;

import java.util.Locale;

Expand Down Expand Up @@ -141,4 +142,20 @@
return s.toLowerCase(Locale.ROOT);
}

/**
* Filter characters before byte conversion
*
* @since 5.2
*/
@Internal
public static byte filterIfRequired(final int c) {
if ((c >= 0x20 && c <= 0x7E) || // Visible ASCII
(c >= 0xA0 && c <= 0xFF) || // Visible ISO-8859-1
c == 0x09) { // TAB
return (byte) c;

Check failure

Code scanning / CodeQL

User-controlled data in numeric cast Critical

This cast to a narrower type depends on a
user-provided value
Loading
, potentially causing truncation.
This cast to a narrower type depends on a
user-provided value
Loading
, potentially causing truncation.
Copy link
Member

@arturobernalg arturobernalg Aug 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vismayku I think you need to ensure unexpected truncation by checking the range of the input before performing the cast.

if (c <= 127) {   // Ensure it's within byte range
    return (byte) c;
}

Also, squash your commits into a single one for a cleaner commit history

Copy link
Member

@ok2c ok2c Aug 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vismayku I think you need to ensure unexpected truncation by checking the range of the input before performing the cast.

@arturobernalg I think the existing implementation already does that. All non-printable as well as all non-ascii characters get converted to ?. This is expected behavior.

} else {
return '?';
}
}

}
28 changes: 28 additions & 0 deletions httpcore5/src/test/java/org/apache/hc/core5/http/impl/nio/TestSessionInOutBuffers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,34 @@ public void testWriteLineChunks() throws Exception {
Assertions.assertEquals("One\r\nTwo\r\nThree\r\nFour\r\n", s);
}

@Test
public void testNonASCIIWriteLine() throws Exception {
final String testString = "123\u010Anew-header-from-some-header:injected-value";
final String expectedResult = "123?new-header-from-some-header:injected-value";

final CharArrayBuffer chbuffer = new CharArrayBuffer(32);
final SessionOutputBuffer outbuf = new SessionOutputBufferImpl(1024, 16);

chbuffer.clear();
chbuffer.append(testString);
outbuf.writeLine(chbuffer);

//this write operation should have no effect
outbuf.writeLine(null);

final ByteArrayOutputStream outStream = new ByteArrayOutputStream();
final WritableByteChannel outChannel = newChannel(outStream);
outbuf.flush(outChannel);

final ReadableByteChannel channel = newChannel(outStream.toByteArray());

final SessionInputBuffer inbuf = new SessionInputBufferImpl(1024, 16, 0);
inbuf.fill(channel);
chbuffer.clear();
inbuf.readLine(chbuffer, true);
Assertions.assertEquals(expectedResult, chbuffer.toString());
}

@Test
public void testBasicReadWriteLine() throws Exception {

Expand Down