Repository: netty/netty (Java)

Issue:
Unexpected characters are synthesized by DefaultHttp2HeadersDecoder when Huffman encoding is enabled in HpackEncoder
### What happened?
1. Create a header `DefaultHttp2Headers` with a certain value that exceeds certain size and contain non-ASCII characters, 
2. Encode the header using `DefaultHttp2HeadersEncoder`.
3. Decode the encoded header using `DefaultHttp2HeadersDecoder`.
4. If Huffman encoding is enabled in the `HpackDecoder` (used inside `DefaultHttp2HeadersDecoder`), unexpected character(s) magically show up in the decoded string. 

"Unexpected characters" means:
1. They are not in the original header value which is encoded.
2. Their unicode code points are smaller than 32.

Please note that different header value input can yield different unexpected characters. In the below repro code, `NULL (U+0000)` is the unexpected character. We also have header value (which contains company-specific information) that would yield `\r` as unexpected characters.

IMO the first thing is to confirm that there is indeed a bug in the HPACK implementation (and/or any other components).

### Steps to reproduce
Please refer to the below code.

### Minimal yet complete reproducer code (or URL to code)
```scala
test("NULL character is magically synthesized by DefaultHttp2HeadersDecoder when Huffman encoding is enabled in HpackEncoder") {
 import io.netty.handler.codec.http2.DefaultHttp2Headers
 import io.netty.handler.codec.http2.DefaultHttp2HeadersEncoder
 import io.netty.buffer.Unpooled
 import io.netty.handler.codec.http2.DefaultHttp2HeadersDecoder

 val headers = new DefaultHttp2Headers
 val badHeaderValue =
   "{\"xxxx\":\"xxxxxxxxxxxxxxxxxxxxxxxxxxxx\",\"xxxxxxx\":\"xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\",\"xxxx\":\"x\",\"xxxxxxx\":\"xxxxxxxxxxxxxxxxxxxxxxxxxx\",\"xxxxxxxxxxxxxx\":\"我们在Netty中发现了一个bug_ми знайшли проблему в_123мынашлипроблемув_우리는에서문제를발견했습니다Saturday10311031UsersVlad@Alex_ver10311031Sharedで問題が見つかりましたIAPKI_ActivityVlad@Alex우리는에서문제를발견했습니다BatchWeFound_10311031UsersVlad@Alex_ver10311031Shared\",\"xxxxxx\":\"xxxxxxxx\",\"xxxxxxxx\":\"xxxxxx-87\",\"xxxxxxxxx\":\"xxxxxx.xxxxxx.xxxxxx.xxxxxx.24hgjbu2g2hgwoejf231Xxxxxxx/Xxxx\"}"
 headers.set("some-key", badHeaderValue)

 val neverSensitive: SensitivityDetector = new SensitivityDetector() {
   override def isSensitive(name: CharSequence, value: CharSequence) = false
 }
 val enableHuffmanEncoding = true
 // Use the below threshold to control whether Huffman encoding is enabled for the above header value at runtime.
 val huffCodeThreshold: Int = if (enableHuffmanEncoding) 512 else 51200

 val encoder = new DefaultHttp2HeadersEncoder(
   neverSensitive,
   false, // ignoreMaxHeaderListSize, same value as default.
   64, // dynamicTableArraySizeHint, same value as default.
   huffCodeThreshold
 )
 val buf = Unpooled.buffer(10000)
 encoder.encodeHeaders(1, headers, buf)

 val decoder = new DefaultHttp2HeadersDecoder()
 val decodedHeader = decoder.decodeHeaders(1, buf)
 val decodedHeaderValue: String = decodedHeader.get("some-key").toString
 val nullChar: Char = 0.toChar

 assert(!badHeaderValue.contains(nullChar)) // `Null` is not in the original header value.
 if (enableHuffmanEncoding) {
   // 👇👇👇 Unexpected 'Null' character gets synthesized in the header value 🫢
   assert(decodedHeaderValue.contains(nullChar))
 } else {
   // Huffman encoding is disabled.
   assert(!decodedHeaderValue.contains(nullChar))
 }

 val scalaVersion: String = scala.util.Properties.scalaPropOrElse("version.number", "unknown")
 println(s"Scala version: $scalaVersion") // 2.12.15
 val javaVersion: String = System.getProperty("java.version")
 println(s"Java version number: $javaVersion") // 11.0.10
 val javaVendor: String = System.getProperty("java.vendor")
 println(s"Java vendor: $javaVendor") // Azul Systems, Inc
}
```

### Netty version
`4.1.86.Final`

### JVM version (e.g. `java -version`)
```
Scala version: 2.12.15
Java version number: 11.0.10
Java vendor: Azul Systems, Inc
```


Hints:
I have reproduced it and found that the decoded value is **not equal** to the original value whenever the `enableHuffmanEncoding` is true or false. And I found a comment in `HpackEncoder.java`: Only **ASCII** is allowed in http2 headers(https://tools.ietf.org/html/rfc7540#section-8.1.2), so if there exists unexpected characters, unexpected errors may happen. 
1. support charset ISO-8859-1 
<img width="804" alt="image" src="https://github.com/netty/netty/assets/23135058/56b43e66-f645-4a34-bf2b-9d64fab5c421">
2. support characters with one byte 
<img width="517" alt="image" src="https://github.com/netty/netty/assets/23135058/8b4f6d0d-6574-4804-bb31-345278cabf39">

In your case, your headers contains invalid character, which is not in ISO-8859-1 charset or more than one byte.



Thanks for looking into this @tmyVvit! You're totally right that you're not supposed to use non-US-ASCII chars in the the header values but the interesting bit is that, even if you do, something in the decoder/encoder chain normally does sanitization of the string and replaces characters that are outside of allowed range with `?`. Here is a quick REPL demo:

```scala
@ val buf = Unpooled.buffer(10000) 
buf: io.netty.buffer.ByteBuf = UnpooledByteBufAllocator$InstrumentedUnpooledUnsafeHeapByteBuf(ridx: 0, widx: 0, cap: 10000)

@ headers.set("some-key", "hello 우리는에서문제를발견했습니다world") 
res14: io.netty.handler.codec.http2.Http2Headers = DefaultHttp2Headers[some-key: hello 우리는에서문제를발견했습니다world]

@ encoder.encodeHeaders(1, headers, buf) 


@  val decodedHeader = decoder.decodeHeaders(1, buf) 
decodedHeader: io.netty.handler.codec.http2.Http2Headers = DefaultHttp2Headers[some-key: hello ??????????????world, some-key: hello ??????????????world]

@  val decodedHeaderValue = decodedHeader.get("some-key") 
decodedHeaderValue: CharSequence = hello ??????????????world
```

Something in that chain is obviously not doing the sanitization right when a) Huffman encoding is ON and b) the string contains certain UTF-8 chars in the certain positions.
@tmyVvit is right: it's the `一` char at index 195 that is turning onto a `0x0`. The only reason it doesn't turn into a `0x0` when not using huffman encoding is because of the `.getBytes(IO_8859_1)` call instead of doing what huffman encoding does and doing a simple truncation via `string.charAt(i) & 0xFF`. Despite not being a `0x0`, it's a certainty that the resulting value is still not what you wanted.

@vkostyukov, can you provide a complete example? I'm not getting the same results as you. It may depend on how the `DefaultHttp2Headers` is instantiated. Specifically, when I run
```
Http2Headers headers = new DefaultHttp2Headers(true, true, 16);
headers.set("some-key", "hello 우리는에서문제를발견했습니다world");

DefaultHttp2HeadersEncoder encoder = new DefaultHttp2HeadersEncoder();
ByteBuf buf = Unpooled.buffer(10000);
encoder.encodeHeaders(1, headers, buf);

Http2HeadersDecoder decoder = new DefaultHttp2HeadersDecoder();
Http2Headers decodedHeader = decoder.decodeHeaders(1, buf);
throw new Exception("Value:" + decodedHeader.get("some-key"));
// results in java.lang.Exception: Value:hello °¬Ð8|¬µÈäworld
```

