Bug 37488 - Filepaths not validated in ZIP upload to picture-upload.pl
Summary: Filepaths not validated in ZIP upload to picture-upload.pl
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: David Cook
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 37323
Blocks:
  Show dependency treegraph
 
Reported: 2024-07-26 03:44 UTC by David Cook
Modified: 2024-09-18 03:46 UTC (History)
17 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00,24.05.03,23.11.08,23.05.14,22.11.20
Circulation function:


Attachments
Bug 37488: Validate paths in datalink.txt/idlink.txt files (1.56 KB, patch)
2024-07-26 04:04 UTC, David Cook
Details | Diff | Splinter Review
Bug 37488: Validate paths in datalink.txt/idlink.txt files (1.61 KB, patch)
2024-07-26 13:22 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Image file that can be used for testing (357.05 KB, application/zip)
2024-07-26 13:24 UTC, Nick Clemens (kidclamp)
Details
Bug 37488: Validate paths in datalink.txt/idlink.txt files (1.71 KB, patch)
2024-08-02 09:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2024-07-26 03:44:44 UTC
As noted on bug 37323, in a datalink.txt file, you can craft a filepath to load any image on the system that is readable by that instance's Linux user.
Comment 2 David Cook 2024-07-26 04:04:25 UTC
Created attachment 169640 [details] [review]
Bug 37488: Validate paths in datalink.txt/idlink.txt files

This change validates the paths in datalink.txt/idlink.txt,
so that only images in the unpacked archive directory are allowed

Test plan:
0. Apply the patch
1. koha-plack --reload kohadev
2. Create a datalink.txt file with the following:
42,selfie.jpg
3. Create a jpeg at selfie.jpg
4. ZIP the datalink.txt and selfie.jpg files
5. Upload to the "Upload patron images" tool
(after enabling the "patronimages" system preference)
6. Note that the image uploads correctly
Comment 3 Nick Clemens (kidclamp) 2024-07-26 13:22:01 UTC
Created attachment 169704 [details] [review]
Bug 37488: Validate paths in datalink.txt/idlink.txt files

This change validates the paths in datalink.txt/idlink.txt,
so that only images in the unpacked archive directory are allowed

Test plan:
0. Apply the patch
1. koha-plack --reload kohadev
2. Create a datalink.txt file with the following:
42,selfie.jpg
3. Create a jpeg at selfie.jpg
4. ZIP the datalink.txt and selfie.jpg files
5. Upload to the "Upload patron images" tool
(after enabling the "patronimages" system preference)
6. Note that the image uploads correctly

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 4 Nick Clemens (kidclamp) 2024-07-26 13:24:53 UTC
Created attachment 169707 [details]
Image file that can be used for testing

Combination of helpfulness and vanity :-) Feel free to use my lovely selfie to ease testing
Comment 5 Marcel de Rooy 2024-08-02 07:56:53 UTC
Looking here
Comment 6 Marcel de Rooy 2024-08-02 09:04:51 UTC
Created attachment 169992 [details] [review]
Bug 37488: Validate paths in datalink.txt/idlink.txt files

This change validates the paths in datalink.txt/idlink.txt,
so that only images in the unpacked archive directory are allowed

Test plan:
0. Apply the patch
1. koha-plack --reload kohadev
2. Create a datalink.txt file with the following:
42,selfie.jpg
3. Create a jpeg at selfie.jpg
4. ZIP the datalink.txt and selfie.jpg files
5. Upload to the "Upload patron images" tool
(after enabling the "patronimages" system preference)
6. Note that the image uploads correctly

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2024-08-02 09:05:14 UTC
Isnt this patch only redirecting to another error message actually ?

Without this patch, I have something like:

[2024/08/02 08:58:38] [WARN] Opening /tmp/7PkHGkjawG//etc/selfie.jpg failed! at /usr/share/koha/tools/picture-upload.pl line 413.
(Line number may be affected by a few debugs.)

And with this patch:

[2024/08/02 08:59:49] [WARN] Missing filename at /usr/share/koha/tools/picture-upload.pl line 435.

Using a path /etc/selfie.jpg (nice picture, Nick ;)

Will put a QA stamp on it, just asking..

Note that missing filename is not really the best error message?
Comment 8 David Cook 2024-08-04 23:31:04 UTC
(In reply to Marcel de Rooy from comment #7)
> Without this patch, I have something like:
> 
> [2024/08/02 08:58:38] [WARN] Opening /tmp/7PkHGkjawG//etc/selfie.jpg failed!
> at /usr/share/koha/tools/picture-upload.pl line 413.
> (Line number may be affected by a few debugs.)

You haven't crafted your exploit quite right there. You'd need to preface "/etc/selfie.jpg" with "../.." and then it would work. 

> And with this patch:
> 
> [2024/08/02 08:59:49] [WARN] Missing filename at
> /usr/share/koha/tools/picture-upload.pl line 435.
> 
> Using a path /etc/selfie.jpg (nice picture, Nick ;)
> 
> Will put a QA stamp on it, just asking..
> 
> Note that missing filename is not really the best error message?

No, it's probably not the best error message, but I thought it was accurate enough, and kept the patch small and easy to backport.
Comment 9 Marcel de Rooy 2024-08-05 06:53:50 UTC
(In reply to David Cook from comment #8)
> (In reply to Marcel de Rooy from comment #7)
> > Without this patch, I have something like:
> > 
> > [2024/08/02 08:58:38] [WARN] Opening /tmp/7PkHGkjawG//etc/selfie.jpg failed!
> > at /usr/share/koha/tools/picture-upload.pl line 413.
> > (Line number may be affected by a few debugs.)
> 
> You haven't crafted your exploit quite right there. You'd need to preface
> "/etc/selfie.jpg" with "../.." and then it would work. 
> 
Tried several variations. Retried this one:

[2024/08/05 06:49:30] [WARN] Opening /tmp/ArJVJ8uzLT/../../etc/selfie.jpg failed! at /usr/share/koha/tools/picture-upload.pl line 416.

Debian 11 container.

There must be more to it..
Comment 10 David Cook 2024-08-05 23:03:25 UTC
(In reply to Marcel de Rooy from comment #9)
> Tried several variations. Retried this one:
> 
> [2024/08/05 06:49:30] [WARN] Opening /tmp/ArJVJ8uzLT/../../etc/selfie.jpg
> failed! at /usr/share/koha/tools/picture-upload.pl line 416.
> 
> Debian 11 container.
> 
> There must be more to it..

It looks like you're not using ktd. I'm guessing it's with your work Koha setup? That could complicate things. Have you checked the file permissions? What permissions does the Koha user run as?
Comment 11 Marcel de Rooy 2024-08-06 11:46:40 UTC
(In reply to David Cook from comment #10)
> (In reply to Marcel de Rooy from comment #9)
> > Tried several variations. Retried this one:
> > 
> > [2024/08/05 06:49:30] [WARN] Opening /tmp/ArJVJ8uzLT/../../etc/selfie.jpg
> > failed! at /usr/share/koha/tools/picture-upload.pl line 416.
> > 
> > Debian 11 container.
> > 
> > There must be more to it..
> 
> It looks like you're not using ktd. I'm guessing it's with your work Koha
> setup? That could complicate things. Have you checked the file permissions?
> What permissions does the Koha user run as?

No ktd. But the setup is fine, running the koha user for Apache and Plack.

I just realize that I was kind of misinterpreting your test plan and the contents of Nicks test zip file. We are not preventing to write to /tmp or /etc here! We are preventing to load from /tmp or /etc. So, all good.
Comment 12 Marcel de Rooy 2024-08-06 11:47:33 UTC
Katrin, could you get this further please? Thx
Comment 13 Katrin Fischer 2024-08-06 15:35:50 UTC
(In reply to Marcel de Rooy from comment #12)
> Katrin, could you get this further please? Thx

Please check my note on the Mattermost security channel.
Comment 14 Tomás Cohen Arazi (tcohen) 2024-08-12 14:37:09 UTC
Pushed to main for 24.11.

Nice work everyone, thanks!
Comment 15 Wainui Witika-Park 2024-09-18 03:46:26 UTC
Not backporting to 23.05.x unless requested