[Bug Report] Use after free in debug.upvaluejoin

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[Bug Report] Use after free in debug.upvaluejoin

fady osman
Hi there,
I found a heap-user-after-free bug in lua-5.3.5.

The function `lua_upvaluejoin` in file lapi.c at line 1287 suffers from a use after free bug when supplied the same function for parameter f1 and f2 and the same upvalue index, additionally the bug happens only when the upvalue is closed, this happens because the `luaC_upvdeccount` function found in file lgc.c at line 678 will decrement the refcount and then free the upvalue if the refcount is zero and if the upvalue is closed.
See the comments below for more explanation.
--------------
LUA_API void lua_upvaluejoin (lua_State *L, int fidx1, int n1,
                                            int fidx2, int n2) {
  LClosure *f1;
  UpVal **up1 = getupvalref(L, fidx1, n1, &f1);
  UpVal **up2 = getupvalref(L, fidx2, n2, NULL);
  luaC_upvdeccount(L, *up1);      //Will delete up1
  *up1 = *up2;     //up1 is up2 because it's the same upvalue and now it's freed.
  (*up1)->refcount++;   //up1 is freed, yet it's used here.
  if (upisopen(*up1)) (*up1)->u.open.touched = 1;
  luaC_upvalbarrier(L, *up1);
}
--------------

To trigger the bug simply use a lua program like this (this one will crash):
--
f=load(function() end)
interesting={}
interesting[0]=string.rep("A",512)
debug.upvaluejoin(f,1,f,1)
---
Another program that will not crash (unless you use -fsanitize=address):
---
function w() 
local x = {}
f = function() print(x) end
end
w()
debug.upvaluejoin(f,2,f,2)
---

Regards,

Fady Othman
Information Security Consultant

Security Consultant # ZINAD IT
G006D-THUB, Dubai Silicon Oasis, Dubai, U.A.E

Reply | Threaded
Open this post in threaded view
|

Re: [Bug Report] Use after free in debug.upvaluejoin

Philippe Verdy
Do you suggest...

Le jeu. 10 janv. 2019 à 14:49, fady osman <[hidden email]> a écrit :
Hi there,
I found a heap-user-after-free bug in lua-5.3.5.

The function `lua_upvaluejoin` in file lapi.c at line 1287 suffers from a use after free bug when supplied the same function for parameter f1 and f2 and the same upvalue index, additionally the bug happens only when the upvalue is closed, this happens because the `luaC_upvdeccount` function found in file lgc.c at line 678 will decrement the refcount and then free the upvalue if the refcount is zero and if the upvalue is closed.
--------------
LUA_API void lua_upvaluejoin (lua_State *L, int fidx1, int n1,
                                            int fidx2, int n2) {
  LClosure *f1;
  UpVal **up1 = getupvalref(L, fidx1, n1, &f1);
  UpVal **up2 = getupvalref(L, fidx2, n2, NULL);
 
... moving this line:
  luaC_upvdeccount(L, *up1);      //Will delete up1

... below this: 
  *up1 = *up2;     //up1 is up2 because it's the same upvalue and now it's freed.
  (*up1)->refcount++;   //up1 is freed, yet it's used here.
  if (upisopen(*up1)) (*up1)->u.open.touched = 1; // yet it's also used here
  luaC_upvalbarrier(L, *up1); // used here also???

...i.e. here ?
}

Reply | Threaded
Open this post in threaded view
|

Re: [Bug Report] Use after free in debug.upvaluejoin

Philippe Verdy


Le jeu. 10 janv. 2019 à 15:21, Philippe Verdy <[hidden email]> a écrit :
Do you suggest...

Le jeu. 10 janv. 2019 à 14:49, fady osman <[hidden email]> a écrit :
Hi there,
I found a heap-user-after-free bug in lua-5.3.5.

