Repository: netty/netty
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
```

Issue comments:
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 

Pull request review comments:
We cannot directly use `AsciiString.c2b(...)` here because this function does 2 things:

1. Sanitize the given input char.
2. Cast the sanitized char to a `byte`.

Then we cast the returned `byte` to `int b`. Overall, we are doing `sanitizedChar.toByte.toInt` and it could yield negative value which cannot be used as array index. For example `128.toByte.toInt == -128`.

We can also consider doing:
```java
int b = (char) AsciiString.c2b(data.charAt(i));
```
or 
```java
int b = AsciiString.b2c(AsciiString.c2b(data.charAt(i)));
```

But it does not seem to be expressive and clear on our intention and it seems awkward IMO.

So, I decided to create this `sanitizeChar` local function.
Updated it to `AsciiString.c2b(data.charAt(i)) & 0xFF` as `0xFF` can produce unsigned int which is what we want here.
`it's`?
My understanding of the small value is that it wont sanitize anything since it will never yield a value greater than 255, making it not a very interesting test. The 300 case should cover what we're really interested in wrt ensuring we don't get wrap-around values of null etc.
A second type of test is that would be valuable is that we should get the same result whether we perform the Huffman encoding or not. What if we just test the size 300 and test it twice: once with the encoded set to 512 (the default I believe) and once super short such to force Huffman encoding, then make sure they both yield the same results?
Thanks Bryce for the suggestion. I think it's a good idea and I have updated this PR with this idea implemented. PTAL.
looks good.

Patch that resolved the issue:
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
index 322898f94a1..8f2d5a4df9f 100644
--- 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
index c794ade385e..d3641ce98df 100644
--- 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
index b5f5d6879b8..2a54fbfab9a 100644
--- 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
index 2a6ea038aac..8b4bddcf998 100644
--- 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++) {