Use-after-free in AsyncWebServerRequest::_removeNotInterestingHeaders

[depau]

Hi, While running my code with Valgrind in an effort to troubleshoot memory-corruption issues I noticed there's a use-after-free error in this library's code.

Here the function iterates over a linked list of headers, and removes not-interesting ones from the list. https://github.com/me-no-dev/ESPAsyncWebServer/blob/master/src/WebRequest.cpp#L183-L186

Here, the linked list's remove method correctly updates the next pointer and frees the removed item:

https://github.com/me-no-dev/ESPAsyncWebServer/blob/master/src/StringArray.h#L120-L132

However, if an item is deleted during iteration, the iterator still holds a reference to its memory, and it still uses it to retrieve the next pointer:

https://github.com/me-no-dev/ESPAsyncWebServer/blob/master/src/StringArray.h#L53

While this works in most cases and particularly on the ESP MCUs, on my testing setup I get a nice SIGSEGV.

A potential fix could be retrieving the next pointer ahead of time and storing it in the iterator. Then in operator++ replace it with the "next-next" pointer and return the stored next pointer. This should make deleting the current item safe.

---

(If you're wondering how the hell I'm running it with valgrind, I stubbed all the Arduino calls and patched ESPAsyncTCP to use the POSIX socket API, and I'm running my application as a regular Linux program)

[depau]

This branch has all my pull requests: https://github.com/Depau/ESPAsyncWebServer/tree/wi-se-patches

On Tue, May 25, 2021, 09:12 zekageri @.***> wrote:

Do you have a fork to try with the fixes?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/me-no-dev/ESPAsyncWebServer/issues/951#issuecomment-847609844, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIZCWIV32N3VR5JSBU5LEDTPNEWRANCNFSM4YX3HCKA .

[zekageri]

Thanks. I found one before but I think it wasnt that one. I will try it. Iam experiencing random crashes on my esp and it seems it is because this linked list problem. I cant debug because its remote and I cant put serial to it

[depau]

If you'd like to run your ESP application on a Linux computer, I wrote a bunch of stubs to do so: https://github.com/Depau/wi-se-sw/tree/main/fakeesp This is what allowed me to run this library with Valgrind/GDB and find the issues.

YMMV though, and I don't have time to provide any supports on them. Feel free to do whatever you want with the code, if you have any improvements lmk.

[zekageri]

Thank you for the help. Your version of the library seems pretty stable. It runs almost 4 days straight now. I got a crash like in 20 hours before. But i really get stable consistent file servings and client connection/disconnetions. I will look at your linux thingy too. I really appreciate these and hope Me_no_Dev will see your fixes

[stale[bot]]

[STALE_SET] This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

[depau]

Unstale, still waiting for this to be reviewed

[stale[bot]]

[STALE_CLR] This issue has been removed from the stale queue. Please ensure activity to keep it openin the future.

[brandonros]

@Depau can you please pull latest me-no-dev master into your ``wi-se-patches` branch and push? It does not build otherwise.

[stale[bot]]

[STALE_SET] This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

[depau]

Unstale

[stale[bot]]

[STALE_CLR] This issue has been removed from the stale queue. Please ensure activity to keep it openin the future.

[szbeni]

Is this bug fix going to be merged?

[stale[bot]]

[STALE_SET] This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

[softhack007]

Is this bugfix already integrated?

[stale[bot]]

[STALE_CLR] This issue has been removed from the stale queue. Please ensure activity to keep it openin the future.

[depau]

Is this bugfix already integrated?

No

[softhack007]

@me-no-dev Is it planned to integrate this fix ? (https://github.com/me-no-dev/ESPAsyncWebServer/pull/952). We are currently trying to port our software (WLED) to the new MCU's, and this one seems to cause troubles on -C3 and (maybe) -S2.

Another problem we run into on -S3 and -S2 is described in https://github.com/me-no-dev/ESPAsyncWebServer/issues/1210 - "[E][vfs_api.cpp:29] open(): myFile.txt does not start with /"

Would be great if these issues could be solved. We use LittleFS from arduino-esp32 v2.0.4, which seems to behave little different from the original "Lorol LittleFS" that we used for arduino-esp32 v1.0.6.

[jacopomaroli]

I opened a PR against the wi-se-patches branch here https://github.com/depau/ESPAsyncWebServer/pull/1 to align it with its upstream

it's not much but at least if you're using a recent version of esp-idf with platformio you can simply point it to the fixed and up-to-date version with

lib_deps =
    ESP Async WebServer=https://github.com/jacopomaroli/ESPAsyncWebServer/archive/e0176a52ed643d5afa7bb534e7dcd643f4c1ab28.zip

[stale[bot]]

[STALE_SET] This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

[A40in]

replace in WebRequest.cpp

void AsyncWebServerRequest::_removeNotInterestingHeaders(){
  if (_interestingHeaders.containsIgnoreCase("ANY")) return; // nothing to do
  StringArray *a = &_interestingHeaders;
  auto cmpr = LinkedList<AsyncWebHeader*>::Predicate{[&a](AsyncWebHeader* h) {
    return !a->containsIgnoreCase(h->name().c_str());
  }};
  while (_headers.remove_first(cmpr)){}
}

or

void AsyncWebServerRequest::_removeNotInterestingHeaders(){
  if (_interestingHeaders.containsIgnoreCase("ANY")) return; // nothing to do
  StringArray *a = &_interestingHeaders;
  auto cmpr = LinkedList<AsyncWebHeader*>::Predicate{[&a](AsyncWebHeader* h) {
    return !a->containsIgnoreCase(h->name().c_str());
  }};
  _headers.remove_all(cmpr);
}

and add metod to class LinkedList in StringArray.h

    bool remove_all(Predicate predicate){
      auto it = _root;
      auto pit = _root;
      bool flag = false;
      while (it) {
        if(predicate(it->value())){
          auto d = it;
          if (it == _root) {
            _root = _root->next;
            pit   = _root;
            it    = _root;
          } else {
            pit->next = it->next;
            it = pit->next;
          }
          if (_onRemove) {
            _onRemove(d->value());
          }
          delete d;
          flag = true;
        } else {
          pit = it;
          it = it->next;
        }
      }
      return flag;
    }

[stale[bot]]

[STALE_CLR] This issue has been removed from the stale queue. Please ensure activity to keep it openin the future.