fix Invalid JSON desearilaztion beyond buffer end #184

ayavilevich · 2025-05-23T09:24:30Z

fix for #183

Copilot

Pull Request Overview

This PR addresses issue #183 by correcting JSON deserialization beyond the buffer end.

Updated deserializeJson call for ArduinoJson v6 and non-v6 implementations, passing request->contentLength() as an additional boundary argument.

Copilot · 2025-05-23T14:17:39Z

src/AsyncJson.cpp

@@ -125,12 +125,12 @@ void AsyncCallbackJsonWebHandler::handleRequest(AsyncWebServerRequest *request)
      if (json.success()) {
 #elif ARDUINOJSON_VERSION_MAJOR == 6
      DynamicJsonDocument jsonBuffer(this->maxJsonBufferSize);
-      DeserializationError error = deserializeJson(jsonBuffer, (uint8_t *)(request->_tempObject));
+      DeserializationError error = deserializeJson(jsonBuffer, (uint8_t *)(request->_tempObject), request->contentLength());


Consider adding an inline comment here to explain that the request->contentLength() parameter is used to restrict the deserialization to the valid buffer size, addressing issue #183.

Copilot · 2025-05-23T14:17:40Z

src/AsyncJson.cpp

      if (!error) {
        JsonVariant json = jsonBuffer.as<JsonVariant>();
 #else
      JsonDocument jsonBuffer;
-      DeserializationError error = deserializeJson(jsonBuffer, (uint8_t *)(request->_tempObject));
+      DeserializationError error = deserializeJson(jsonBuffer, (uint8_t *)(request->_tempObject), request->contentLength());


Consider adding a similar inline comment here to clarify the purpose of passing request->contentLength() for preventing invalid JSON read beyond the allocated buffer.

need to think about it again...

mathieucarbou

The fix is not as simple as relying on request->contentLength());.

What is happening if you run:

curl -v -X POST -H 'Content-Type: application/json' -d '5' -H 'Content-Length: 3' http://127.0.0.1:8080/json2

ayavilevich · 2025-05-24T09:07:30Z

The fix is not as simple as relying on request->contentLength());.

What is happening if you run:

curl -v -X POST -H 'Content-Type: application/json' -d '5' -H 'Content-Length: 3' http://127.0.0.1:8080/json2

If you send 1 byte but specify in headers that you are sending 3 then the server will keep on waiting for 2 more bytes. Eventually it will timeout and handleBody won't be called.

This is what curl will say:

curl -v -X POST -H "Content-Type: application/json" -d "5" -H "Content-Length: 3" http://192.168.4.1/json2
Note: Unnecessary use of -X or --request, POST is already inferred.
*   Trying xxxxx
* Connected to xxxx port 80
> POST /json2 HTTP/1.1
> Host: xxxx
> User-Agent: curl/8.9.1
> Accept: */*
> Content-Type: application/json
> Content-Length: 3
>
* upload completely sent off: 1 bytes
* Empty reply from server
* shutting down connection #0
curl: (52) Empty reply from server

An edge case in the other direction where you send 5 bytes but claim 3 in the headers will result in 2 bytes being ignored.

curl:

C:\Users\arik>curl -v -X POST -H "Content-Type: application/json" -d "55555" -H "Content-Length: 3" http://192.168.4.1/json2
Note: Unnecessary use of -X or --request, POST is already inferred.
*   Trying xxxx
* Connected to xxxx port 80
> POST /json2 HTTP/1.1
> Host: xxxx
> User-Agent: curl/8.9.1
> Accept: */*
> Content-Type: application/json
> Content-Length: 3
>
* upload completely sent off: 5 bytes
< HTTP/1.1 200 OK
< connection: close
< accept-ranges: none
< content-length: 3
< content-type: text/plain
<
int* shutting down connection #0

where ESP will say

12:00:45.874 > Json:
12:00:45.875 > 555
12:00:45.876 > Got an int

So 555 and not 55555. Expected.

So it seems that it works however you have a point. There are two different _contentLength here (which seem to be in sync as far as I can check) one in AsyncCallbackJsonWebHandler and one in AsyncWebServerRequest.
The one used to allocate the buffer is the local one to AsyncCallbackJsonWebHandler and not the one used in the proposed fix. It might be safer/better to use the local _contentLength directly. Do you agree? LMK and I will update.

ayavilevich · 2025-05-24T09:31:02Z

The one used to allocate the buffer is the local one to AsyncCallbackJsonWebHandler and not the one used in the proposed fix. It might be safer/better to use the local _contentLength directly. Do you agree? LMK and I will update.

Actually, after further inspection, I take it back.
We have to remove the local _contentLength as it doesn't make sense. A single handler can handle many requests, even concurrently. So having a global length for all requests is a mistake.
So the original proposal is fine. Maybe we can add some additional checks. More work is needed to determine if there are cases whee it might be needed.

mathieucarbou · 2025-05-24T09:45:25Z

The one used to allocate the buffer is the local one to AsyncCallbackJsonWebHandler and not the one used in the proposed fix. It might be safer/better to use the local _contentLength directly. Do you agree? LMK and I will update.

Actually, after further inspection, I take it back. We have to remove the local _contentLength as it doesn't make sense. A single handler can handle many requests, even concurrently. So having a global length for all requests is a mistake. So the original proposal is fine. Maybe we can add some additional checks. More work is needed to determine if there are cases whee it might be needed.

I agree!

mathieucarbou · 2025-05-24T09:46:07Z

Do you want to see how this can be improved ?

ayavilevich · 2025-05-24T13:30:50Z

Do you want to see how this can be improved ?

yes, I will update the PR.

mathieucarbou · 2025-05-24T19:27:23Z

src/ESPAsyncWebServer.h

@@ -261,6 +261,7 @@ class AsyncWebServerRequest {
 public:
  File _tempFile;
  void *_tempObject;
+  size_t _tempSize;


I think it would be better to have a struct with :

size_t len

uint8_t* buffer (type TBD)

that you would put inside the _tempObject instead of adding a new field for all requests

I think it would be better to have a struct with :

* size_t len * uint8_t* buffer (type TBD)

Yes, it is possible. A struct with a buffer at the end, so it can be variable size.

size_t length uint8_t buffer[1]

mathieucarbou · 2025-05-24T19:36:58Z

src/AsyncJson.cpp

    }
    if (request->_tempObject != NULL) {
-      memcpy((uint8_t *)(request->_tempObject) + index, data, len);
+      // check if the buffer is the right size so we don't write out of bounds
+      if (request->_tempSize >= total) {


I do not understand this check: handleBody can be called several times, but _tempSize and total do not change

I do not understand this check: handleBody can be called several times, but _tempSize and total do not change

I wasn't sure if total won't change from one call to handleBody to the other. In my tests it was the same but I wanted to take precautions.
Can I assume it will be the same all the time? If that is the case then we don't need _tempSize at all.
tempSize represents the max number of bytes we can store in the buffer. So once it is allocated it doesn't change.

mathieucarbou · 2025-05-24T19:40:37Z

src/AsyncJson.cpp

-    } else if (request->_tempObject != NULL) {
+    }
+    // this is not a GET
+    // check if body is too large, if it is, don't parse


wrong: it is already parsed: handleBody is called before handleRequest.
(but handleBody will have done nothing)

I meant JSON parsing. Should replace "parse" with "de-serialize" here.

mathieucarbou

PR looks better! Just a few things to fix.
If you want to go further in code cleanup, like using the index instead of the tmpObject to make it a little more readable, go ahead!

Copilot

Pull Request Overview

This PR addresses an issue with invalid JSON deserialization beyond the buffer end by improving buffer allocation and JSON parsing logic. Key changes include:

Adding a new _tempSize member to track allocated buffer size.
Removing the unused _contentLength variable in the JSON handler.
Adjusting JSON parsing logic to ensure proper null termination and buffer bounds checking.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/WebRequest.cpp	Added _tempSize to the constructor initializer list.
src/ESPAsyncWebServer.h	Declared _tempSize member to support buffer size tracking.
src/AsyncJson.h	Removed the unused _contentLength member.
src/AsyncJson.cpp	Modified JSON parsing and buffer handling logic for safety.

Comments suppressed due to low confidence (1)

src/AsyncJson.cpp:174

Since the buffer is allocated with an extra byte (i.e., malloc(total + 1)), consider renaming or updating _tempSize to reflect the actual allocated size to avoid potential off-by-one misunderstandings in future code changes.

request->_tempSize = total;  // store the size of allocation we made into _tempSize

Copilot · 2025-05-24T19:44:24Z

src/AsyncJson.cpp

+        min(request->contentLength(), request->_tempSize);  // smaller value of contentLength or the size of the buffer. normally those should match.
 #if ARDUINOJSON_VERSION_MAJOR == 5
      DynamicJsonBuffer jsonBuffer;
-      JsonVariant json = jsonBuffer.parse((uint8_t *)(request->_tempObject));
+      uint8_t *p = (uint8_t *)(request->_tempObject);
+      p[dataSize] = '\0';  // null terminate, assume we allocated one extra char


The null termination here relies on the assumption that the allocated buffer has an extra byte, but _tempSize is set to the original total. To improve clarity, consider documenting or renaming _tempSize so it explicitly reflects the buffer's capacity (i.e., total bytes allocated minus one) to avoid confusion in future modifications.

ayavilevich · 2025-05-25T05:52:51Z

If you want to go further in code cleanup, like using the index instead of the tmpObject to make it a little more readable, go ahead!

What is "the index"? How will it replace tmpObject?

fix ESP32Async#183

56470cb

mathieucarbou previously approved these changes May 23, 2025

View reviewed changes

mathieucarbou assigned ayavilevich May 23, 2025

mathieucarbou added the Status: Pending Merge label May 23, 2025

mathieucarbou requested review from me-no-dev, vortigont and Copilot May 23, 2025 14:17

Copilot AI reviewed May 23, 2025

View reviewed changes

mathieucarbou self-requested a review May 23, 2025 14:20

mathieucarbou requested changes May 23, 2025

View reviewed changes

make edge case handing in AsyncCallbackJsonWebHandler more robust

2070e49

mathieucarbou linked an issue May 24, 2025 that may be closed by this pull request

Invalid JSON desearilaztion beyond buffer end #183

Open

5 tasks

ci(pre-commit): Apply automatic fixes

172a88d

mathieucarbou reviewed May 24, 2025

View reviewed changes

mathieucarbou requested changes May 24, 2025

View reviewed changes

mathieucarbou requested a review from Copilot May 24, 2025 19:43

Copilot AI reviewed May 24, 2025

View reviewed changes

fix Invalid JSON desearilaztion beyond buffer end #184

Are you sure you want to change the base?

fix Invalid JSON desearilaztion beyond buffer end #184

Uh oh!

Conversation

ayavilevich commented May 23, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI May 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 23, 2025

Choose a reason for hiding this comment

Uh oh!

mathieucarbou left a comment

Choose a reason for hiding this comment

Uh oh!

ayavilevich commented May 24, 2025

Uh oh!

ayavilevich commented May 24, 2025

Uh oh!

mathieucarbou commented May 24, 2025

Uh oh!

mathieucarbou commented May 24, 2025

Uh oh!

ayavilevich commented May 24, 2025

Uh oh!

mathieucarbou May 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ayavilevich May 25, 2025

Choose a reason for hiding this comment

Uh oh!

mathieucarbou May 24, 2025

Choose a reason for hiding this comment

Uh oh!

ayavilevich May 25, 2025

Choose a reason for hiding this comment

Uh oh!

mathieucarbou May 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ayavilevich May 25, 2025

Choose a reason for hiding this comment

Uh oh!

mathieucarbou left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI May 24, 2025

Choose a reason for hiding this comment

Uh oh!

ayavilevich commented May 25, 2025

Uh oh!

Uh oh!

mathieucarbou May 24, 2025 •

edited

Loading

mathieucarbou May 24, 2025 •

edited

Loading