Bug 34349 - Validate inputs for task scheduler
Summary: Validate inputs for task scheduler
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P1 - high critical (vote)
Assignee: David Cook
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 35009
  Show dependency treegraph
 
Reported: 2023-07-24 03:47 UTC by David Cook
Modified: 2024-01-10 07:59 UTC (History)
14 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:
23.11.00,23.05.04,22.11.10,22.05.16,21.11.24


Attachments
Bug 34349: WIP (2.47 KB, patch)
2023-07-24 04:31 UTC, David Cook
Details | Diff | Splinter Review
Bug 34349: Validate input for scheduler (7.89 KB, patch)
2023-07-27 10:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 34349: Validate/escape inputs for task scheduler (3.17 KB, patch)
2023-08-01 00:43 UTC, David Cook
Details | Diff | Splinter Review
Bug 34349: Validate/escape inputs for task scheduler (3.27 KB, patch)
2023-08-03 08:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34349: Validate/escape inputs for task scheduler (3.53 KB, patch)
2023-08-04 06:30 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 2023-07-24 03:47:37 UTC
As per bug 34348, we need to validate the inputs to the Task Scheduler.
Comment 1 David Cook 2023-07-24 04:31:39 UTC
Created attachment 153822 [details] [review]
Bug 34349: WIP
Comment 2 David Cook 2023-07-24 04:32:04 UTC
Actually, that's not going to be enough, since a RFC2822 valid email address can still be a malicious payload.

Even if we tried to scrub out metacharacters, there's a lot of ways you can hack a string to do malicious things on the CLI.

That means we either need to remove scheduler.pl or change its mechanism.

We could create a Background::Job, or we could add a new database table that keeps the string at arms length. 

Open to other ideas too.
Comment 3 David Cook 2023-07-24 04:37:53 UTC
I suppose another idea would be to select a borrower and pass the borrowernumber to runreport.pl instead of an email address, but that probably wouldn't make many people happy.

Perhaps it is time to retire scheduler.pl and come up with a new solution.
Comment 4 Liz Rea 2023-07-26 14:47:00 UTC
I like the notion of making a scheduler that runs a cron every minute to check for a scheduled job, that was inserted in the background_jobs table under a type of "cron." 

story: 
user schedules a job (say, a report)
Job is inserted into background jobs
Koha has a cron that runs every minute to check for these jobs - if nothing, just quit, if something, run it as specified. 

Added bonus that things could be scheduled to be recurring
Comment 5 David Cook 2023-07-27 01:09:59 UTC
(In reply to Liz Rea from comment #4)
> I like the notion of making a scheduler that runs a cron every minute to
> check for a scheduled job, that was inserted in the background_jobs table
> under a type of "cron." 
> 
> story: 
> user schedules a job (say, a report)
> Job is inserted into background jobs
> Koha has a cron that runs every minute to check for these jobs - if nothing,
> just quit, if something, run it as specified. 
> 
> Added bonus that things could be scheduled to be recurring

Sounds good to me. I think we're continuing the discussion on bug 1993.
Comment 6 Jonathan Druart 2023-07-27 07:37:25 UTC
Still, I think we could test if the email is in the DB and looks like a valid email (and eventually reject email with backticks, even if it's valid).

Wouldn't that be enough?
Comment 7 Marcel de Rooy 2023-07-27 08:08:29 UTC
(In reply to Jonathan Druart from comment #6)
> Still, I think we could test if the email is in the DB and looks like a
> valid email (and eventually reject email with backticks, even if it's valid).
> 
> Wouldn't that be enough?

No. We should indeed reject backticks too.
But also check the other inputs. As said before, you can also do it via a GET with format.
Will be submitting something here.
Comment 8 Jonathan Druart 2023-07-27 08:17:53 UTC
Yes but this patch already deals with format.

Waiting for your patch.
Comment 9 Marcel de Rooy 2023-07-27 09:58:24 UTC
(In reply to Jonathan Druart from comment #8)
> Yes but this patch already deals with format.
> 
> Waiting for your patch.

I did not see this patch until David pointed to it on the othre report.
I remove the WIP one therefore.
This is a more generic and I will polish it on follow-up.
The WIP patch does only check for job_add. I check always here and fail for whatever reason we encounter.
Comment 10 Marcel de Rooy 2023-07-27 10:02:49 UTC
Created attachment 153955 [details] [review]
Bug 34349: Validate input for scheduler

Test plan:
Try various wrong inputs via URL or form. See other report for
hacking format with backtics.
Run t/Koha/Validator.t
Comment 11 Marcel de Rooy 2023-07-27 11:25:39 UTC
(In reply to Marcel de Rooy from comment #9)
> I remove the WIP one therefore.

Forgot to do that. Done now.
Comment 12 Jonathan Druart 2023-07-27 11:43:59 UTC
That's too overthought in my opinion. We need something simple/kiss here.

You cannot add a Koha::Validator package without real plans for it.
Comment 13 Jonathan Druart 2023-07-27 11:46:33 UTC
$report: looks_like_number
$format: (text html csv tsv)
$email: Koha::Email->is_valid and reject if has backticks

That's 3 lines.
Comment 14 Marcel de Rooy 2023-07-27 11:57:36 UTC
(In reply to Jonathan Druart from comment #12)
> That's too overthought in my opinion. We need something simple/kiss here.
Look at the code. It is simple.
Allowing flexibility with default rules, value lists, regex and subroutine within just a few lines.

> You cannot add a Koha::Validator package without real plans for it.
This could be a pattern to validate CGI params in other places too.
But is a bit more generic to allow for validating other sets too.
Comment 15 Marcel de Rooy 2023-07-27 11:58:22 UTC
(In reply to Jonathan Druart from comment #13)
> $report: looks_like_number
> $format: (text html csv tsv)
> $email: Koha::Email->is_valid and reject if has backticks
> 
> That's 3 lines.

These 3 lines are converted to three rule lines.
And one call ->check.
So 4 lines.
But more consistent.
Comment 16 Jonathan Druart 2023-07-27 12:07:51 UTC
That's the thing, this bug is about fixing a security bug that will need to be backported. Not a place to introduce a new module + test + make thing complex and start a discussion about how to implement it :D

I will let somebody else give their opinion.
Comment 17 Marcel de Rooy 2023-07-27 12:11:58 UTC
(In reply to Jonathan Druart from comment #16)
> That's the thing, this bug is about fixing a security bug that will need to
> be backported. Not a place to introduce a new module + test + make thing
> complex and start a discussion about how to implement it :D

This can be simply backported because moving most code to a new module. No rebasing problems there.
New test should be no problem.
I did not start this discussion btw..

But let someone else have a look too :)
Comment 18 Marcel de Rooy 2023-07-27 13:35:59 UTC
As a side note. It is possible to save email with backtics via REST API.

And you can trigger nice error in process_message_queue:

Argument contains empty host portion of address at /usr/share/perl5/Email/MIME/Encode.pm line 71.
no recipients
Trace begun at /usr/share/perl5/Email/Sender/Simple.pm line 116
Email::Sender::Simple::send_email('Email::Sender::Simple', 'Email::Abstract=ARRAY(0x56120ea826a8)', 'HASH(0x56120ea82bb8)') called at /usr/share/perl5/Email/Sender/Role/CommonSending.pm line 45
Email::Sender::Role::CommonSending::try {...}  at /usr/share/perl5/Try/Tiny.pm line 102
eval {...} at /usr/share/perl5/Try/Tiny.pm line 93
Try::Tiny::try('CODE(0x56120f56b1b8)', 'Try::Tiny::Catch=REF(0x56120f052910)') called at /usr/share/perl5/Email/Sender/Role/CommonSending.pm line 58
Email::Sender::Role::CommonSending::send('Email::Sender::Simple', 'Email::MIME=HASH(0x56120effa6d8)', 'HASH(0x56120ea82a08)') called at /usr/share/perl5/Email/Stuffer.pm line 757
Email::Stuffer::send_or_die('Koha::Email=HASH(0x56120effdf28)', 'HASH(0x56120f52ed00)') called at /usr/share/koha/Koha/Email.pm line 233
Koha::Email::send_or_die('Koha::Email=HASH(0x56120effdf28)', 'HASH(0x56120f52ed00)') called at /usr/share/koha/C4/Letters.pm line 1476
C4::Letters::try {...}  at /usr/share/perl5/Try/Tiny.pm line 100
eval {...} at /usr/share/perl5/Try/Tiny.pm line 93
Try::Tiny::try('CODE(0x56120f00f760)', 'Try::Tiny::Catch=REF(0x56120f0527f0)') called at /usr/share/koha/C4/Letters.pm line 1498
C4::Letters::_send_message_by_email('HASH(0x56120efe19d0)', undef, undef, 'LOGIN', 'HASH(0x56120eee68b0)') called at /usr/share/koha/C4/Letters.pm line 1014
C4::Letters::SendQueuedMessages('HASH(0x56120624e520)') called at misc/cronjobs/process_message_queue.pl line 115
 at /usr/share/koha/C4/Letters.pm line 1498.
Use of uninitialized value $Mail::Sendmail::error in string at /usr/share/koha/C4/Letters.pm line 1496.
 at /usr/share/koha/C4/Letters.pm line 1498.
Comment 19 Marcel de Rooy 2023-07-27 13:39:12 UTC
Wont be around until Tuesday. If anyone else wants to have a go, just remove the patch and do it yourself again :)
Comment 20 David Cook 2023-08-01 00:28:52 UTC
(In reply to Jonathan Druart from comment #12)
> That's too overthought in my opinion. We need something simple/kiss here.
> 
> You cannot add a Koha::Validator package without real plans for it.

Agreed. I don't know why Marcel killed my original patch, since it properly validated the "report" and "format" parameters. 

(In reply to Jonathan Druart from comment #13)
> $report: looks_like_number
> $format: (text html csv tsv)
> $email: Koha::Email->is_valid and reject if has backticks
> 
> That's 3 lines.

After doing some digging, I've got another thought. 

One of the problems with rejecting backticks is that they're valid characters in an email address. Also, it's always possible to inject other shell metacharacters, which are harder to abuse, but still doable.

But if we wrap the email address in single quotes and then escape single quotes (which are remarkably common in email addresses in the English world), then we should be OK. In fact, we wouldn't even need to reject backticks, because they wouldn't be interpolated.
Comment 21 David Cook 2023-08-01 00:43:49 UTC
Created attachment 154120 [details] [review]
Bug 34349: Validate/escape inputs for task scheduler

This change validates and escapes inputs for task scheduler.

Test plan:
0. Apply patch
1. koha-plack --reload kohadev
2. Go to http://localhost:8081/cgi-bin/koha/tools/scheduler.pl
3. Input a time a minute in the future and leave the date blank
4. Choose an existing report and output format
5. Type a malicious string which is also a valid email address
into the Email field
6. Click "Save"
7. Note that the job is added but the Email is wrapped in single
quotes
8. Try using a non-malicious email address with a single quote.
9. Note that the single quote is escaped, so that it will still
be used by runreport.pl
Comment 23 Jonathan Druart 2023-08-03 08:13:44 UTC
Created attachment 154166 [details] [review]
Bug 34349: Validate/escape inputs for task scheduler

This change validates and escapes inputs for task scheduler.

Test plan:
0. Apply patch
1. koha-plack --reload kohadev
2. Go to http://localhost:8081/cgi-bin/koha/tools/scheduler.pl
3. Input a time a minute in the future and leave the date blank
4. Choose an existing report and output format
5. Type a malicious string which is also a valid email address
into the Email field
6. Click "Save"
7. Note that the job is added but the Email is wrapped in single
quotes
8. Try using a non-malicious email address with a single quote.
9. Note that the single quote is escaped, so that it will still
be used by runreport.pl

JD amended patch: tidy

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 24 Jonathan Druart 2023-08-03 08:16:57 UTC
($email) =~ s/'/'"'"'/g;

Why the parenthesis?
Why this substitution and not simply escaping with backslash?
Comment 25 Marcel de Rooy 2023-08-03 08:47:39 UTC
(In reply to Jonathan Druart from comment #24)
> ($email) =~ s/'/'"'"'/g;
> 
> Why the parenthesis?
> Why this substitution and not simply escaping with backslash?

Yes, you dont need the parentheses.

And note:
Enclosing characters in single quotes (‘'’) preserves the literal value of each character within the quotes. A single quote may not occur between single quotes, even when preceded by a backslash.
Comment 26 David Cook 2023-08-04 01:11:34 UTC
(In reply to Marcel de Rooy from comment #25)
> (In reply to Jonathan Druart from comment #24)
> > ($email) =~ s/'/'"'"'/g;
> > 
> > Why the parenthesis?
> > Why this substitution and not simply escaping with backslash?
> 
> Yes, you dont need the parentheses.

Typo. Happy for them to be removed or to submit a new one with them removed.

> And note:
> Enclosing characters in single quotes (‘'’) preserves the literal value of
> each character within the quotes. A single quote may not occur between
> single quotes, even when preceded by a backslash.

Yeah escaping with a backslash doesn't work. This is the only escape that I found that works.
Comment 27 Marcel de Rooy 2023-08-04 06:30:23 UTC
Created attachment 154235 [details] [review]
Bug 34349: Validate/escape inputs for task scheduler

This change validates and escapes inputs for task scheduler.

Test plan:
0. Apply patch
1. koha-plack --reload kohadev
2. Go to http://localhost:8081/cgi-bin/koha/tools/scheduler.pl
3. Input a time a minute in the future and leave the date blank
4. Choose an existing report and output format
5. Type a malicious string which is also a valid email address
into the Email field
6. Click "Save"
7. Note that the job is added but the Email is wrapped in single
quotes
8. Try using a non-malicious email address with a single quote.
9. Note that the single quote is escaped, so that it will still
be used by runreport.pl

JD amended patch: tidy

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Removed pars for $email =~ regex, removed old commented lines.
Comment 28 David Cook 2023-08-30 01:14:16 UTC
This should be pushed ASAP since it's a remote code execution vulnerability.
Comment 29 Tomás Cohen Arazi 2023-09-25 22:09:19 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 30 Matt Blenkinsop 2023-10-17 16:31:04 UTC
This has been pushed to 23.05.x and 22.11.x. For some reason the bug wasn't updated so I'm following up now
Comment 31 David Cook 2023-11-03 00:06:04 UTC
Looks like we still need this one for 22.05 and 21.11? 

Or will 21.11 drop off the supported list in a few weeks?
Comment 32 Fridolin Somers 2023-12-14 00:13:55 UTC
I see this commit in branches from 23.11.x to 21.11.x