|
Description
David Cook
2023-07-24 03:47:37 UTC
Created attachment 153822 [details] [review] Bug 34349: WIP 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. 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. 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 (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. 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? (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. Yes but this patch already deals with format. Waiting for your patch. (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. 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 (In reply to Marcel de Rooy from comment #9) > I remove the WIP one therefore. Forgot to do that. Done now. 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. $report: looks_like_number $format: (text html csv tsv) $email: Koha::Email->is_valid and reject if has backticks That's 3 lines. (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. (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. 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. (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 :) 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.
Wont be around until Tuesday. If anyone else wants to have a go, just remove the patch and do it yourself again :) (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. 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 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> ($email) =~ s/'/'"'"'/g; Why the parenthesis? Why this substitution and not simply escaping with backslash? (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. (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. 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. This should be pushed ASAP since it's a remote code execution vulnerability. Pushed to master for 23.11. Nice work everyone, thanks! 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 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? I see this commit in branches from 23.11.x to 21.11.x |