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

Fix use-after-free on error during module evaluation #26

Merged
merged 2 commits into from
Nov 7, 2023

Conversation

saghul
Copy link
Contributor

@saghul saghul commented Nov 7, 2023

E.g. if during evaluation of module A, we start loading module B and an error occurs. This results in a call to js_free_modules() with JS_FREE_MODULE_NOT_EVALUATED, and since module A isn't yet evaluated, it gets freed prematurely.

To solve this we improve js_free_modules() to ensure eval_mark is not set. Once js_evaluate_module() returns for module A, it will notice that an exception occurred and call js_free_modules() with JS_FREE_MODULE_NOT_EVALUATED. Since eval_mark has been cleared by then, module A gets cleaned up as well.

E.g. if during evaluation of module A, we start loading module B and
an error occurs. This results in a call to js_free_modules() with
JS_FREE_MODULE_NOT_EVALUATED, and since module A isn't yet evaluated,
it gets freed prematurely.

To solve this we improve js_free_modules() to ensure `eval_mark` is not
set. Once js_evaluate_module() returns for module A, it will notice that
an exception occurred and call js_free_modules() with
JS_FREE_MODULE_NOT_EVALUATED. Since `eval_mark` has been cleared by then,
module A gets cleaned up as well.
@saghul
Copy link
Contributor Author

saghul commented Nov 7, 2023

Borrowed from the Frida fork, I noticed it fixes a bug I was facing in txiki.js!

@saghul saghul requested a review from bnoordhuis November 7, 2023 05:35
saghul added a commit to saghul/txiki.js that referenced this pull request Nov 7, 2023
Copy link
Contributor

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but does this mean the module isn't freed until JS_FreeContext() is called?

@saghul
Copy link
Contributor Author

saghul commented Nov 7, 2023

I can check that. I think it will be freed when the dependent module is freed. So if A imports B then B fails first B is freed then A after the eval is done.

saghul added a commit to saghul/txiki.js that referenced this pull request Nov 7, 2023
@saghul
Copy link
Contributor Author

saghul commented Nov 7, 2023

LGTM but does this mean the module isn't freed until JS_FreeContext() is called?

I tested and yeah, it's only freed at the end. Are we ok merging this still?

@saghul
Copy link
Contributor Author

saghul commented Nov 7, 2023

Pushed a fixup, which does free the non-evaluated modules now!

Copy link
Contributor

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@saghul saghul merged commit 4f02ab8 into master Nov 7, 2023
7 checks passed
@saghul saghul deleted the fix-modules-uaf branch November 7, 2023 21:23
saghul added a commit to saghul/txiki.js that referenced this pull request Nov 8, 2023
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.

3 participants