AddressSanitizer: heap-use-after-free /home/yongheng/mujs/jsrun.c:1362 in jsR_run #136
Comments
|
Is this issue reproducible? Does it still happen if you revert 331c5ec "Issue 133: Eliminate recursion in GC scanning phase" ? |
|
@avih Yes, I just checkout the commit and tried. The poc still works. And this also works in the latest commit, as I wrote in the issue above. |
|
I meant if you revert that commit. I.e. either try the commit before it - 8c5f2f2 or just run |
|
Oh. If I revert the commit, the poc didn't work. |
Interesting. I suspect use-after-free on master, and suspect commit 331c5ec. The fact that it didn't happen with that commit reverted doesn't prove the suspition, but I think it supports it. |
|
I think this should fix it: From 3710aa49a88faef2084a1477babc5272086064a2 Mon Sep 17 00:00:00 2001
From: "Avi Halachmi (:avih)" <avihpit@yahoo.com>
Date: Fri, 3 Jul 2020 23:16:22 +0300
Subject: [PATCH] gc: fix incorrect free of some objects
When scanning an iterator object, the iterated object was marked
unconditionally. Now it's marked only if it's not already marked - like
all other object markings.
This code was incorrect for some years, but wasn't really an issue
before commit 331c5ec because marking an object twice simply used some
more CPU cycles but otherwise without issues - unless there were cycles,
and apparently typically/always there never were cycles with iterators,
so it was hard/impossible to behave badly.
However, since 331c5ec, marking an object means inserting it into a
linked list where the list nodes are part of the object, therefore
marking the same object twice now creates a broken linked list.
A broken list means that some objects are skipped while scanned, which
means they don't get marked even when they should, and as a result
freed incorrectly while still referenced by other objects, resulting in
random errors related to use-after-free.
---
jsgc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/jsgc.c b/jsgc.c
index da6dfb7..dba35e9 100644
--- a/jsgc.c
+++ b/jsgc.c
@@ -100,7 +100,7 @@ static void jsG_scanobject(js_State *J, int mark, js_Object *obj)
jsG_markproperty(J, mark, obj->properties);
if (obj->prototype && obj->prototype->gcmark != mark)
jsG_markobject(J, mark, obj->prototype);
- if (obj->type == JS_CITERATOR) {
+ if (obj->type == JS_CITERATOR && obj->u.iter.target->gcmark != mark) {
jsG_markobject(J, mark, obj->u.iter.target);
}
if (obj->type == JS_CFUNCTION || obj->type == JS_CSCRIPT || obj->type == JS_CEVAL) {
--
2.17.1 |
|
@Changochen It should be fixed in master now. Can you please test your use case again and confirm it's fixed? |
|
hi @avih. Yeah I tested it and the problem has gone |
|
Thanks. |
|
Side-note, when reading daily CVE and other security reports on behalf of pkgsrc Security Team it seems that CVE-2020-24343 was requested for this issue. Quoting it directly inline for convenience:
According the analysis done by @avih I think that this is wrong, i.e. the issue was introduced after 1.0.7 and was never present in a stable MuJS release. If that's the case I think it should be rejected - normally no CVE are requested for development version. If anyone can confirm that, please let me know and I will go ahead requesting to reject it and/or feel free to do yourself via https://cveform.mitre.org/ Thank you! |
FWIW, i follow the sqlite3 forum closely that it happens all the time that CVEs are filed against non-release sqlite3 versions, sometimes even those in non-trunk development branches. That project's policy is not to track CVEs: https://www.sqlite.org/cves.html See also: https://sqlite.org/forum/forumpost/5230bca319 |
This is correct in this context - i.e. when talking of use-after-free. However, I probably wasn't clear enough in my description, which coud have led to this misunderstanding.
So the source of the issue is old, but was with trivial implocations, while a recent commit (after 1.0.7) changed its implication to use-after-free. |
|
Hi. I thought UAF existed in the release since the root cause is in a stable release. But it seems this is wrong. In that case, I might need to make a request to the CVE team to dispute this CVE. Does this make sense? Sorry for the trouble. |
FYI, the description is from the CVE team, they sometimes changed the reporters' description according to their analysis |
That seems to me an entirely incorrect statement. Unconditional marking at 1.0.7 or earlier could lead, as far as I can tell, to stack overflow at most - due to infinite recursion. I'm not even sure if infinite recursion could occure, because I think the recursion would be broken once any other object at the cycle is reached again while already marked (the bug of not checking the marking only applied, as far as I can tell, to one case - an iterator object - which the patch fixes). The GC in MuJS works like this:
Marking does not and did not do new memory allocations. It was recursing, and now it does not, and instead it links objects which should be marked using pointers which already exist at the object. With this new list, Inserting the same object twice also never creates an unterminated-list. It simply may cause some objects in that list to be skipped when the list is iterated later. So skipping objects while marking (which is the thing which can lead to incorrect free) at 1.0.7 or earlier was never an issue. It was the other way at most - possibly marking them more than once - which doesn't have negative effect except if it leads to an infinite recursion - which I'm almost sure could not happen anyway. |
|
I see. I will make a request to dispute this CVE. Since there is reference in the CVE info to this issue, people can just visit this issue to check what happened. |
|
Request made. The status of the CVE should be updated soon. |
git hash:
9f3e141d805cddec7cce1fae38373a82c61e3300cmd:
mujs poc.jsPOC:
Stack dump:
The text was updated successfully, but these errors were encountered: