Infinite loop parsing headers (in chrome)

[pnegre]

Description

When using chrome 99.0.4844.51 (linux) to access the HTTPS server, the microcontroller goes into a infinite loop. It happens when it is parsing the client headers.

How To Reproduce
Steps to reproduce the behavior:

1. Compile and upload this example: https://github.com/fhessel/esp32_https_server/blob/master/examples/Self-Signed-Certificate/Self-Signed-Certificate.ino
2. Go to https://esp32-ip/ (where "esp32-ip" is the local ip assigned by the router)
3. Accept the certificate exception
4. See the error: the server never responds.

If using curl or wget it works fine. I have checked that in firefox also works.

Expected Behavior

Should respond with the expected html code

Actual Behavior

The response never arrives to the client.

ESP32 Module
Please provide specifications of your module

• Chip is ESP32-D0WDQ6 (revision 1)

Software (please complete the following information if applicable)

• Arduino 1.8.19
• OS: Ubuntu 20.04 LTS
• Client used to access the server: Chrome 99.0.4844.51

Possible solution

See patch for "possible" solution. I know it is not the right answer. But when I apply this patch to the code, it works fine in chrome too.

I don't understand where is exactly the problem, i think that is the management of the buffer. Maybe the headers are too long??

HTTPConnection.patch.txt

[pnegre]

To clarify the issue, this is the "readline" function (in HTTPConnection.cpp):

void HTTPConnection::readLine(int lengthLimit) {
  while(_bufferProcessed < _bufferUnusedIdx) {
    char newChar = _receiveBuffer[_bufferProcessed];

    if ( newChar == '\r') {
      // Look ahead for \n (if not possible, wait for next round
      if (_bufferProcessed+1 < _bufferUnusedIdx) {
        if (_receiveBuffer[_bufferProcessed+1] == '\n') {
          _bufferProcessed += 2;
          _parserLine.parsingFinished = true;
          return;
        } else {
          // Line has not been terminated by \r\n
          HTTPS_LOGW("Line without \\r\\n (got only \\r). FID=%d", _socket);
          raiseError(400, "Bad Request");
          return;
        }
      }
    } else {
      _parserLine.text += newChar;
      _bufferProcessed += 1;
    }

    // Check that the max request string size is not exceeded
    if (_parserLine.text.length() > lengthLimit) {
      HTTPS_LOGW("Header length exceeded. FID=%d", _socket);
      raiseError(431, "Request Header Fields Too Large");
      return;
    }
  }
}

I think the problem is, if the condition (_bufferProcessed+1 < _bufferUnusedIdx) is false, the program never leaves the while statement.

[pnegre]

I think I've identified the problem more accurately.

The "infinite loop" problem happens when "_bufferProcessed=511" and "_bufferUnused=512".

You don't need chrome to reproduce the problem. Attached to this file there is the custom headers i've "fabricated" to trigger the infinite loop situation.

Run with "curl -H @header_file.txt --insecure https://esp32-ip"
header_file.txt

[pnegre]

More information: this is the modified function i've used to identify the bug:

void HTTPConnection::readLine(int lengthLimit) {
  HTTPS_LOGW("bufferProcessed: %d", _bufferProcessed);
  HTTPS_LOGW("bufferUnused: %d", _bufferUnusedIdx);
  while(_bufferProcessed < _bufferUnusedIdx) {
    char newChar = _receiveBuffer[_bufferProcessed];
    HTTPS_LOGW(_parserLine.text.c_str());
    if ( newChar == '\r') {
      // Look ahead for \n (if not possible, wait for next round
      if (_bufferProcessed+1 < _bufferUnusedIdx) {
	if (_receiveBuffer[_bufferProcessed+1] == '\n') {
	  _bufferProcessed += 2;
	  _parserLine.parsingFinished = true;
	  return;
	} else {
	  // Line has not been terminated by \r\n
	  HTTPS_LOGW("Line without \\r\\n (got only \\r). FID=%d", _socket);
	  raiseError(400, "Bad Request");
	  return;
	}
      } else {
	  HTTPS_LOGW("halted");
	  //_parserLine.parsingFinished = true;
	  while(1) ;
      }
    } else {
      _parserLine.text += newChar;
      _bufferProcessed += 1;
    }

    // Check that the max request string size is not exceeded
    if (_parserLine.text.length() > lengthLimit) {
      HTTPS_LOGW("Header length exceeded. FID=%d", _socket);
      raiseError(431, "Request Header Fields Too Large");
      return;
    }
  }
}

If you put this code into the library (replace the original "readLine" in HTTPConnection.cpp), you will see in the debug messages the values of the variables bufferProcessed and bufferUnused...

To trigger the bug, you only have to make sure that at the end of the header line, bufferProcessed is 511. It is easy... you only have to add characters to the last header.

I don't know what is the fix, though!

[sairan22]

I faced the same problem and managed to get it fixed.
(1) The "readLine" routine needs to exit if the expected '\n' is not present in the current data chunk. I just added "else { return; }" (2) Similarly, in the "case STATE_REQUEST_FINISHED:" section, it needs to break calling "readLine" in such situation.

// ===== the updated "readLine"

void HTTPConnection::readLine(int lengthLimit) {
while(_bufferProcessed < _bufferUnusedIdx) {
char newChar = _receiveBuffer[_bufferProcessed];

if ( newChar == '\r') {
  // Look ahead for \n (if not possible, wait for next round
  if (_bufferProcessed+1 < _bufferUnusedIdx) {
    if (_receiveBuffer[_bufferProcessed+1] == '\n') {
      _bufferProcessed += 2;
      _parserLine.parsingFinished = true;
      return;
    } else {
      // Line has not been terminated by \r\n
      HTTPS_LOGW("Line without \\r\\n (got only \\r). FID=%d", _socket);
      raiseError(400, "Bad Request");
      return;
    }
  } else {
	return;
  }
} else {
  _parserLine.text += newChar;
  _bufferProcessed += 1;
}

// Check that the max request string size is not exceeded
if (_parserLine.text.length() > lengthLimit) {
  HTTPS_LOGW("Header length exceeded. FID=%d", _socket);
  raiseError(431, "Request Header Fields Too Large");
  return;
}

}
}

// ===== the updated "case STATE_REQUEST_FINISHED:" section

case STATE_REQUEST_FINISHED: // Read headers

  while (_bufferProcessed < _bufferUnusedIdx && !isClosed()) {
    readLine(HTTPS_REQUEST_MAX_HEADER_LENGTH);

    if (_parserLine.parsingFinished && _connectionState != STATE_ERROR) {

      if (_parserLine.text.empty()) {
        HTTPS_LOGD("Headers finished, FID=%d", _socket);
        _connectionState = STATE_HEADERS_FINISHED;

        // Break, so that the rest of the body does not get flushed through
        _parserLine.parsingFinished = false;
        _parserLine.text = "";
        break;
      } else {
        int idxColon = _parserLine.text.find(':');
        if ( (idxColon != std::string::npos) && (_parserLine.text[idxColon+1]==' ') ) {
          _httpHeaders->set(new HTTPHeader(
              _parserLine.text.substr(0, idxColon),
              _parserLine.text.substr(idxColon+2)
          ));
          HTTPS_LOGD("Header: %s = %s (FID=%d)", _parserLine.text.substr(0, idxColon).c_str(), _parserLine.text.substr(idxColon+2).c_str(), _socket);
        } else {
          HTTPS_LOGW("Malformed request header: %s", _parserLine.text.c_str());
          raiseError(400, "Bad Request");
          break;
        }
      }

      _parserLine.parsingFinished = false;
      _parserLine.text = "";
	  
    } else if (_bufferProcessed < _bufferUnusedIdx && _receiveBuffer[_bufferProcessed] == '\r') {
      break;
    }
  }

  break;

[pnegre]

Thanks!

I hope the maintainers will fix it.