Bug 3280 - opac/opac-sendbasket.pl security leaky
Summary: opac/opac-sendbasket.pl security leaky
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: 3.10
Hardware: All All
: P3 normal
Assignee: Sébastien Marie
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 28024
  Show dependency treegraph
 
Reported: 2009-06-01 06:42 UTC by D Ruth Holloway
Modified: 2021-04-09 09:37 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 3280 Restrict Send-basket feature (3.27 KB, patch)
2011-12-05 09:21 UTC, Sébastien Marie
Details | Diff | Splinter Review
Bug 3280 Restrict Send basket feature (2.89 KB, patch)
2012-06-09 13:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 3280 Restrict Send basket feature (3.05 KB, patch)
2012-06-28 09:29 UTC, Sébastien Marie
Details | Diff | Splinter Review
Signed patch for 3280 (3.17 KB, patch)
2012-06-28 09:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Followup for privacy issue (896 bytes, patch)
2012-07-02 08:12 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 Chris Cormack 2010-05-21 01:08:38 UTC


---- Reported by dbavousett@ptfs.com 2009-06-01 18:42:11 ----

It is possible for a clever user who has access to the source to spot that opac-sendbasket.pl has a security leak, and use a Koha server as a spam relay.

opac-sendbasket.pl uses CGI's param() method to fetch in parameters.  This functions fine, but it doesn't *care* if the parameters come in via GET or POST methods.  Thus, you can:

http://server/cgi-bin/koha/opac-sendbasket. pl?&email_add=user@domain.com&comment=”blahblahblah”

This will send a message with the cart header as defined elsewhere, plus the comments from the "comment" param, to user@domain.com.  A moderately-clever geek could easily work out a script to use this as a spamming tool.

My suggestion would be to either use CGI_Lite, which allows for control over passing method, or manually fetching in the parameters "the hard way", which would allow you to explicitly disallow GET params in (at a minimum) email_add and comment.



---- Additional Comments From dbavousett@ptfs.com 2009-06-01 18:46:22 ----

Another part of this, I suppose, is that it doesn't seem check for an empty cart (variable bib_list?).  Doing that would also tighten up the security of this script, I suspect, and would make it a bit more user-friendly.



---- Additional Comments From chris@bigballofwax.co.nz 2009-06-01 19:33:03 ----

Yeah, disallowing GET, would mean they would SPAM through POST instead. Spammers are nothing if not persistent.
Checking for a non empty cart seems like a good way to go.



---- Additional Comments From joe.atzberger@liblime.com 2009-06-01 20:31:53 ----

Might be good to insert, "this request came from IP: $ENV{REMOTE_ADDR}".  Doesn't prevent the problem, but may help ID your culprit if spamming occurs.

Other possibilities:
 ~ require user to be logged in to send cart, then send only to (one of) their registered addresses, using message queue (true prevention, with part of the feature sacrificed)
 ~ build internal tracking of messages sent with thresholds considered "abusive" (damage limitation)
 ~ take the address in a separate request from the "send" confirmation, either retaining it on the server side or passing it back and forth in an encrypted form (problem obfuscation -- not truly more secure, but more difficult to exploit)



--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:08 UTC  ---

This bug was previously known as _bug_ 3280 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=3280

Unknown operating system Linux - Debian. Setting to default OS "All".
Actual time not defined. Setting to 0.0
CC member dhall@ebpl.org does not have an account here
CC member joe.atzberger@liblime.com does not have an account here
CC member jwagner@ptfs.com does not have an account here

Comment 1 Ian Walls 2011-12-04 18:01:52 UTC
Confirmed this is still an issue.

Not sure there is any good way around this.  To truly solve the issue, we'd need to be able to separate the spam from the ham (as it were), which is too subjective to handle systematically.

Perhaps instead of allowing arbitrary email addresses, we only allow either other borrowernumbers (so you can send to your friends) or a selection of one of the patron's own email addresses.  The latter would be much easier than the former.  This would reduce the flexibility of opac-sendbasket, but I think that flexibility is the fundamental security issue here.
Comment 2 Sébastien Marie 2011-12-05 09:21:46 UTC Comment hidden (obsolete)
Comment 3 Sébastien Marie 2011-12-05 09:25:37 UTC
Does opac/opac-sendshelf.pl should be have a separated bug report ?

As there have same features (send email to someone), corrections (or enhancements) should be consistent.
Comment 4 Kyle M Hall 2012-06-09 13:36:25 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2012-06-28 07:51:21 UTC
Kyle,
Your patch now removes the feature of an anonymous user putting books into the cart and mailing them. Would it not be enough to check if the cart is not empty instead of forcing login now? 
Should we discuss restricting all email functionality in Koha to authenticated users? Or add yet another pref :-)
Comment 6 Katrin Fischer 2012-06-28 07:56:31 UTC
Hi Marcel, not sure that was Kyle - there is bug 4274 for the problem. It used to be possible for not logged in users to mail the cart (I know it was in 3.2.x). Then it got changed some time after that. Now it requires you to log in before you can send items from the cart.
Comment 7 Marcel de Rooy 2012-06-28 08:06:19 UTC
(In reply to comment #6)
> Hi Marcel, not sure that was Kyle - there is bug 4274 for the problem. It
> used to be possible for not logged in users to mail the cart (I know it was
> in 3.2.x). Then it got changed some time after that. Now it requires you to
> log in before you can send items from the cart.

It is still possible in master. I just did ??
Comment 8 Katrin Fischer 2012-06-28 08:09:41 UTC
Perhaps it changed again? :( I tested on 3.6.3.
Comment 9 Marcel de Rooy 2012-06-28 08:19:08 UTC
(In reply to comment #8)
> Perhaps it changed again? :( I tested on 3.6.3.

No, it is not possible. Somehow I cannot reproduce anymore what I did before..
Comment 10 Marcel de Rooy 2012-06-28 08:38:33 UTC
OK. The patch came from Frere Sebastien Marie. And current master does not allow anynomous users here (anymore).

One question remains: It works for me. But if you change the From address to the patron's address, could we have Relay Access Denied errors or similar? A mail server could reject outgoing mail if not sent from designated domains?
Comment 11 Sébastien Marie 2012-06-28 08:46:52 UTC
(En réponse au commentaire 10)
> One question remains: It works for me. But if you change the From address to
> the patron's address, could we have Relay Access Denied errors or similar? A
> mail server could reject outgoing mail if not sent from designated domains?

Yes, it is possible that mail will be reported as spam, if the patron address use SPF (spam prevention system based on whitelist of smtp authorised to send email for particuliar domain).
Comment 12 Sébastien Marie 2012-06-28 09:06:31 UTC
(En réponse au commentaire 11)
> (En réponse au commentaire 10)
> > One question remains: It works for me. But if you change the From address to
> > the patron's address, could we have Relay Access Denied errors or similar? A
> > mail server could reject outgoing mail if not sent from designated domains?
> 
> Yes, it is possible that mail will be reported as spam, if the patron
> address use SPF (spam prevention system based on whitelist of smtp
> authorised to send email for particuliar domain).

So it is possible to remove using GetFirstValidEmailAddress($borrowernumber) as From address, and inconditionnally use C4::Context->preference('KohaAdminEmailAddress').

But I think we should include *in the body* the name/address of the sender:
"This mail was sent to you from $user->{firstname} $user->{surname} <$user_email>"

So authentificated user will not abuse sending email using opac-sendbasket.pl

We could also add a Reply-To field with "$user->{firstname} $user->{surname} <$user_email>" ? (in order to prevent error like press the reply-to button and mail to KohaAdminEmailAddress instead of $user_email)
Comment 13 Sébastien Marie 2012-06-28 09:29:37 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2012-06-28 09:42:30 UTC
Created attachment 10548 [details] [review]
Signed patch for 3280
Comment 15 Marcel de Rooy 2012-06-28 09:46:13 UTC
QA Comment:
Small patch. Code looks good to me. Idea of Reply-to address seems to me a good approach as compared to changing the From address.
Passed QA
Comment 16 Marcel de Rooy 2012-06-28 09:54:34 UTC
(In reply to comment #3)
> Does opac/opac-sendshelf.pl should be have a separated bug report ?
> As there have same features (send email to someone), corrections (or
> enhancements) should be consistent.

Sebastien: You could also adjust the title of this report, take over the assignee field (you wrote the patch, not Galen) and submit another patch for send-shelf. That would be great!
Comment 17 Paul Poulain 2012-06-29 14:27:38 UTC
(assignee changed, to reflect who sent the patch)
Comment 18 Chris Cormack 2012-06-29 22:02:04 UTC
Because this now makes it impossible to send a cart without being logged in (which is probably a valid change) it is a big behaviour change which I dont want to spring on people mid cycle. Holding for 3.10
Comment 19 Katrin Fischer 2012-06-30 07:16:03 UTC
Hi Chris, but this is already the case in 3.6?
Comment 20 Katrin Fischer 2012-07-02 07:18:33 UTC
Ok, rereading through the bug report there is a bit that worries me:
- add field X-Orig-IP with IP of sender

IP addresses in web server logs have been issue of data privacy here - I wonder if it's ok adding the IP to the mail?
Comment 21 Marcel de Rooy 2012-07-02 07:45:57 UTC
(In reply to comment #20)
> Ok, rereading through the bug report there is a bit that worries me:
> - add field X-Orig-IP with IP of sender
> 
> IP addresses in web server logs have been issue of data privacy here - I
> wonder if it's ok adding the IP to the mail?

Would not worry about that too much. Could be lots of ip addresses in the mail headers showing the route of this message from A to B.
Moreover, measures against spammers here should not be considered as privacy intruding. You can find this ip address also in your access log; that is on your server not just in the mail header..
Comment 22 Paul Poulain 2012-07-02 08:01:37 UTC
(In reply to comment #21)
> (In reply to comment #20)
> > Ok, rereading through the bug report there is a bit that worries me:
> > - add field X-Orig-IP with IP of sender
> > 
> > IP addresses in web server logs have been issue of data privacy here - I
> > wonder if it's ok adding the IP to the mail?
> 
> Would not worry about that too much. Could be lots of ip addresses in the
> mail headers showing the route of this message from A to B.
+1, ans I suspect that, in France, it's legally requested to track this kind of things = the mail is sent by someone, the server admin must be able to answer any legal/court request
Comment 23 Marcel de Rooy 2012-07-02 08:12:14 UTC
Created attachment 10595 [details] [review]
Followup for privacy issue

Paul: Katrin raised the question on privacy issue on x-orig-ip.
Since we did not have it for years, i would suggest to remove it now first. And come up with a solution for all parties later on.
Comment 24 Marcel de Rooy 2012-07-02 08:14:13 UTC
Setting this to Signed off to get Paul's attention :-)
Comment 25 Marcel de Rooy 2012-07-02 08:36:27 UTC
(In reply to comment #24)
> Setting this to Signed off to get Paul's attention :-)
Passed QA may even be a better idea..
Comment 26 Paul Poulain 2012-07-03 16:17:55 UTC
OK, patch pushed and discussion started on mailing lists
Comment 27 Marcel de Rooy 2012-07-04 14:15:02 UTC
(In reply to comment #26)
> OK, patch pushed and discussion started on mailing lists
Paul: I do not see the followup appearing in master ?
Comment 28 Paul Poulain 2012-07-04 14:22:05 UTC
(In reply to comment #27)
> (In reply to comment #26)
> > OK, patch pushed and discussion started on mailing lists
> Paul: I do not see the followup appearing in master ?

Sorry, it was commited on my local computer, forgot to push yesterday, before leaving office. It's pushed now
Comment 29 Katrin Fischer 2012-07-05 13:39:16 UTC
I have forwarded this to a coworker and hoping to find out what we have to do soon. Whatever we do, we should not forget about the lists as they are also sent out using a similar technique.
Comment 30 Chris Cormack 2013-04-20 10:55:06 UTC
This was released in 3.10.0