What you all but certainly want is eager validation when the header is added. Unfortunately, and it doesn't seem to be what you're seeing with the '?'s, but it does look like the default validation is broken [here](https://github.com/netty/netty/blob/4.1/codec-http/src/main/java/io/netty/handler/codec/http/HttpHeaderValidationUtil.java#L143), specifically chars wider than 1 byte will all be considered 'ok' which is wrong.
re: https://github.com/netty/netty/issues/13540#issuecomment-1671799091

@tmyVvit @bryce-anderson what I find a bit confusing in the comment referenced above - https://datatracker.ietf.org/doc/html/rfc7540#section-8.1.2 says

> header **field names** are strings of ASCII

which covers header names, but it doesn't say anything about header **field values**, which sidetracked us into thinking that field values with non-US-ASCII chars are not illegal, and there's a bug in the `HpackEncoder.java`. would it make sense to update the comment in the `HpackEncoder.java` to clarify that and add more context? after re-reading the spec - think it's covered by https://datatracker.ietf.org/doc/html/rfc7540#section-10.3 which points to the "field-content" ABNF rule in https://datatracker.ietf.org/doc/html/rfc7230#section-3.2, and https://datatracker.ietf.org/doc/html/rfc7230#section-3.2.4 which says:

> Newly defined header fields SHOULD limit their field values to
   US-ASCII octets.  A recipient SHOULD treat other octets in field
   content (obs-text) as opaque data

I think some clarification would be helpful for posterity.
It actually looks like [this code path](https://github.com/netty/netty/blob/4.1/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackEncoder.java#L308C17-L310C71) (when Huffman encoding is bypassed) works in our favor, if you will.

```java
// Only ASCII is allowed in http2 headers, so its fine to use this.
// https://tools.ietf.org/html/rfc7540#section-8.1.2
out.writeCharSequence(string, CharsetUtil.ISO_8859_1);
```

This end up calling into `ByteBufUtil.writeAscii` [here](https://github.com/netty/netty/blob/4.1/buffer/src/main/java/io/netty/buffer/AbstractByteBuf.java#L717), which performs sanitization with the `?` character [here](https://github.com/netty/netty/blob/4.1/buffer/src/main/java/io/netty/buffer/ByteBufUtil.java#L1222-L1229). Relevant function is `AsciiString.c2b`

```java
public static byte c2b(char c) {
  return (byte) ((c > MAX_CHAR_VALUE) ? '?' : c);
}
```

Perhaps we should re-use the same conversion function in `HpackHuffmanEncoder` [here](https://github.com/netty/netty/blob/4.1/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackHuffmanEncoder.java#L90) (instead of `data.charAt(i) & 0xFF`) to make sanitization behavior consistent whether or not Huffman encoding is engaged. 
> which performs sanitization with the ? character [here](https://github.com/netty/netty/blob/4.1/buffer/src/main/java/io/netty/buffer/ByteBufUtil.java#L1222-L1229)

+1 to perform sanitization over making non-US-ASCII characters [prohibited](https://github.com/netty/netty/blob/7127e6074c2f4b965305738d7756d196729b493e/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Headers.java#L91-L92) - IMO that may break Netty users who happened to put non-US-ASCII chars in the header values without hitting this issue. if there's insufficient test coverage  - making chars prohibited will start failing their requests at runtime causing outages. sanitizing them seems less damaging. as a precedent - Jetty sanitizes non-US-ASCII chars by replacing them with spaces ([ref](https://github.com/eclipse/jetty.project/blob/ad0b5cfa1533e83814c74063b6c2e17b099e5796/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java#L894-L906)). given that spec uses "SHOULD" when suggesting to restricting chars in field values to US-ASCII octets - I think it may be a reasonable option to sanitize and limit prohibited chars validations to the chars which are prohibited for security reasons https://datatracker.ietf.org/doc/html/rfc7540#section-10.3.
> Perhaps we should re-use the same conversion function in HpackHuffmanEncoder [here](https://github.com/netty/netty/blob/4.1/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackHuffmanEncoder.java#L90) (instead of data.charAt(i) & 0xFF) to make sanitization behavior consistent whether or not Huffman encoding is engaged.

I think that makes sense, [here](https://github.com/netty/netty/blob/4.1/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackHuffmanEncoder.java#L136) would also need to be fixed. Would one of you be interested in putting up a PR?

> +1 to perform sanitization over making non-US-ASCII characters [prohibited](https://github.com/netty/netty/blob/7127e6074c2f4b965305738d7756d196729b493e/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Headers.java#L91-L92) - IMO that may break Netty users who happened to put non-US-ASCII chars in the header values without hitting this issue.

These are not mutually exclusive: the validation is optional (and disabled by default) and happens on header insertion which is a nice feature since it lets you figure out what code is adding illegal headers so you can fix you app. If we don't fix the validation the feature will remain forever broken.
>I think that makes sense, [here](https://github.com/netty/netty/blob/4.1/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackHuffmanEncoder.java#L136) would also need to be fixed. Would one of you be interested in putting up a PR?

I will give it a shot (putting up a PR)! @bryce-anderson @vkostyukov 

Patch:
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackEncoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackEncoder.java
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackEncoder.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackEncoder.java
@@ -305,7 +305,7 @@ private void encodeStringLiteral(ByteBuf out, CharSequence string) {
                 AsciiString asciiString = (AsciiString) string;
                 out.writeBytes(asciiString.array(), asciiString.arrayOffset(), asciiString.length());
             } else {
-                // Only ASCII is allowed in http2 headers, so its fine to use this.
+                // Only ASCII is allowed in http2 headers, so it is fine to use this.
                 // https://tools.ietf.org/html/rfc7540#section-8.1.2
                 out.writeCharSequence(string, CharsetUtil.ISO_8859_1);
             }
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackHuffmanEncoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackHuffmanEncoder.java
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackHuffmanEncoder.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackHuffmanEncoder.java
@@ -87,7 +87,7 @@ private void encodeSlowPath(ByteBuf out, CharSequence data) {
         int n = 0;
 
         for (int i = 0; i < data.length(); i++) {
-            int b = data.charAt(i) & 0xFF;
+            int b = AsciiString.c2b(data.charAt(i)) & 0xFF;
             int code = codes[b];
             int nbits = lengths[b];
 
@@ -133,7 +133,7 @@ int getEncodedLength(CharSequence data) {
     private int getEncodedLengthSlowPath(CharSequence data) {
         long len = 0;
         for (int i = 0; i < data.length(); i++) {
-            len += lengths[data.charAt(i) & 0xFF];
+            len += lengths[AsciiString.c2b(data.charAt(i)) & 0xFF];
         }
         return (int) (len + 7 >> 3);
     }


Test patch:
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackEncoderTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackEncoderTest.java
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackEncoderTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackEncoderTest.java
@@ -204,6 +204,48 @@ public void testManyHeaderCombinations() throws Http2Exception {
         }
     }
 
+    @Test
+    public void testSanitization() throws Http2Exception {
+        final int headerValueSize = 300;
+        StringBuilder actualHeaderValueBuilder = new StringBuilder();
+        StringBuilder expectedHeaderValueBuilder = new StringBuilder();
+
+        for (int i = 0; i < headerValueSize; i++) {
+            actualHeaderValueBuilder.append((char) i); // Use the index as the code point value of the character.
+            if (i <= 255) {
+                expectedHeaderValueBuilder.append((char) i);
+            } else {
+                expectedHeaderValueBuilder.append('?'); // Expect this character to be sanitized.
+            }
+        }
+        String actualHeaderValue = actualHeaderValueBuilder.toString();
+        String expectedHeaderValue = expectedHeaderValueBuilder.toString();
+        HpackEncoder encoderWithHuffmanEncoding =
+                new HpackEncoder(false, 64, 0); // Low Huffman code threshold.
+        HpackEncoder encoderWithoutHuffmanEncoding =
+                new HpackEncoder(false, 64, Integer.MAX_VALUE); // High Huffman code threshold.
+
+        // Expect the same decoded header value regardless of whether Huffman encoding is enabled or not.
+        verifyHeaderValueSanitization(encoderWithHuffmanEncoding, actualHeaderValue, expectedHeaderValue);
+        verifyHeaderValueSanitization(encoderWithoutHuffmanEncoding, actualHeaderValue, expectedHeaderValue);
+    }
+
+    private void verifyHeaderValueSanitization(
+            HpackEncoder encoder,
+            String actualHeaderValue,
+            String expectedHeaderValue
+    ) throws Http2Exception {
+
+        String headerKey = "some-key";
+        Http2Headers toBeEncodedHeaders = new DefaultHttp2Headers().add(headerKey, actualHeaderValue);
+        encoder.encodeHeaders(0, buf, toBeEncodedHeaders, Http2HeadersEncoder.NEVER_SENSITIVE);
+        DefaultHttp2Headers decodedHeaders = new DefaultHttp2Headers();
+        hpackDecoder.decode(0, buf, decodedHeaders, true);
+        buf.clear();
+        String decodedHeaderValue = decodedHeaders.get(headerKey).toString();
+        Assertions.assertEquals(expectedHeaderValue, decodedHeaderValue);
+    }
+
     private void setMaxTableSize(int maxHeaderTableSize) throws Http2Exception {
         hpackEncoder.setMaxHeaderTableSize(buf, maxHeaderTableSize);
         hpackDecoder.setMaxHeaderTableSize(maxHeaderTableSize);
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackHuffmanTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackHuffmanTest.java
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackHuffmanTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackHuffmanTest.java
@@ -37,9 +37,11 @@
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.function.Executable;
 
+import java.util.Arrays;
 import java.util.Random;
 
 import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
@@ -162,6 +164,39 @@ public void execute() throws Throwable {
         });
     }
 
+    @Test
+    public void testEncoderSanitizingMultiByteCharacters() throws Http2Exception {
+        final int inputLen = 500;
+        StringBuilder sb = new StringBuilder();
+        for (int i = 0; i < inputLen; i++) {
+            // Starts with 0x4E01 because certain suboptimal sanitization could cause some problem with this input.
+            // For example, if a multibyte character C is sanitized by doing (C & OxFF), if C == 0x4E01, then
+            // (0x4E01 & OxFF) is greater than zero which indicates insufficient sanitization.
+            sb.append((char) (0x4E01 + i));
+        }
+        HpackHuffmanEncoder encoder = new HpackHuffmanEncoder();
+        String toBeEncoded = sb.toString();
+        ByteBuf buffer = Unpooled.buffer();
+        byte[] bytes;
+        try {
+            encoder.encode(buffer, toBeEncoded);
+            bytes = new byte[buffer.readableBytes()];
+            buffer.readBytes(bytes);
+        } finally {
+            buffer.release(); // Release as soon as possible.
+        }
+        byte[] actualBytes = decode(bytes);
+        String actualDecoded = new String(actualBytes);
+        char[] charArray = new char[inputLen];
+        Arrays.fill(charArray, '?');
+        String expectedDecoded = new String(charArray);
+        assertEquals(
+                expectedDecoded,
+                actualDecoded,
+                "Expect the decoded string to be sanitized and contains only '?' characters."
+        );
+    }
+
     private static byte[] makeBuf(int ... bytes) {
         byte[] buf = new byte[bytes.length];
         for (int i = 0; i < buf.length; i++) {