The function `lua_upvaluejoin` in file lapi.c at line 1287 suffers from a use after free bug when supplied the same function for parameter f1 and f2 and the same upvalue index, additionally the bug happens only when the upvalue is closed, this happens because the `luaC_upvdeccount` function found in file lgc.c at line 678 will decrement the refcount and then free the upvalue if the refcount is zero and if the upvalue is closed.
--------------
LUA_API void lua_upvaluejoin (lua_State *L, int fidx1, int n1,
                                            int fidx2, int n2) {
  LClosure *f1;
  UpVal **up1 = getupvalref(L, fidx1, n1, &f1);
  UpVal **up2 = getupvalref(L, fidx2, n2, NULL);
 
... moving this line:
  luaC_upvdeccount(L, *up1);      //Will delete up1

... below this: 
  *up1 = *up2;     //up1 is up2 because it's the same upvalue and now it's freed.
  (*up1)->refcount++;   //up1 is freed, yet it's used here.
  if (upisopen(*up1)) (*up1)->u.open.touched = 1; // yet it's also used here
  luaC_upvalbarrier(L, *up1); // used here also???

...i.e. here ?
}

This would then be:
LUA_API void lua_upvaluejoin (lua_State *L, int fidx1, int n1,
                                            int fidx2, int n2) {
  // LClosure *f1; // commented out, not used ???
  UpVal **up1 = getupvalref(L, fidx1, n1, /*&f1*/NULL); // commented out f1, not used???
  UpVal **up2 = getupvalref(L, fidx2, n2, NULL);
   (*up2)->refcount++;   //up2 is not freed
  if (upisopen(*up2)) (*up2)->u.open.touched = 1; // it's also used here
  luaC_upvalbarrier(L, *up2); // used here also???
  luaC_upvdeccount(L, *up1); //Will delete up1
  *up1 = *up2; // safe now to change it

Reply | Threaded
Open this post in threaded view
|

Re: [Bug Report] Use after free in debug.upvaluejoin

fady osman
Looks good to me,
I tested the new code with the test cases with address sanitizer enabled and no UAF detected and there was no crashes.
However, I don't know if it's necessary to do the joining if it's already the same upvalue, I mean it's already the same so can we do something like:
if(*up1 == *up2) return;
so the code becomes:

  UpVal **up1 = getupvalref(L, fidx1, n1, &f1);
  UpVal **up2 = getupvalref(L, fidx2, n2, NULL);
  if(*up1 == *up2) return;      //Already joined.
  luaC_upvdeccount(L, *up1);
  *up1 = *up2;
  (*up1)->refcount++;
  if (upisopen(*up1)) (*up1)->u.open.touched = 1;
  luaC_upvalbarrier(L, *up1);




On Thu, Jan 10, 2019 at 4:29 PM Philippe Verdy <[hidden email]> wrote:


Le jeu. 10 janv. 2019 à 15:21, Philippe Verdy <[hidden email]> a écrit :
Do you suggest...

Le jeu. 10 janv. 2019 à 14:49, fady osman <[hidden email]> a écrit :
Hi there,
I found a heap-user-after-free bug in lua-5.3.5.

The function `lua_upvaluejoin` in file lapi.c at line 1287 suffers from a use after free bug when supplied the same function for parameter f1 and f2 and the same upvalue index, additionally the bug happens only when the upvalue is closed, this happens because the `luaC_upvdeccount` function found in file lgc.c at line 678 will decrement the refcount and then free the upvalue if the refcount is zero and if the upvalue is closed.
--------------
LUA_API void lua_upvaluejoin (lua_State *L, int fidx1, int n1,
                                            int fidx2, int n2) {
  LClosure *f1;
  UpVal **up1 = getupvalref(L, fidx1, n1, &f1);
  UpVal **up2 = getupvalref(L, fidx2, n2, NULL);
 
... moving this line:
  luaC_upvdeccount(L, *up1);      //Will delete up1

... below this: 
  *up1 = *up2;     //up1 is up2 because it's the same upvalue and now it's freed.
  (*up1)->refcount++;   //up1 is freed, yet it's used here.
  if (upisopen(*up1)) (*up1)->u.open.touched = 1; // yet it's also used here
  luaC_upvalbarrier(L, *up1); // used here also???

...i.e. here ?
}

This would then be:
LUA_API void lua_upvaluejoin (lua_State *L, int fidx1, int n1,
                                            int fidx2, int n2) {
  // LClosure *f1; // commented out, not used ???
  UpVal **up1 = getupvalref(L, fidx1, n1, /*&f1*/NULL); // commented out f1, not used???
  UpVal **up2 = getupvalref(L, fidx2, n2, NULL);
   (*up2)->refcount++;   //up2 is not freed
  if (upisopen(*up2)) (*up2)->u.open.touched = 1; // it's also used here
  luaC_upvalbarrier(L, *up2); // used here also???
  luaC_upvdeccount(L, *up1); //Will delete up1
  *up1 = *up2; // safe now to change it



--

Fady Othman
Information Security Consultant

Security Consultant # ZINAD IT
G006D-THUB, Dubai Silicon Oasis, Dubai, U.A.E

Reply | Threaded
Open this post in threaded view
|

Re: [Bug Report] Use after free in debug.upvaluejoin

Philippe Verdy
Why do you need to pass &f1 (locally declared LClosure *f1) ? does it make a reference to be counted (but then not decremented) ?

Le jeu. 10 janv. 2019 à 15:57, fady osman <[hidden email]> a écrit :
Looks good to me,
I tested the new code with the test cases with address sanitizer enabled and no UAF detected and there was no crashes.
However, I don't know if it's necessary to do the joining if it's already the same upvalue, I mean it's already the same so can we do something like:
if(*up1 == *up2) return;
so the code becomes:

  UpVal **up1 = getupvalref(L, fidx1, n1, &f1);
  UpVal **up2 = getupvalref(L, fidx2, n2, NULL);
  if(*up1 == *up2) return;      //Already joined.
  luaC_upvdeccount(L, *up1);
  *up1 = *up2;
  (*up1)->refcount++;
  if (upisopen(*up1)) (*up1)->u.open.touched = 1;
  luaC_upvalbarrier(L, *up1);




On Thu, Jan 10, 2019 at 4:29 PM Philippe Verdy <[hidden email]> wrote:


Le jeu. 10 janv. 2019 à 15:21, Philippe Verdy <[hidden email]> a écrit :
Do you suggest...

Le jeu. 10 janv. 2019 à 14:49, fady osman <[hidden email]> a écrit :
Hi there,
I found a heap-user-after-free bug in lua-5.3.5.

The function `lua_upvaluejoin` in file lapi.c at line 1287 suffers from a use after free bug when supplied the same function for parameter f1 and f2 and the same upvalue index, additionally the bug happens only when the upvalue is closed, this happens because the `luaC_upvdeccount` function found in file lgc.c at line 678 will decrement the refcount and then free the upvalue if the refcount is zero and if the upvalue is closed.
--------------
LUA_API void lua_upvaluejoin (lua_State *L, int fidx1, int n1,
                                            int fidx2, int n2) {
  LClosure *f1;
  UpVal **up1 = getupvalref(L, fidx1, n1, &f1);
  UpVal **up2 = getupvalref(L, fidx2, n2, NULL);
 
... moving this line:
  luaC_upvdeccount(L, *up1);      //Will delete up1

... below this: 
  *up1 = *up2;     //up1 is up2 because it's the same upvalue and now it's freed.
  (*up1)->refcount++;   //up1 is freed, yet it's used here.
  if (upisopen(*up1)) (*up1)->u.open.touched = 1; // yet it's also used here
  luaC_upvalbarrier(L, *up1); // used here also???

...i.e. here ?
}

This would then be:
LUA_API void lua_upvaluejoin (lua_State *L, int fidx1, int n1,
                                            int fidx2, int n2) {
  // LClosure *f1; // commented out, not used ???
  UpVal **up1 = getupvalref(L, fidx1, n1, /*&f1*/NULL); // commented out f1, not used???
  UpVal **up2 = getupvalref(L, fidx2, n2, NULL);
   (*up2)->refcount++;   //up2 is not freed
  if (upisopen(*up2)) (*up2)->u.open.touched = 1; // it's also used here
  luaC_upvalbarrier(L, *up2); // used here also???
  luaC_upvdeccount(L, *up1); //Will delete up1
  *up1 = *up2; // safe now to change it



--

Fady Othman
Information Security Consultant

Security Consultant # ZINAD IT
G006D-THUB, Dubai Silicon Oasis, Dubai, U.A.E

Reply | Threaded
Open this post in threaded view
|

Re: [Bug Report] Use after free in debug.upvaluejoin

fady osman
I did it by mistake, I copied the old code but I think passing NULL will also work without problems so it can be removed.

On Thu, Jan 10, 2019 at 6:49 PM Philippe Verdy <[hidden email]> wrote:
Why do you need to pass &f1 (locally declared LClosure *f1) ? does it make a reference to be counted (but then not decremented) ?

Le jeu. 10 janv. 2019 à 15:57, fady osman <[hidden email]> a écrit :
Looks good to me,
I tested the new code with the test cases with address sanitizer enabled and no UAF detected and there was no crashes.
However, I don't know if it's necessary to do the joining if it's already the same upvalue, I mean it's already the same so can we do something like:
if(*up1 == *up2) return;
so the code becomes:

  UpVal **up1 = getupvalref(L, fidx1, n1, &f1);
  UpVal **up2 = getupvalref(L, fidx2, n2, NULL);
  if(*up1 == *up2) return;      //Already joined.
  luaC_upvdeccount(L, *up1);
  *up1 = *up2;
  (*up1)->refcount++;
  if (upisopen(*up1)) (*up1)->u.open.touched = 1;
  luaC_upvalbarrier(L, *up1);




On Thu, Jan 10, 2019 at 4:29 PM Philippe Verdy <[hidden email]> wrote:


Le jeu. 10 janv. 2019 à 15:21, Philippe Verdy <[hidden email]> a écrit :
Do you suggest...

Le jeu. 10 janv. 2019 à 14:49, fady osman <[hidden email]> a écrit :
Hi there,
I found a heap-user-after-free bug in lua-5.3.5.

The function `lua_upvaluejoin` in file lapi.c at line 1287 suffers from a use after free bug when supplied the same function for parameter f1 and f2 and the same upvalue index, additionally the bug happens only when the upvalue is closed, this happens because the `luaC_upvdeccount` function found in file lgc.c at line 678 will decrement the refcount and then free the upvalue if the refcount is zero and if the upvalue is closed.
--------------
LUA_API void lua_upvaluejoin (lua_State *L, int fidx1, int n1,
                                            int fidx2, int n2) {
  LClosure *f1;
  UpVal **up1 = getupvalref(L, fidx1, n1, &f1);
  UpVal **up2 = getupvalref(L, fidx2, n2, NULL);
 
... moving this line:
  luaC_upvdeccount(L, *up1);      //Will delete up1

... below this: 
  *up1 = *up2;     //up1 is up2 because it's the same upvalue and now it's freed.
  (*up1)->refcount++;   //up1 is freed, yet it's used here.
  if (upisopen(*up1)) (*up1)->u.open.touched = 1; // yet it's also used here
  luaC_upvalbarrier(L, *up1); // used here also???

...i.e. here ?
}

This would then be:
LUA_API void lua_upvaluejoin (lua_State *L, int fidx1, int n1,
                                            int fidx2, int n2) {
  // LClosure *f1; // commented out, not used ???
  UpVal **up1 = getupvalref(L, fidx1, n1, /*&f1*/NULL); // commented out f1, not used???
  UpVal **up2 = getupvalref(L, fidx2, n2, NULL);
   (*up2)->refcount++;   //up2 is not freed
  if (upisopen(*up2)) (*up2)->u.open.touched = 1; // it's also used here
  luaC_upvalbarrier(L, *up2); // used here also???
  luaC_upvdeccount(L, *up1); //Will delete up1
  *up1 = *up2; // safe now to change it



--

Fady Othman
Information Security Consultant

Security Consultant # ZINAD IT
G006D-THUB, Dubai Silicon Oasis, Dubai, U.A.E



--

Fady Othman
Information Security Consultant

Security Consultant # ZINAD IT
G006D-THUB, Dubai Silicon Oasis, Dubai, U.A.E

Reply | Threaded
Open this post in threaded view
|

Re: [Bug Report] Use after free in debug.upvaluejoin

Roberto Ierusalimschy
> I did it by mistake, I copied the old code but I think passing NULL will
> also work without problems so it can be removed.

First, thanks for the bug report.

About this argument, the parameter 'uv' in 'aux_upvalue' could be
removed. It was left over in commit af35c7f398e8.

-- Roberto