Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
depau opened this issue Mar 7, 2021 · 21 comments · May be fixed by #952 or #1452
Open

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

depau opened this issue Mar 7, 2021 · 21 comments · May be fixed by #952 or #1452

Comments

@depau
Copy link

depau commented Mar 7, 2021

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
Copy link
Author

depau commented May 25, 2021 via email

@zekageri
Copy link

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
Copy link
Author

depau commented May 31, 2021

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
Copy link

zekageri commented Jun 1, 2021

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
Copy link

stale bot commented Jul 31, 2021

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

@stale stale bot added the stale label Jul 31, 2021
@depau
Copy link
Author

depau commented Aug 1, 2021

Unstale, still waiting for this to be reviewed

@stale
Copy link

stale bot commented Aug 1, 2021

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

@brandonros
Copy link

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

@stale
Copy link

stale bot commented Mar 30, 2022

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

@stale stale bot added the stale label Mar 30, 2022
@depau
Copy link
Author

depau commented Apr 5, 2022

Unstale

@stale
Copy link

stale bot commented Apr 5, 2022

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

@stale stale bot removed the stale label Apr 5, 2022
@szbeni
Copy link

szbeni commented May 11, 2022

Is this bug fix going to be merged?

@stale
Copy link

stale bot commented Jul 10, 2022

[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
Copy link

Is this bugfix already integrated?

@stale
Copy link

stale bot commented Sep 18, 2022

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

@stale stale bot removed the stale label Sep 18, 2022
@depau
Copy link
Author

depau commented Sep 18, 2022

Is this bugfix already integrated?

No

@softhack007
Copy link

softhack007 commented Sep 18, 2022

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

@jacopomaroli
Copy link

jacopomaroli commented Mar 10, 2023

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
Copy link

stale bot commented May 21, 2023

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

@stale stale bot added the stale label May 21, 2023
willmmiles added a commit to willmmiles/ESPAsyncWebServer that referenced this issue Jan 28, 2024
Upgrade the list structure to support quick insertion, and fix a
use-after-free in _removeNotInterestingHeaders.  This bug has been
reported and "fixed" many times upstream, but several of those fixes
seem to be bad.

See me-no-dev/ESPAsyncWebServer me-no-dev#951, me-no-dev#837
willmmiles added a commit to willmmiles/ESPAsyncWebServer that referenced this issue Feb 2, 2024
Upgrade the list structure to support quick insertion, and fix a
use-after-free in _removeNotInterestingHeaders.  This bug has been
reported and "fixed" many times upstream, but several of those fixes
seem to be bad.

See me-no-dev/ESPAsyncWebServer me-no-dev#951, me-no-dev#837
@A40in
Copy link

A40in commented Oct 17, 2024

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;
    }

Copy link

stale bot commented Oct 17, 2024

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

@stale stale bot removed the stale label Oct 17, 2024
@controlol controlol linked a pull request Dec 12, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants