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

remove lua-md5 dependency #11

Closed
wants to merge 1 commit into from
Closed

remove lua-md5 dependency #11

wants to merge 1 commit into from

Conversation

peixotorms
Copy link

remove lua-md5 dependency

remove lua-md5 dependency
@peixotorms peixotorms mentioned this pull request May 4, 2022
@diego-treitos
Copy link
Owner

The variable cmp_cache_key is partially controlled by the user. For this reason your code is at a high risk of being vulnerable to an arbitrary command injection. For this reason, and also performance, I don't like to abuse on the use of shell calls.

@peixotorms peixotorms deleted the patch-1 branch May 5, 2022 12:35
@peixotorms
Copy link
Author

peixotorms commented May 5, 2022

I have removed the pull request, however, even if the cmp_cache_key is partially controlled by the user, I don't see how it can be a vulnerability if whatever is sent, is being md5 encoded. Even if the key is $request_uri I am not sure about this or maybe I am not knowledgeable enough, sorry. Perhaps you have some example of how that could happen?

Also, doing an md5sum via shell or lua module has probably the same performance.
I mean, you are already using the shell for the find command... so why not just pipe the result from md5sum directly, instead of relying on another module, that doesn't work on ubuntu 20.0.4 for example.

If you have an alternative for #10 md5-lua module to work on openresty on ubuntu 20, I would be happy to use it instead of this, but if not, I couldn't find any other way to make it work.

Thanks

@diego-treitos
Copy link
Owner

I am not an expert either but in my understanding, because you used double quotes (") in the echo call, an attacker could execute code if they manage to inject back ticks or dollar and parenthesis ..`<command>`.. or ..$(<command>)... This would not happen if you would used single quaotes (') in the echo -n. However, even in this case, the attacker could manage to inject something like ..'; <command> ; echo '...

I am not 100% sure if the attacker could inject this, mainly because cmp_cache_key could be url encoded, but even so, I consider it a good practice to not do such things, even if now it is not vulnerable (it could be in the future) or if I am not able to see how they can do it (I might be wrong). If I can avoid it, I try to prevent to use user input directly in shell calls.

Regarding the performance side, I guess the performance impact is not great if you do not make a lot of calls, but things like awk are specially expensive.

All in all, I think the best choice is to use a different md5 module for lua.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants