-
Notifications
You must be signed in to change notification settings - Fork 677
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
[NEW] Disable LUA integration during build #1204
Comments
@valkey-io/core-team Thoughts? |
You could just use ACLs with |
Like I already mentioned, it could be avoided with ACL rules but the vulnerability is still part of the binary and the administrator has to be careful about the default permission of each new user. I think it would be much nicer security default if they don't build LUA into the binaries. |
@dmitrypol You mentioned once that you had some thoughts around building without LUA. I guess my concern is just that it sort of bifurcates the entire experience. Some people won't have lua scripts and they'll be annoyed. There are a lot of time we say "no" to a feature because you can implement it with lua. |
This is what I also want to understand more about. Do we add friction to the dev experience by shoo-ing them away by saying "Use LUA". LUA usage is also a double edged sword, some users abuse it to an extent that you don't know if it was actually intended the way it is. It also becomes an operational nightmare with long running scripts. |
Would be nice if folks from the community can add why/how they use LUA and if they find some API missing in Valkey forcing them to build LUA scripts. |
|
Agree with @PingXie that we can do add some Usually we don't want compile-time configuration, like ifdefs for various features, but this is an optional dependency. It's different. |
The CVEs I can find are these two, listed on https://www.cvedetails.com/version/1198481/LUA-LUA-5.1.html
In our
If we have patches for all the CVEs, we should definitely document it better, so users don't feel they need to disable Lua. We never updated Lua because we don't want to break any scripts. I wonder how big that risk is. Considering Lua 5.1 is end-of-life since 2012, maybe we should reconsider this decision at some point? If we do, then I believe LuaJIT is the better option compared to Lua 5.4. |
Recent one around LUA: There were lot more which antirez fixed in the past http://antirez.com/news/119 (when the project wasn't focused around security aspect)
I know it will cause fragmentation but I think we should consider reducing the attack vector(s) possible on Valkey and for easy migration we would need to provide concrete alternatives.
This is another tangent we can think about. |
I have some experience on Lua script issue (It is really annoyed), but I have no too much strong point to disable it during build:
|
My preference is we leave this issue open and see if anyone actually wants to build Valkey without Lua. AWS won't want to build without Lua. I find it unlikely the other managed providers will want to build without Lua. I don't want to add the flag without a flag end-user for it. |
Yes, I think that's reasonable. Let's hear out the community and understand better. 👍 |
The comments here dovetail with #1229, but I need to put it here as foundation for my vote for disabling Lua integration. For a while, I managed my own fork of Redis with LuaJIT, modifying the build process. Even though LuaJIT is significantly faster than Lua, Redis Lua Scripting uses the Lua C API which is inherently slow (albeit multiples faster with LuaJIT's implementation). All the Redis arguments are marshalled onto a stack, the That work was from 2013 (!)... once Modules were introduced (2016!), I was excited to experiment with access to it via modules with LuaJIT FFI. The Module API way was such a natural way to interact with the Redis core and the LuaJIT FFI is very efficient. As I noted in one of Redis' Lua issues, such a module with dynamically linked LuaJIT conflicts with the statically linked Lua. But I did experiment with this on another LuaJIT-forked Redis. I never used it in production, but I found it ironic that other languages can use the Module API but Lua could not. If Lua were disabled at build-level, such a LuaJIT Module could load its own LuaJIT (or the system LuaJIT, etc). Or get a LuaJIT Beyond all that, removing Lua is a simple hardening technique for the vast amount cases that don't use Lua at all. Thanks for all the stewardship with ValKey. |
@neomantra Thanks for providing the use cases for this. Running Lua FFI and module API from EVAL is a big security issue in setups where not all clients can be guaranteed to be inside a trusted environement. I understand you run your clients inside a local or at least trusted network, which is very valid. The official Lua support is sandboxed (at least it's supposed to be), because clients can not always be trusted, while the module API is not. Manage database service providers obviously don't allow the customers to use their own modules, but not all setups are like that. IMO, we should not disable Lua by default, but only provide a way to compile without it, so users who want this don't need a patched Valkey version to do that. I would accept a PR that adds a make variable for this, such as |
|
Thanks @neomantra for explaining your setup and use case. 👍 |
Adds the ability to disable Lua scripting using the build conifguration `USE_LUA`. This will prevent the building of the Lua static library and will keep Lua and most Lua scripting internals out of the build. This approach strived to minimize the change surface, stubbing out keys requried functions and otherwise `ifdef`ing out large swathes of code. The base Lua scripting commands like `EVAL` remain intact, but reply with an error message `Lua scripting disabled`. `INFO` commands still include scripting statistics to prevent breaking any DevOps scripts, etc.
Adds the ability to disable Lua scripting using the build conifguration `USE_LUA`. This will prevent the building of the Lua static library and will keep Lua and most Lua scripting internals out of the build. This approach strived to minimize the change surface, stubbing out keys requried functions and otherwise `ifdef`ing out large swathes of code. The base Lua scripting commands like `EVAL` remain intact, but reply with an error message `Lua scripting disabled`. `INFO` commands still include scripting statistics to prevent breaking any DevOps scripts, etc. Signed-off-by: Evan Wies <[email protected]>
@neomantra Could you submit a PR against valkey? |
I did earlier today... working on test coverage now. |
Adds the ability to disable Lua scripting using the build conifguration `USE_LUA`. This will prevent the building of the Lua static library and will keep Lua and most Lua scripting internals out of the build. This approach strived to minimize the change surface, stubbing out keys requried functions and otherwise `ifdef`ing out large swathes of code. The base Lua scripting commands like `EVAL` remain intact, but reply with an error message `Lua scripting disabled`. `INFO` commands still include scripting statistics to prevent breaking any DevOps scripts, etc. Signed-off-by: Evan Wies <[email protected]>
Adds the ability to disable Lua scripting using the build conifguration `USE_LUA`. This will prevent the building of the Lua static library and will keep Lua and most Lua scripting internals out of the build. This approach strived to minimize the change surface, stubbing out keys requried functions and otherwise `ifdef`ing out large swathes of code. The base Lua scripting commands like `EVAL` remain intact, but reply with an error message `Lua scripting disabled`. `INFO` commands still include scripting statistics to prevent breaking any DevOps scripts, etc. Signed-off-by: Evan Wies <[email protected]>
With this Lua-less Valkey, I was able to update my previous Module work -- migrated it to valkey-mod_luajit -- and was able to build a module linked with LuaJIT and load it into Valkey. It's just a POC and very rough, but at least it's possible to play now.
|
Based on my observation, there are multiple CVEs occurring due to bugs/issues found in Lua engine/debugger. If a user doesn't have any use case of using Lua scripts and if they can remove it from the Valkey binary, they don't need to address the patching of security vulnerabilities in their system.
I found a similar request on Stack Overflow. Currently, a user could disable LUA usage via ACL rules by restricting EVAL/SCRIPT but I think they would still need to patch their servers within a certain period of time.
Could we make LUA optional ? This could also help us building right set of API(s) for users who rely on LUA to address functionalities not supported by server like CAS.
CVE(s): https://docs.aws.amazon.com/AmazonElastiCache/latest/dg/cve.html https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=Lua+Redis
The text was updated successfully, but these errors were encountered: