Use-after-free in AsyncWebServerRequest::_removeNotInterestingHeaders

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)

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 <#951 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIZCWIV32N3VR5JSBU5LEDTPNEWRANCNFSM4YX3HCKA> .

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

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.

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_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.

Unstale, still waiting for this to be reviewed

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

@depau

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

[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.

Unstale

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

[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.

Is this bugfix already integrated?

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

Is this bugfix already integrated?

No

@me-no-dev

@me-no-dev Is it planned to integrate this fix ? (#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 #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.

I opened a PR against the wi-se-patches branch here depau#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_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.

Fix _removeNotInterestingHeaders

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_CLR] This issue has been removed from the stale queue. Please ensure activity to keep it openin the future.

Navigation Menu

Repository navigation

Use-after-free in AsyncWebServerRequest::_removeNotInterestingHeaders #951

1 remaining item

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Use-after-free in AsyncWebServerRequest::_removeNotInterestingHeaders #951

Description

Activity

depau commented on May 25, 2021

zekageri commented on May 26, 2021

depau commented on May 31, 2021

zekageri commented on Jun 1, 2021

stale commented on Jul 31, 2021

depau commented on Aug 1, 2021

stale commented on Aug 1, 2021

brandonros commented on Aug 16, 2021

stale commented on Mar 30, 2022

depau commented on Apr 5, 2022

stale commented on Apr 5, 2022

1 remaining item

stale commented on Jul 10, 2022

softhack007 commented on Sep 18, 2022

stale commented on Sep 18, 2022

depau commented on Sep 18, 2022

softhack007 commented on Sep 18, 2022

jacopomaroli commented on Mar 10, 2023

stale commented on May 21, 2023

A40in commented on Oct 17, 2024

stale commented on Oct 17, 2024

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions