Stack overflow in lsys_load (lua/loadlib.c:134)

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

Stack overflow in lsys_load (lua/loadlib.c:134)

secwx weizhenvul
Stack overflow in lsys_load (lua/loadlib.c:134)

###### Lua revision

version : Lua 5.4.0

###### Build platform

Ubuntu 18.10 LTS (Linux ubuntu 4.18.0-25-generic x86_64)

###### Build steps

```
add -fsanitize=address -g to CFLAGS
make
```

###### Test case

Stack_overflow_lsys_load.lua
```
function main() 
 v3 = _VERSION.rep(1000000.0 ,1000000.0 )  
 v4 = package.loadlib(v3,v3)  
-- Stderr:
end 
main()
```

###### Execution steps

```
$ ls 
Stack_overflow_lsys_load.lua
$ ./Test_lua/lua/lua ./crashes/Stack_overflow_lsys_load.lua
AddressSanitizer:DEADLYSIGNAL
=================================================================
==90451==ERROR: AddressSanitizer: stack-overflow on address 0x7ffc56ae1e38 (pc 0x7f84c442acf7 bp 0x7ffc573773f0 sp 0x7ffc56ae1e40 T0)
    #0 0x7f84c442acf6  (/lib64/ld-linux-x86-64.so.2+0x7cf6)
    #1 0x7f84c442cd4d  (/lib64/ld-linux-x86-64.so.2+0x9d4d)
    #2 0x7f84c44379c3  (/lib64/ld-linux-x86-64.so.2+0x149c3)
    #3 0x7f84c31d148e in _dl_catch_exception (/lib/x86_64-linux-gnu/libc.so.6+0x15c48e)
    #4 0x7f84c44372c5  (/lib64/ld-linux-x86-64.so.2+0x142c5)
    #5 0x7f84c34a9255  (/lib/x86_64-linux-gnu/libdl.so.2+0x1255)
    #6 0x7f84c31d148e in _dl_catch_exception (/lib/x86_64-linux-gnu/libc.so.6+0x15c48e)
    #7 0x7f84c31d151e in _dl_catch_error (/lib/x86_64-linux-gnu/libc.so.6+0x15c51e)
    #8 0x7f84c34a9a24  (/lib/x86_64-linux-gnu/libdl.so.2+0x1a24)
    #9 0x7f84c34a92e5 in dlopen (/lib/x86_64-linux-gnu/libdl.so.2+0x12e5)
    #10 0x7f84c36c9a33  (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x8ea33)
    #11 0x56529c0e5908 in lsys_load /home/test/Lua/Test_lua/lua/loadlib.c:134
    #12 0x56529c0e5908 in lookforfunc /home/test/Lua/Test_lua/lua/loadlib.c:391
    #13 0x56529c0e5a14 in ll_loadlib /home/test/Lua/Test_lua/lua/loadlib.c:412
    #14 0x56529c09025c in luaD_call /home/test/Lua/Test_lua/lua/ldo.c:481
    #15 0x56529c0b8824 in luaV_execute /home/test/Lua/Test_lua/lua/lvm.c:1614
    #16 0x56529c0b8824 in luaV_execute /home/test/Lua/Test_lua/lua/lvm.c:1614
    #17 0x56529c090605 in luaD_callnoyield /home/test/Lua/Test_lua/lua/ldo.c:525
    #18 0x56529c08dc51 in luaD_rawrunprotected /home/test/Lua/Test_lua/lua/ldo.c:148
    #19 0x56529c0911e0 in luaD_pcall /home/test/Lua/Test_lua/lua/ldo.c:749
    #20 0x56529c086f8f in lua_pcallk /home/test/Lua/Test_lua/lua/lapi.c:1031
    #21 0x56529c08042a in docall /home/test/Lua/Test_lua/lua/lua.c:139
    #22 0x56529c08179d in handle_script /home/test/Lua/Test_lua/lua/lua.c:228
    #23 0x56529c08179d in pmain /home/test/Lua/Test_lua/lua/lua.c:603
    #24 0x56529c09025c in luaD_call /home/test/Lua/Test_lua/lua/ldo.c:481
    #25 0x56529c090605 in luaD_callnoyield /home/test/Lua/Test_lua/lua/ldo.c:525
    #26 0x56529c08dc51 in luaD_rawrunprotected /home/test/Lua/Test_lua/lua/ldo.c:148
    #27 0x56529c0911e0 in luaD_pcall /home/test/Lua/Test_lua/lua/ldo.c:749
    #28 0x56529c086f8f in lua_pcallk /home/test/Lua/Test_lua/lua/lapi.c:1031
    #29 0x56529c07fbda in main /home/test/Lua/Test_lua/lua/lua.c:629
    #30 0x7f84c309909a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
    #31 0x56529c080239 in _start (/home/test/Lua/Test_lua/lua/lua+0x18239)

SUMMARY: AddressSanitizer: stack-overflow (/lib64/ld-linux-x86-64.so.2+0x7cf6) 
==90451==ABORTING
```

```
./lua/lua Stack_overflow_lsys_load.lua
Segmentation fault
```

###### Backtrace

```
$ gdb -q -args Stack_overflow_lsys_load.lua
Reading symbols from ./lua/lua...(no debugging symbols found)...done.
(gdb) r
Starting program: /home/test/Lua/lua/lua Stack_overflow_lsys_load.lua

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff7fdacf7 in open_path (
    name=name@entry=0x7ffff6694028 "1000000.01000000.01000000.01000000.01000000.01000000.01000000.01000000.01000000.01000000.01000000.01000000.01000000.01000000.01000000.01000000.01000000.01000000.01000000.01000000.01000000.01000000.010"..., namelen=namelen@entry=9000001, mode=mode@entry=-1879048190, sps=sps@entry=0x7ffff7ffc8d0 <rtld_search_dirs>, realname=realname@entry=0x7fffffffd0f0, 
    fbp=fbp@entry=0x7fffffffd100, loader=0x7ffff7ffe190, whatcode=64, found_other_class=0x7fffffffd0ef) at dl-load.c:2041
2041    dl-load.c: No such file or directory.
(gdb) x/5i $rip
=> 0x7ffff7fdacf7 <open_path+215>:  callq  0x7ffff7ff3900 <mempcpy>
   0x7ffff7fdacfc <open_path+220>:  mov    %rax,%r14
   0x7ffff7fdacff <open_path+223>:  lea    -0xc0(%rbp),%rax
   0x7ffff7fdad06 <open_path+230>:  mov    %rax,-0xf8(%rbp)
   0x7ffff7fdad0d <open_path+237>:  mov    -0xd0(%rbp),%rax
(gdb) 
```
Reply | Threaded
Open this post in threaded view
|

Re: Stack overflow in lsys_load (lua/loadlib.c:134)

Roberto Ierusalimschy
> Stack overflow in lsys_load (lua/loadlib.c:134)
>
> [...]

This seems to be an issue with dlopen. Apparently, it is using a
stack-allocated array to hold the name of the file in its error handling.
As the file name is too big, that causes a stack overflow.

(*) That may not be a bug in dlopen if there is an explicit limit on
the length of its filename argument. I could not find such limit in its
documentation, so I am inclined to say that this is a bug in dlopen.

I know Linux and other systems have limits on actual file-name lenghts,
but dlopen must accept non-existent file names, too. For instance, if we
call fopen with such a name, it dutifully reports a "File name too long"
error.

-- Roberto
Reply | Threaded
Open this post in threaded view
|

Re: Stack overflow in lsys_load (lua/loadlib.c:134)

Ką Mykolas


On Fri, Sep 4, 2020, 16:49 Roberto Ierusalimschy <[hidden email]> wrote:
> Stack overflow in lsys_load (lua/loadlib.c:134)
>
> [...]
I know Linux and other systems have limits on actual file-name lenghts,
but dlopen must accept non-existent file names, too. For instance, if we
call fopen with such a name, it dutifully reports a "File name too long"
error.

-- Roberto

PATH_MAX from linux/limits.h :}

Also, some filesystems may have some arbitrary length limits as well.
Reply | Threaded
Open this post in threaded view
|

Re: Stack overflow in lsys_load (lua/loadlib.c:134)

Roberto Ierusalimschy
> > I know Linux and other systems have limits on actual file-name lenghts,
> > but dlopen must accept non-existent file names, too. For instance, if we
> > call fopen with such a name, it dutifully reports a "File name too long"
> > error.
> >
> > -- Roberto
> >
>
> PATH_MAX from linux/limits.h :}

Repeating myself:
I know Linux and other systems have limits on actual file-name lenghts
(e.g., PATH_MAX), but dlopen must accept non-existent file names, too.

I did not find any documentation saying that invalid file names
(filename arguments, in particular) should respect that limit, too.

-- Roberto
Reply | Threaded
Open this post in threaded view
|

Re: Stack overflow in lsys_load (lua/loadlib.c:134)

Roberto Ierusalimschy
In reply to this post by Roberto Ierusalimschy
> > Stack overflow in lsys_load (lua/loadlib.c:134)
> >
> > [...]

Python 3 has the same issue (which reinforces the blame on dlopen):

$ python3
Python 3.6.9 (default, Jul 17 2020, 12:50:27)
[GCC 8.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from ctypes import *
>>> x = 'a' * 10000000
>>> cdll.LoadLibrary(x)
Segmentation fault (core dumped)

-- Roberto
Reply | Threaded
Open this post in threaded view
|

Re: Stack overflow in lsys_load (lua/loadlib.c:134)

Ką Mykolas
On Fri, Sep 4, 2020 at 10:58 PM Roberto Ierusalimschy
<[hidden email]> wrote:
>
> > > Stack overflow in lsys_load (lua/loadlib.c:134)
> > >
> > > [...]

Valgrind on this matter:

> ==15401== Warning: client switching stacks?  SP change: 0x1ffeffd8a0 --> 0x1ffe801aa0
> ==15401==          to suppress, use: --max-stackframe=8371712 or greater
> ==15401== Stack overflow in thread #1: can't grow stack to 0x1ffe801000
> ==15401==
> ==15401== Process terminating with default action of signal 11 (SIGSEGV)
> ==15401==  Access not within mapped region at address 0x1FFE801A98
> ==15401== Stack overflow in thread #1: can't grow stack to 0x1ffe801000
> ==15401==    at 0x40062B6: open_path (dl-load.c:1963)

So, I wonder if it refers to some kind of "optimisation" putting the
whole darn string onto the stack instead of passing it by the address.
Reply | Threaded
Open this post in threaded view
|

Re: Stack overflow in lsys_load (lua/loadlib.c:134)

Ką Mykolas
Also, funnily, on Musl-C I've got this:

==16325== Conditional jump or move depends on uninitialised value(s)
==16325==    at 0x42EC0C: strlen (in /tmp/static_test/musl/lua-5.4.0/src/lua)
==16325==    by 0x40E588: luaS_new (lstring.c:265)
==16325==    by 0x401F22: lua_pushstring (lapi.c:515)
==16325==    by 0x41F82F: lsys_load (loadlib.c:135)
==16325==    by 0x41F82F: lookforfunc (loadlib.c:391)
==16325==    by 0x41F874: ll_loadlib (loadlib.c:412)
==16325==    by 0x4058EC: luaD_call (ldo.c:482)
==16325==    by 0x41274A: luaV_execute (lvm.c:1615)
==16325==    by 0x41274A: luaV_execute (lvm.c:1615)
==16325==    by 0x4059FB: luaD_callnoyield (ldo.c:526)
==16325==    by 0x404CD8: luaD_rawrunprotected (ldo.c:148)
==16325==    by 0x405CDD: luaD_pcall (ldo.c:749)
==16325==    by 0x402B96: lua_pcallk (lapi.c:1023)
==16325==  Uninitialised value was created
==16325==    at 0x432E81: __expand_heap (in
/tmp/static_test/musl/lua-5.4.0/src/lua)
==16325==    by 0x29EB233E9F05FB6F: ???

On Sat, Sep 5, 2020 at 12:06 AM Ką Mykolas <[hidden email]> wrote:

>
> On Fri, Sep 4, 2020 at 10:58 PM Roberto Ierusalimschy
> <[hidden email]> wrote:
> >
> > > > Stack overflow in lsys_load (lua/loadlib.c:134)
> > > >
> > > > [...]
>
> Valgrind on this matter:
>
> > ==15401== Warning: client switching stacks?  SP change: 0x1ffeffd8a0 --> 0x1ffe801aa0
> > ==15401==          to suppress, use: --max-stackframe=8371712 or greater
> > ==15401== Stack overflow in thread #1: can't grow stack to 0x1ffe801000
> > ==15401==
> > ==15401== Process terminating with default action of signal 11 (SIGSEGV)
> > ==15401==  Access not within mapped region at address 0x1FFE801A98
> > ==15401== Stack overflow in thread #1: can't grow stack to 0x1ffe801000
> > ==15401==    at 0x40062B6: open_path (dl-load.c:1963)
>
> So, I wonder if it refers to some kind of "optimisation" putting the
> whole darn string onto the stack instead of passing it by the address.
Reply | Threaded
Open this post in threaded view
|

Re: Stack overflow in lsys_load (lua/loadlib.c:134)

Philippe Verdy-2
In reply to this post by Roberto Ierusalimschy
I wonder what is the use of passing a 10MB long string as the "name" of the library to load. As far as I know, all I/O APIs should have length limitations in the filesystem. Though path lengths longer that 256 bytes are now becoming common in virtualized OSes and with containers, but still limited (in Windows the 256 characters limit exists by default but has to be explicitly removed by a security token, but it's then up to the application to perform early validation. I don't see anyway any use for having a single file name (independently of the length of the full path) to be larger than 256 bytes.
So a simple validation would just look for a "/" or null byte in this limit of 256 bytes, allowing then dlopen() to create longer paths is needed (from the dynamic link path environment).
Note that GCC uses a special implementation of dlopen() in its library, which also has its own "strange" (but documented) bug as it uses an internal structure with addresses that are resolved incorrectly between compile time and run-time, and requiring some part of the code to be compiled in "position-independant code" mode.
The bug is also tied to the C library you use with GCC and may be fixed depending on versions of this library, because dlopen() is not an OS API but an implementation of the POSIX interface inherited from old versions of SunOS. The implementation of this library is in charge to perform lookups of paths, and for this it may create local strings not allocated on the heap but possibly using alloca() on the stack, assuming that these buffers would not be overly large to fit with the dlopen() parameter or with the environment strings listing the search path.
Anyway dlopen() has mostly been tested with common library names that are short such as "libm.so".
And using random library names with dlopen() can be a security risk. library names specified in user scripts should then have a minimal validation.
Note that it is not possible to use the pointer directly as the implementation needs to derive full path names from it for the search...


Le ven. 4 sept. 2020 à 21:57, Roberto Ierusalimschy <[hidden email]> a écrit :
> > Stack overflow in lsys_load (lua/loadlib.c:134)
> >
> > [...]

Python 3 has the same issue (which reinforces the blame on dlopen):

$ python3
Python 3.6.9 (default, Jul 17 2020, 12:50:27)
[GCC 8.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from ctypes import *
>>> x = 'a' * 10000000
>>> cdll.LoadLibrary(x)
Segmentation fault (core dumped)

-- Roberto
Reply | Threaded
Open this post in threaded view
|

Re: Stack overflow in lsys_load (lua/loadlib.c:134)

Viacheslav Usov
In reply to this post by Roberto Ierusalimschy
On Fri, Sep 4, 2020 at 8:47 PM Roberto Ierusalimschy
<[hidden email]> wrote:

> I know Linux and other systems have limits on actual file-name lenghts
> (e.g., PATH_MAX), but dlopen must accept non-existent file names, too.

Quite frankly, I find this very similar to a bug I recently reported,
wherein Lua originally permitted user code to pass arbitrary mode
strings to popen(), which, per POSIX, might lead to undefined
behavior. Your decision then was to reject any mode strings that were
not POSIX conformant by default, while giving Lua users a compile-time
option to override that behavior.

> I did not find any documentation saying that invalid file names
> (filename arguments, in particular) should respect that limit, too.

The file argument to dlopen(), per POSIX, is a "pathname" [1]. A
pathname is defined to contain at most PATH_MAX bytes, including the
zero terminator. A longer string would not be POSIX conformant. Your
decision now?

Cheers,
V.

[1] Unfortunately, it is more complicated than that. It is a pathname
only when it contains a slash, and when it does not, the whole thing
is implementation defined. I am not sure if Lua permits the user code
to require a library so that it leads to a slash-less file argument
passed to dlopen(), but I'd think at least by default it should not be
permitted when Lua is compiled in the POSIX mode.
Reply | Threaded
Open this post in threaded view
|

Re: Stack overflow in lsys_load (lua/loadlib.c:134)

Roberto Ierusalimschy
In reply to this post by Roberto Ierusalimschy
> > > Stack overflow in lsys_load (lua/loadlib.c:134)
> > >
> > > [...]
>
> Python 3 has the same issue (which reinforces the blame on dlopen):
>
> $ python3
> Python 3.6.9 (default, Jul 17 2020, 12:50:27)
> [GCC 8.4.0] on linux
> Type "help", "copyright", "credits" or "license" for more information.
> >>> from ctypes import *
> >>> x = 'a' * 10000000
> >>> cdll.LoadLibrary(x)
> Segmentation fault (core dumped)

Perl also has the same issue:

  $ perl -de1
  Loading DB routines from perl5db.pl version 1.51
  [...]
  DB<1> require DynaLoader;
  DB<2> print(&DynaLoader::dl_load_file("a" x 100000000, 1));
  Segmentation fault (core dumped)

Again, the segfault happens inside open_path, which uses 'alloca' to
get a temporary buffer to manipulate the 'file' parameter.

Note that, in all cases, the given parameter is not intended to be a
pathname, because it does not contain a slash.

-- Roberto
Reply | Threaded
Open this post in threaded view
|

Re: Stack overflow in lsys_load (lua/loadlib.c:134)

Viacheslav Usov
On Tue, Sep 8, 2020 at 12:24 AM Roberto Ierusalimschy
<[hidden email]> wrote:

> Note that, in all cases, the given parameter is not intended to be a
> pathname, because it does not contain a slash.

I am not sure what that was supposed to demonstrate. Yes, it is not a
pathname, which makes this subject to an implementation-defined limit,
and all of Lua, Perl and Python let their users exceed the limit and
crash the process.

Since this is currently not listed as a bug in Lua, this is apparently
believed to be correct behavior. Why then was it a completely
different story with io.popen?

Cheers,
V.
Reply | Threaded
Open this post in threaded view
|

Re: Stack overflow in lsys_load (lua/loadlib.c:134)

Roberto Ierusalimschy
> I am not sure what that was supposed to demonstrate. Yes, it is not a
> pathname, which makes this subject to an implementation-defined limit,
> and all of Lua, Perl and Python let their users exceed the limit and
> crash the process.
>
> Since this is currently not listed as a bug in Lua, this is apparently
> believed to be correct behavior. Why then was it a completely
> different story with io.popen?

1) The issue with io.open is a well documented restriction, while
the issue with loadlib seems to be a bug in libc/dlopen.

2) An invalid mode for popen seems like a realistic mistake; a name
with 10e7 characters to loadlib seems possible only with malice.

3) loadlib is inherently low-level and unsafe, so this issue is just one
more way to use that function to crash the program when malice is in the
equation:

  > package.loadlib("/lib/x86_64-linux-gnu/libc.so.6", "memcpy")()
  Segmentation fault (core dumped)

-- Roberto
Reply | Threaded
Open this post in threaded view
|

Re: Stack overflow in lsys_load (lua/loadlib.c:134)

Viacheslav Usov
On Mon, Sep 14, 2020 at 8:58 PM Roberto Ierusalimschy
<[hidden email]> wrote:

> 3) loadlib is inherently low-level and unsafe

It is certainly true that this function can be used to cause a lot of
harm even without the dlopen() issue. So trying to address this issue
in the context of only loadlib() does not make much sense.

On the other hand, require() seems to be immune to this, which I think
is due to the use of fopen() before it even tries to call dlopen().
Interestingly, fopen() seems to be able to deal with very long path
names on both Linux and Windows, even though, per the standard, it
should only be called with filenames not longer than FILENAME_MAX,
which is a relatively small constant on these systems. The C standard
uses peculiarly ambiguous language when talking about FILENAME_MAX, so
it is debatable whether undefined behavior could ever ensue.

So considering all this, I agree with you that this needs no fixing
except perhaps the following.

The word "unsafe" is not explicitly stated in the documentation on
loadlib(). And since it applies to a Lua-level API, the dangers may
not be so obvious to the reader. Especially to an inexperienced
reader.

I believe it would be beneficial to mark up "unsafe" Lua-level API
explicitly in the manual, and add a section with a shorter version of
the last paragraph of the "Sandboxing" section in PiL4 with a
reference to the "unsafe" markup. The intent is to guide the users
willing to embed Lua, but who are yet new to Lua.

Cheers,
V.
Reply | Threaded
Open this post in threaded view
|

Re: Stack overflow in lsys_load (lua/loadlib.c:134)

Roberto Ierusalimschy
> Interestingly, fopen() seems to be able to deal with very long path
> names on both Linux and Windows, even though, per the standard, it
> should only be called with filenames not longer than FILENAME_MAX,
> which is a relatively small constant on these systems. The C standard
> uses peculiarly ambiguous language when talking about FILENAME_MAX, so
> it is debatable whether undefined behavior could ever ensue.

The C standard is not clear, but Posix explicitly lists ENAMETOOLONG
as a possible error for fopen:

    https://pubs.opengroup.org/onlinepubs/9699919799/functions/fopen.html

    "[ENAMETOOLONG] The length of a pathname exceeds {PATH_MAX}, or
    pathname resolution of a symbolic link produced an intermediate
    result with a length that exceeds PATH_MAX."

-- Roberto