diego-treitos / nginx_cache_multipurge

31 stars 12 forks source link

remove lua-md5 dependency #11

Closed peixotorms closed 2 years ago

peixotorms commented 2 years ago

remove lua-md5 dependency

diego-treitos commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.