Skip to content

CRITICAL: Prevent Remote Stack Buffer Overflow in WebSettings#284

Open
bytecodesky wants to merge 1 commit intoshining-man:mainfrom
bytecodesky:fix/websettings-buffer-overflow
Open

CRITICAL: Prevent Remote Stack Buffer Overflow in WebSettings#284
bytecodesky wants to merge 1 commit intoshining-man:mainfrom
bytecodesky:fix/websettings-buffer-overflow

Conversation

@bytecodesky
Copy link
Copy Markdown

Bug: Stack-based Buffer Overflow in addJsonElem via Unbounded sprintf (CWE-121)

Impact:
The handleGetValues function allocates a fixed-size 4096-byte buffer (char data[4096]) on the stack and iteratively passes it to overloaded addJsonElem functions for each HTTP request argument parsed from server->args(). The addJsonElem functions use strlen(buf) to find the end of the string and append JSON key-value pairs using sprintf without verifying the remaining buffer capacity.

An attacker can supply a crafted HTTP request containing an excessive number of query parameters. Because the loop processes all provided arguments and blindly appends to data, the appended payload will eventually exceed the 4096-byte limit. This results in a stack-based buffer overflow, overwriting adjacent stack memory. The immediate impact is a Denial of Service (device crash/watchdog reset), with potential for Remote Code Execution (RCE) depending on the underlying RTOS/firmware stack layout and exploit mitigations.

Vulnerable Code:

void addJsonElem(char *buf, int id, bool val)
{
  uint16_t bufLen = strlen(buf);
  if (bufLen == 0) sprintf(&buf[bufLen], "[");
  else sprintf(&buf[bufLen], ",");
  bufLen++;
  sprintf(&buf[bufLen], "{\"id\":%i, \"val\":%u}",id,val);
}

// ... (other overloaded addJsonElem functions share the same flaw) ...

void WebSettings::handleGetValues(WebServer *server)
{
  uint32_t argName;
  char data[4096] = { 0 };
  
  for(uint8_t i=0; i<server->args(); i++)
  {
    if(isNumber(server->argName(i))) 
    {
      argName = server->argName(i).toInt();
      // ...
      if(u8_storeInFlash==0)
      {
        if(u8_dataType==PARAM_DT_BO) addJsonElem(data, argName, getBool((uint16_t)(argName&0xFFFF)));
        // ...

Proposed Fix:
Refactor the addJsonElem overloads to accept a maximum buffer size parameter and replace sprintf with snprintf to enforce strict bounds checking. Update handleGetValues to pass sizeof(data) and safely append the closing bracket.

// Apply this pattern to all addJsonElem overloads
void addJsonElem(char *buf, size_t max_len, int id, bool val)
{
  size_t bufLen = strlen(buf);
  if (bufLen >= max_len - 1) return; // Buffer full, drop payload
  
  int written = snprintf(&buf[bufLen], max_len - bufLen, (bufLen == 0) ? "[" : ",");
  if (written > 0 && (size_t)written < max_len - bufLen) {
      bufLen += written;
  } else {
      return; // Truncation occurred
  }
  
  snprintf(&buf[bufLen], max_len - bufLen, "{\"id\":%i, \"val\":%u}", id, val);
}

// ... update other addJsonElem signatures similarly ...

void WebSettings::handleGetValues(WebServer *server)
{
  uint32_t argName;
  char data[4096] = { 0 };
  const size_t max_len = sizeof(data);
  
  for(uint8_t i=0; i<server->args(); i++)
  {
    if(isNumber(server->argName(i))) 
    {
      argName = server->argName(i).toInt();
      uint8_t u8_storeInFlash = ((argName>>16)&0xff);
      uint8_t u8_dataType = ((argName>>24)&0xff);

      if(u8_storeInFlash==0)
      {
        if(u8_dataType==PARAM_DT_BO) addJsonElem(data, max_len, argName, getBool((uint16_t)(argName&0xFFFF)));
        else if(u8_dataType==PARAM_DT_FL) addJsonElem(data, max_len, argName, getFloat((uint16_t)(argName&0xFFFF)));
        else if(u8_dataType==PARAM_DT_ST) addJsonElem(data, max_len, argName, getString((uint16_t)(argName&0xFFFF),false,PARAM_DT_ST));
        else addJsonElem(data, max_len, argName, getInt((uint16_t)(argName&0xFFFF), u8_dataType));
      }
      else if(u8_storeInFlash==1)
      {
        if(u8_dataType==PARAM_DT_BO) addJsonElem(data, max_len, argName, getBoolFlash((uint16_t)(argName&0xFFFF)));
        else if(u8_dataType==PARAM_DT_FL) addJsonElem(data, max_len, argName, getFloatFlash((uint16_t)(argName&0xFFFF)));
        else if(u8_dataType==PARAM_DT_ST) addJsonElem(data, max_len, argName, getStringFlash((uint16_t)(argName&0xFFFF)));
        else addJsonElem(data, max_len, argName, (int)getIntFlash((uint16_t)(argName&0xFFFF), u8_dataType));
      }
    }
    else
    {
      BSC_LOGE(TAG,"handleGetValues: not number");
    }
  }
  
  size_t current_len = strlen(data);
  if (current_len < max_len - 1) {
      snprintf(&data[current_len], max_len - current_len, "]");
  }

  server->send(200, "application/json", data);
}

Replaced unbounded sprintf with snprintf and added max_len parameter to prevent memory corruption when parsing a large number of HTTP arguments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant