Repository navigation
fix(jsonrpc): reject invalid UTF-8 and trailing request content - #7103
waynercheung wants to merge 7 commits into
Conversation
Check that the request body is well-formed UTF-8 before parsing it. Jackson accepts overlong encodings, UTF-16 surrogates and code points above U+10FFFF, none of which are valid UTF-8 (RFC 3629). A body that is not well-formed UTF-8 now gets a -32700 parse error with a null id, and nothing is dispatched. This applies to single, batch and notification requests alike. Compatibility: well-formed UTF-8, including a byte order mark and non-ASCII text in parameters, is unchanged. Encoding detection is kept only for bodies that are also well-formed UTF-8, so an ASCII body sent as UTF-16 without a byte order mark is still accepted. A UTF-16 or UTF-32 body whose raw bytes are not well-formed UTF-8, such as one that starts with a byte order mark, now gets -32700.
Parse the request body with FAIL_ON_TRAILING_TOKENS so that it must
hold exactly one JSON value. Previously the node answered the first
value and silently dropped anything after it, so a client that sent
two requests received one response with no error.
Anything other than whitespace after the first value now gets a -32700
parse error with a null id, and nothing is dispatched. Trailing
whitespace is still accepted. Only the HTTP JSON-RPC servlet is
changed; the shared mapper is not, and the admin endpoints already
reject trailing content.
BREAKING CHANGE: a body such as `{...} {...}` or `{...}}` used to
return the result of the first request and now returns -32700.
Clients that send several requests in one body must send them as a
batch array instead.
| writeJsonRpcError(resp, JsonRpcError.PARSE_ERROR, "JSON parse error", null, false); | ||
| return; | ||
| } | ||
| } catch (JsonProcessingException e) { |
There was a problem hiding this comment.
[SHOULD] REQUEST_READER.readTree(body) can also throw a plain IOException that is not a JsonProcessingException, for example java.io.CharConversionException from Jackson's character decoding. It skips the -32700 mapping here and leaves doPostInternal, so the client gets an empty HTTP 200 without a JSON-RPC envelope. Because the trailing-token check now reads to the end of the body, a few bodies that used to be executed take this path instead of returning -32700. The body is already in memory, so any IOException from readTree(byte[]) is a decoding failure, not a transport failure. Could this also catch IOException and return -32700, as AdminJsonRpcRequestHandler does (line 47)?
There was a problem hiding this comment.
Thanks, confirmed and fixed in 0a577bb. Over embedded Jetty, a BOM-less UTF-32 request followed by 1 or 3 extra bytes ran on the base and returned an empty HTTP 200 on this branch, because the trailing check now reads to the end of the body. An invalid code unit can also hit this path on the base when the decoder's read-ahead reaches it, so not every case is new, but all of them should be -32700.
The try block now covers only REQUEST_READER.readTree(body) and catches IOException. The missing-node check and the error writes are outside it, so a failure while writing the response is not mistaken for a parse error. The new test undecodableUtf32Body_returnsParseError fails if the catch is narrowed back to JsonProcessingException in that commit. With the NUL check below in place, these UTF-32 cases are rejected before parsing; the additional handling of non-JsonProcessingException IOExceptions remains a safeguard.
| // Transport IOException from readBody propagates as HTTP 500 (genuine IO failure). | ||
| byte[] body = readBody(req.getInputStream()); | ||
| // Jackson accepts overlong and surrogate UTF-8 sequences, so check the bytes first. | ||
| if (!Utf8.isWellFormed(body)) { |
There was a problem hiding this comment.
[SHOULD] Could this also reject bodies that contain a 0x00 byte, as AdminJsonRpcRequestHandler already does next to the same UTF-8 check (!Utf8.isWellFormed(body) || Bytes.contains(body, (byte) 0), line 39; its comment explains why)?
I know the description lists NUL bytes and encoding detection as out of scope, so this is only to make the trade-off explicit:
- A 0x00 byte can never appear in a UTF-8 JSON text that Jackson accepts: unescaped U+0000 is not allowed anywhere (RFC 8259), and with this PR a trailing one is rejected too. So the extra condition cannot reject a valid UTF-8 request;
\u0000escapes still work. - It only changes bodies that Jackson auto-detects as UTF-16 or UTF-32. Jackson picks those decoders only for a UTF-16/32 BOM, which this PR already rejects, or for a 0x00 in the first four bytes.
- Today such a BOM-less body is accepted or rejected depending on its characters: a UTF-16 request with U+4E2D runs, while the same request with U+00E9 gets -32700, as the two
utf16*tests show. BOM-less UTF-32 is accepted the same way. Before this PR all of them ran.
With the NUL check the rule becomes simply "UTF-8 only" (RFC 8259 §8.1), matching the admin endpoint, and Jackson's UTF-16/UTF-32 decoders are no longer reached at all. The cost is one more linear scan of the body. If you'd rather keep the current behavior, could the release note say that BOM-less UTF-16/UTF-32 bodies may still be decoded as such, and the table mention UTF-32?
There was a problem hiding this comment.
Agreed, done in bf6d23a: the check is now !Utf8.isWellFormed(body) || Bytes.contains(body, (byte) 0), the same byte check as the admin endpoint, and the rule is simply UTF-8 only (a UTF-8 BOM is still accepted). The UTF-16 characterization test now asserts that BOM-less UTF-16LE/BE and UTF-32LE/BE bodies, including the U+4E2D one, get -32700; there are new tests for raw NUL bytes (rejected) and the \u0000 escape (still dispatched); the UTF-16 trailing-content test is gone since those bodies no longer reach the parser. For reference, go-ethereum does not auto-detect UTF-16 or UTF-32 either, and rejects requests encoded that way with -32700. The description and the release note are updated.
| public void wellFormedNonAsciiInput_isDispatched() throws Exception { | ||
| TronJsonRpc rpc = useRealRpcServer(); | ||
| when(rpc.web3Sha3(any())).thenReturn("0x1"); | ||
| String chinese = "中文"; |
There was a problem hiding this comment.
[NIT] Two small things:
wellFormedNonAsciiInput_isDispatchedcovers raw 3-byte and 4-byte UTF-8, but its U+0080 to U+07FF case is the JSON escape for U+00E9, which is plain ASCII on the wire. So no positive test sends a raw 2-byte sequence, and the negative test only has 2-byte overlongs. Consider adding one, e.g.new String(Character.toChars(0xE9)). Optionally,new String(Character.toChars(0x10FFFF))instead of the raw literal on line 141 would make that case visible in the diff; plain Unicode escapes would tripAvoidEscapedUnicodeCharacters.- The comment on
JsonRpcServlet.java:127could also mention code points above U+10FFFF, as the description does.
There was a problem hiding this comment.
Done. 1d841d4 adds a raw two-byte parameter. I used U+07FF rather than U+00E9 because the JSON escape case already decodes to U+00E9 and the per-iteration verify would count two calls. The 4-byte parameters are now built with Character.toChars. The comment at JsonRpcServlet.java:127 now mentions code points above U+10FFFF (bf6d23a).
halibobo1205
left a comment
There was a problem hiding this comment.
[NIT] The suggested release note says "The JSON-RPC HTTP endpoint", but the change also applies to the solidity and PBFT JSON-RPC endpoints (JsonRpcOnSolidityServlet and JsonRpcOnPBFTServlet extend JsonRpcServlet and call super.doPost), as the first paragraph of the description says. Since the note is the text that usually gets copied into release notes, consider naming all three.
Catch every IOException from parsing the request body, not only JsonProcessingException, and limit the try block to the parse call. Jackson's UTF-32 decoder reports a malformed or truncated code unit as a CharConversionException, which is not a JsonProcessingException, so it escaped the servlet and the client got an empty response. Such a body now gets a -32700 parse error with a null id, and nothing is dispatched.
Add a raw two-byte UTF-8 parameter (U+07FF) to the well-formed input test; the existing two-byte case was a JSON escape and so plain ASCII on the wire. Build the four-byte parameters from code points so that they are visible in the source.
Also reject a request body that contains a NUL byte, as the admin endpoint already does. A NUL byte cannot appear in a UTF-8 JSON text that the parser accepts, but Jackson takes one in the first four bytes as a sign of UTF-16 or UTF-32 and decodes the body that way. Compatibility: UTF-8 request bodies, with or without a byte order mark, are unchanged, and the JSON escape \u0000 still works. A UTF-16 or UTF-32 body without a byte order mark used to be decoded as such when its raw bytes were also well-formed UTF-8; it now gets a -32700 parse error with a null id.
|
Done, the suggested release note in the description now names the FullNode, solidity and PBFT JSON-RPC HTTP endpoints. |
| writeJsonRpcError(resp, JsonRpcError.PARSE_ERROR, "JSON parse error", null, false); | ||
| } | ||
| rootNode = REQUEST_READER.readTree(body); | ||
| } catch (IOException e) { |
There was a problem hiding this comment.
[DISCUSS] Why is this widened to IOException? With the byte check in front, I could not find a body that reaches this catch with anything other than JsonProcessingException: the CharConversionException cases all contain 0x00 and are rejected earlier, and the tests stay green with the catch narrowed back.
There was a problem hiding this comment.
Agreed, narrowed back to JsonProcessingException in c2ea817; the try still covers only the parse call. The wider catch was needed while BOM-less UTF-32 bodies were still auto-detected: in 0a577bb, narrowing it makes the UTF-32 test fail. Since bf6d23a rejects NUL bytes, those bodies no longer reach Jackson's UTF-32 decoder, and I could not find a body that passes the byte check and fails with anything other than a JsonProcessingException either.
| concat(request, new byte[] {'\n'}), | ||
| concat(request, new byte[] {0, 0, ' '}), | ||
| concat(request, padding, new byte[] {0, 0x11, 0, 0})); | ||
| for (byte[] body : bodies) { |
There was a problem hiding this comment.
[SHOULD] All three bodies start with the request encoded as UTF-32BE (00 00 00 7B ...), so they contain 0x00 and are rejected by the byte check before reaching Jackson's UTF-32 decoder. The comment about CharConversionException no longer matches what the test exercises, and it now covers the same branch as utf16AndUtf32WithoutByteOrderMark_areRejected.
Two more leftovers of the same kind:
utf16BodyThatIsNotWellFormedUtf8_isRejected: all four bodies contain0x00, so the test still passes withUtf8.isWellFormedremoved (onlymalformedUtf8_isRejectedBeforeDispatchand the Jetty counterpart fail).- The
web3ClientVersionstub inmalformedUtf8_isRejectedBeforeDispatchand theweb3Sha3stub inutf16BodyThatIsNotWellFormedUtf8_isRejectedare never hit; both tests pass without them.
Suggest removing or merging these cases.
There was a problem hiding this comment.
Done in dfd2806: undecodableUtf32Body_returnsParseError is removed, the UTF-16 bodies with a BOM and the U+00E9 request from utf16BodyThatIsNotWellFormedUtf8_isRejected are merged into utf16AndUtf32Bodies_areRejected, and the two unused stubs are gone; the never() checks stay. Removing Utf8.isWellFormed still fails the two malformed UTF-8 tests, and removing the NUL check fails the UTF-16/UTF-32 test. The description is updated.
| } | ||
| } | ||
|
|
||
| private static byte[] concat(byte[]... parts) { |
There was a problem hiding this comment.
[NIT] Guava's Bytes.concat(byte[]...) does the same thing, and JsonRpcServlet already imports Bytes. Replacing both helper bodies with it keeps the two test classes green.
There was a problem hiding this comment.
Done in dfd2806, both test classes now use a static import of Bytes.concat.
Request bodies that are not well-formed UTF-8 or that contain a NUL byte are rejected before parsing, so the parser only sees UTF-8 input held in memory and reports every failure as a JsonProcessingException. Go back to catching that type. The try block still covers only the parse call. No behavior change.
- Remove the UTF-32 decoding test; its bodies contain NUL bytes and are rejected before parsing, like the other UTF-16 and UTF-32 cases. - Merge the UTF-16 test with a byte order mark into the UTF-16/UTF-32 rejection test and add a U+00E9 request to it. - Drop stubs for methods that the rejection tests never reach. - Use Guava Bytes.concat instead of local helpers.
d9efdc5 to
dfd2806
Compare
What does this PR do?
Tightens how the JSON-RPC HTTP servlet (
JsonRpcServlet) accepts a request body. The FullNode, solidity and PBFT JSON-RPC services all use it (JsonRpcOnSolidityServletandJsonRpcOnPBFTServletextend it). Before any request is dispatched, the following bodies now get HTTP 200 and{"jsonrpc":"2.0","error":{"code":-32700,"message":"JSON parse error"},"id":null}:Utf8.isWellFormed) or a raw NUL (0x00) byte. Jackson accepts overlong encodings, UTF-16 surrogates and code points above U+10FFFF, none of which are valid UTF-8 (RFC 3629). A NUL byte cannot appear in a UTF-8 JSON text that the parser accepts, but Jackson takes one as a sign of UTF-16 or UTF-32 and decodes the body that way. This is the same byte check the admin endpoint already applies.ObjectReaderthat enablesDeserializationFeature.FAIL_ON_TRAILING_TOKENS.These checks apply to single, batch and notification requests. The feature is enabled only on the servlet's own reader, so the shared
JsonRpcMapperdoes not change. Thetryblock around parsing now covers only the parse call.Why are these changes required?
{...} {...}got one response and no error. go-ethereum (v1.17.6 and later) and Besu both reject such a body with -32700.This PR has been tested by:
JsonRpcServletTestwith a realJsonRpcServer. Each rejected body asserts the full -32700 envelope with a null id, and asserts that neither the server nor the service method was called.}or], a notification pair, and1 2.\uescapes including\u0000, a UTF-8 BOM, and trailing space/tab/CR/LF.JsonRpcServletJettyTest: posts raw bytes through embedded Jetty and the real filter chain, one invalid UTF-8 body and one trailing-content body../gradlew teston dfd2806: the only failures were 7InetUtilTestcases (DNS resolution timeouts), which fail the same way on the unchanged baseline.-x :framework:generateGitProperties, because that task fails in a linked git worktree (JGit cannot resolve the repository). On dfd2806,./gradlew clean build -x test, the Checkstyle tasks frompr-check.ymlandcheck_math_usage.shpassed.Follow up
None.
Extra details
Behavior changes (release note)
{...} {...},{...} xyz,{...}},[...] {...}1 2,"x" {\u0000escapeSuggested release note: The FullNode, solidity and PBFT JSON-RPC HTTP endpoints now reject, with a -32700 parse error, a request body that contains anything other than whitespace after the first JSON value; clients that send several requests in one body must send a batch array instead. They also accept only UTF-8 request bodies: a body that is not well-formed UTF-8, or that is encoded as UTF-16 or UTF-32, now gets a -32700 parse error. Clients sending valid UTF-8 JSON-RPC requests containing a single JSON value are unaffected.
Notes: