Bug 7550 - Self checkout: limit display of patron image to logged-in patron
Summary: Self checkout: limit display of patron image to logged-in patron
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Self checkout (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Target Milestone: ---
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL: /cgi-bin/koha/sco/sco-patron-image.pl...
Whiteboard:
Keywords:
Depends on:
Blocks: 12691
  Show dependency treegraph
 
Reported: 2012-02-16 19:58 UTC by Owen Leonard
Modified: 2017-12-07 22:20 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 7550 - Self checkout: limit display of patron image to logged-in patron (4.16 KB, patch)
2017-04-18 12:11 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 7550 - Self checkout: limit display of patron image to logged-in patron (3.94 KB, patch)
2017-04-18 12:18 UTC, Marc Véron
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7550 - Self checkout: limit display of patron image to logged-in patron (4.06 KB, patch)
2017-04-18 13:18 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 7550: (follow-up) Use image file to create hash (2.80 KB, patch)
2017-04-18 18:20 UTC, Marc Véron
Details | Diff | Splinter Review
[ALTERNATIVE-PATCH] Bug 7550: SCO - Restrict access of patron's image (3.83 KB, patch)
2017-04-19 17:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7550: SCO - Restrict access of patron's image (4.31 KB, patch)
2017-04-20 06:06 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 7550: SCO - Restrict access of patron's image (4.40 KB, patch)
2017-05-01 12:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7550: [QA Follow-up] Resolve param warning from sco-patron-image (1.90 KB, patch)
2017-05-01 12:29 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 Owen Leonard 2012-02-16 19:58:42 UTC
The patron image display in the self-checkout takes a GET parameter from the image source, so if someone copied the image location and substituted the barcode string they could browse through all patron images:

<img alt="" src="/cgi-bin/koha/sco/sco-patron-image.pl?cardnumber=XXXX">

It would offer patrons better privacy to limit that request based on the currently-logged-in user.
Comment 1 Owen Leonard 2014-07-16 13:38:30 UTC
This bug is still valid in master, the only difference being that the image is now called by borrowernumber instead of card number:

<img alt="" src="/cgi-bin/koha/sco/sco-patron-image.pl?borrowernumber=XXXX">
Comment 2 Jonathan Druart 2015-04-08 07:55:44 UTC
(In reply to Owen Leonard from comment #0)
> The patron image display in the self-checkout takes a GET parameter from the
> image source, so if someone copied the image location and substituted the
> barcode string they could browse through all patron images:
> 
> <img alt="" src="/cgi-bin/koha/sco/sco-patron-image.pl?cardnumber=XXXX">
> 
> It would offer patrons better privacy to limit that request based on the
> currently-logged-in user.

It could only work if SelfCheckoutByLogin is set to 'username and password'.
Comment 3 Marc Véron 2016-08-19 09:33:19 UTC
Still valid.
Comment 4 Marc Véron 2017-04-18 12:11:27 UTC Comment hidden (obsolete)
Comment 5 Marc Véron 2017-04-18 12:18:27 UTC
Created attachment 62272 [details] [review]
Bug 7550 - Self checkout: limit display of patron image to logged-in patron

The patron image display in the self-checkout takes a GET parameter from the
image source, so if someone copied the image location and substituted the
barcode string they could browse through all patron images:

<img alt="" src="/cgi-bin/koha/sco/sco-patron-image.pl?borrowernumber=XXXX">

To reproduce:
- Enable self checkout, go to [Your Server]//cgi-bin/koha/sco/sco-main.pl
- Log in with a user 'A' who has a patron image
- Copy the address of the patron image into an other browser window
- Change the borrowernumber to on of an other user 'B' having a patron image
- Verify that the patron image is displayed

To test:
- Apply patch, restart plack / memcached
- Try to reproduce
- Verify that you can no longer display the image of user 'B' by
  tweaking the image address
Comment 6 Owen Leonard 2017-04-18 13:18:37 UTC
Created attachment 62289 [details] [review]
[SIGNED-OFF] Bug 7550 - Self checkout: limit display of patron image to logged-in patron

The patron image display in the self-checkout takes a GET parameter from
the image source, so if someone copied the image location and
substituted the barcode string they could browse through all patron
images:

<img alt="" src="/cgi-bin/koha/sco/sco-patron-image.pl?borrowernumber=XXXX">

To reproduce:
- Enable self checkout, go to [Your
  Server]//cgi-bin/koha/sco/sco-main.pl
- Log in with a user 'A' who has a patron image
- Copy the address of the patron image into an other browser window
- Change the borrowernumber to on of an other user 'B' having a patron
  image
- Verify that the patron image is displayed

To test:
- Apply patch, restart plack / memcached
- Try to reproduce
- Verify that you can no longer display the image of user 'B' by
  tweaking the image address

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

https://bugs.koha-community.org/show_bug.cgi?id=7750
Comment 7 Jonathan Druart 2017-04-18 13:31:10 UTC
No you cannot, unless you know the cardnumber.
Everybody can guess the 'ck' value from the cardnumber.
It may be better to use a hash based on the image.
Comment 8 Marc Véron 2017-04-18 18:20:15 UTC
Created attachment 62311 [details] [review]
Bug 7550: (follow-up) Use image file to create hash

Follow-up as requested by commment #7
Comment 9 Marc Véron 2017-04-18 18:22:06 UTC
Hope it's OK to switch back to Signed Off.
Comment 10 Jonathan Druart 2017-04-18 18:50:03 UTC
Marcel, Do not you think Koha::Token could/should be extended to generate this kind of token?
Comment 11 Marcel de Rooy 2017-04-19 11:15:24 UTC
(In reply to Jonathan Druart from comment #10)
> Marcel, Do not you think Koha::Token could/should be extended to generate
> this kind of token?

Sounds good to me.
Comment 12 Jonathan Druart 2017-04-19 17:12:28 UTC
Created attachment 62400 [details] [review]
[ALTERNATIVE-PATCH] Bug 7550: SCO - Restrict access of patron's image

With this patch if SelfCheckoutByLogin is set to 'username and
password', only the logged in user will be able to see the image linked
to his/her logged in account.
If set to "barcode" we generate a token but it can be easily generated.
You should add a warning in the about page if
SelfCheckoutByLogin="barcode" and ShowPatronImageInWebBasedSelfCheck="Show".
Comment 13 Jonathan Druart 2017-04-19 17:13:46 UTC
No need to extend Koha::Token actually.
It sounds more logical to use it, that's why I have attached an alternative patch using it.

Marcel could you take a look at the 2 approaches please?
Comment 14 Marc Véron 2017-04-19 18:58:49 UTC
(In reply to Jonathan Druart from comment #12)
> Created attachment 62400 [details] [review] [review]
> [ALTERNATIVE-PATCH] Bug 7550: SCO - Restrict access of patron's image
> 
> With this patch if SelfCheckoutByLogin is set to 'username and
> password', only the logged in user will be able to see the image linked
> to his/her logged in account.
> If set to "barcode" we generate a token but it can be easily generated.
> You should add a warning in the about page if
> SelfCheckoutByLogin="barcode" and ShowPatronImageInWebBasedSelfCheck="Show".

Hmm, my patch worked with a hash generated with the image file (as recommended in comment #7), and it did not leave a security hole with SelfCheckoutByLogin="barcode"
Comment 15 Jonathan Druart 2017-04-19 19:24:30 UTC
(In reply to Marc Véron from comment #14)
> (In reply to Jonathan Druart from comment #12)
> > Created attachment 62400 [details] [review] [review] [review]
> > [ALTERNATIVE-PATCH] Bug 7550: SCO - Restrict access of patron's image
> > 
> > With this patch if SelfCheckoutByLogin is set to 'username and
> > password', only the logged in user will be able to see the image linked
> > to his/her logged in account.
> > If set to "barcode" we generate a token but it can be easily generated.
> > You should add a warning in the about page if
> > SelfCheckoutByLogin="barcode" and ShowPatronImageInWebBasedSelfCheck="Show".
> 
> Hmm, my patch worked with a hash generated with the image file (as
> recommended in comment #7), and it did not leave a security hole with
> SelfCheckoutByLogin="barcode"

Yes it does, on the same way as my patch. If you know the cardnumber (easy to guess) of someone you can access his^Ctheir image.
Comment 16 Marc Véron 2017-04-19 20:17:29 UTC
(In reply to Jonathan Druart from comment #15)
> (In reply to Marc Véron from comment #14)
> > (In reply to Jonathan Druart from comment #12)
> > > Created attachment 62400 [details] [review] [review] [review] [review]
> > > [ALTERNATIVE-PATCH] Bug 7550: SCO - Restrict access of patron's image
> > > 
> > > With this patch if SelfCheckoutByLogin is set to 'username and
> > > password', only the logged in user will be able to see the image linked
> > > to his/her logged in account.
> > > If set to "barcode" we generate a token but it can be easily generated.
> > > You should add a warning in the about page if
> > > SelfCheckoutByLogin="barcode" and ShowPatronImageInWebBasedSelfCheck="Show".
> > 
> > Hmm, my patch worked with a hash generated with the image file (as
> > recommended in comment #7), and it did not leave a security hole with
> > SelfCheckoutByLogin="barcode"
> 
> Yes it does, on the same way as my patch. If you know the cardnumber (easy
> to guess) of someone you can access his^Ctheir image.

$patron_image->imagefile is a blob, no? - Really easy to guess.
Comment 17 Jonathan Druart 2017-04-19 20:32:07 UTC
I do not understand how your approach can protect the image in any way if SelfCheckoutByLogin="barcode".

Hit /cgi-bin/koha/sco/sco-main.pl
Guess a cardnumber
=> You see the image

Both approaches have the same problem, that's why I suggest to add a warning on the about page.

The only difference is that the second approach uses Koha::Token that it already used at different places.
Comment 18 Marc Véron 2017-04-19 21:32:52 UTC
Could it be that we talk about different things? 

The initial comment was:
-----
The patron image display in the self-checkout takes a GET parameter from the image source, so if someone copied the image location and substituted the barcode string they could browse through all patron images:

<img alt="" src="/cgi-bin/koha/sco/sco-patron-image.pl?cardnumber=XXXX">

It would offer patrons better privacy to limit that request based on the currently-logged-in user.
-----
(Note that meanwhile the param is borrowernumber).

So somebody who is logged in to SCO can browse through the images by guessing/changing the borrowernumber:
http://192.168.1.31/cgi-bin/koha/sco/sco-patron-image.pl?borrowernumber=1
http://192.168.1.31/cgi-bin/koha/sco/sco-patron-image.pl?borrowernumber=2

With my two patches applied, a hash is added, based on the contents of the image. The contents of the image is stored in the database (as blob)
The new link to display the image is something like:
http://192.168.1.31/cgi-bin/koha/sco/sco-patron-image.pl?borrowernumber=1&ck=a0923303852da8c10a383fb81f1a73d3
If you copy this addres and change borrowernumber e.g. to 2, the image won't display.

To create a hash that fits to borrowernumber 2, you would need the image, otherwise it is impossible.

So again: Are we talking about different things? 
Are you talking about accessing the SCO screen by guessing the card number?
Comment 19 Jonathan Druart 2017-04-19 21:50:10 UTC
(In reply to Marc Véron from comment #18)
> So again: Are we talking about different things? 
> Are you talking about accessing the SCO screen by guessing the card number?

Well it will be hard to answer binarily apparently as you are considering it's 2 different things and I consider it's the same thing.
When you load sco-main.pl it GET sco-patron-image.pl, so there is no different for me.

As I said earlier both versions do the same thing and have the same problem. Just considering using Koha::Token is the way to go.
Comment 20 Marc Véron 2017-04-20 06:01:07 UTC
I'm not at all against the token approach, but I still think there is a misunderstanding about the original target of this bug. 

Since I do not want to block the bug I will sign of your alternative patch.
Comment 21 Marc Véron 2017-04-20 06:06:23 UTC
Created attachment 62441 [details] [review]
Bug 7550: SCO - Restrict access of patron's image

With this patch if SelfCheckoutByLogin is set to 'username and
password', only the logged in user will be able to see the image linked
to his/her logged in account.
If set to "barcode" we generate a token but it can be easily generated.
You should add a warning in the about page if
SelfCheckoutByLogin="barcode" and ShowPatronImageInWebBasedSelfCheck="Show".

How I tested:
- Go to SCO
- Log - Enable self checkout, go to [Your
  Server]//cgi-bin/koha/sco/sco-main.pl
- Log in with a user 'A' who has a patron image
- Copy the address of the patron image into an other browser window
- Change the borrowernumber to on of an other user 'B' having a patron
  image
- Verify that the patron image is NOT displayed

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 22 Jonathan Druart 2017-04-20 13:05:52 UTC
(In reply to Marc Véron from comment #20)
> I'm not at all against the token approach, but I still think there is a
> misunderstanding about the original target of this bug. 

Please detail what's wrong, I think I fully understood the original issue raised.
Comment 23 Marc Véron 2017-04-20 13:22:43 UTC
(In reply to Jonathan Druart from comment #22)
> (In reply to Marc Véron from comment #20)
> > I'm not at all against the token approach, but I still think there is a
> > misunderstanding about the original target of this bug. 
> 
> Please detail what's wrong, I think I fully understood the original issue
> raised.

Nothing wrong with your patch. I tested it and signed it off.
Comment 24 Marcel de Rooy 2017-04-20 13:42:23 UTC
(In reply to Marc Véron from comment #14)
> Hmm, my patch worked with a hash generated with the image file (as
> recommended in comment #7), and it did not leave a security hole with
> SelfCheckoutByLogin="barcode"

Looks to me that this option is a security hole on itself?
If I guess barcodes, I can still see all images? If I come on sco-main, I will automatically get the image from the img tag as well? Or do I misunderstand the discussion here?
Comment 25 Jonathan Druart 2017-04-20 14:06:08 UTC
(In reply to Marcel de Rooy from comment #24)
> (In reply to Marc Véron from comment #14)
> > Hmm, my patch worked with a hash generated with the image file (as
> > recommended in comment #7), and it did not leave a security hole with
> > SelfCheckoutByLogin="barcode"
> 
> Looks to me that this option is a security hole on itself?
> If I guess barcodes, I can still see all images? If I come on sco-main, I
> will automatically get the image from the img tag as well? Or do I
> misunderstand the discussion here?

The commit message says everything:
"""
With this patch if SelfCheckoutByLogin is set to 'username and
password', only the logged in user will be able to see the image linked
to his/her logged in account.
If set to "barcode" we generate a token but it can be easily generated.
You should add a warning in the about page if
SelfCheckoutByLogin="barcode" and ShowPatronImageInWebBasedSelfCheck="Show".
"""
Comment 26 Marc Véron 2017-04-20 14:18:17 UTC
(In reply to Marcel de Rooy from comment #24)
> (In reply to Marc Véron from comment #14)
> > Hmm, my patch worked with a hash generated with the image file (as
> > recommended in comment #7), and it did not leave a security hole with
> > SelfCheckoutByLogin="barcode"
> 
> Looks to me that this option is a security hole on itself?
> If I guess barcodes, I can still see all images? If I come on sco-main, I
> will automatically get the image from the img tag as well? Or do I
> misunderstand the discussion here?

We have two situations:

Situation # 1

- SCO is up and running. A user logs in with what ever credentioals necessary, depending on SelfCheckoutByLogin 
- User copies the image link into the address bar of a new browser window. 
  It is something like:
  .../cgi-bin/koha/sco/sco-patron-image.pl?borrowernumber=XXXX
- User changes the borrowernumber
- Image of an other user is displayed (should not be possible)

That's what this bug is about. Problem is solved by adding an unguessable token to the link.

Problem #2

- SCO is up and running. SelfCheckOut is set to barcode (i.e. card number)
- Someboy comes along the SCO station and tries to log in by guessing card numbers. If the numbering pattern is simple, there is a good chance that they can break in.

That's what this bug is not about. IMO problem #2 should be discussed and addressed in a new bug.
Comment 27 Marcel de Rooy 2017-05-01 12:29:20 UTC
Created attachment 62912 [details] [review]
Bug 7550: SCO - Restrict access of patron's image

With this patch if SelfCheckoutByLogin is set to 'username and
password', only the logged in user will be able to see the image linked
to his/her logged in account.
If set to "barcode" we generate a token but it can be easily generated.
You should add a warning in the about page if
SelfCheckoutByLogin="barcode" and ShowPatronImageInWebBasedSelfCheck="Show".

How I tested:
- Go to SCO
- Log - Enable self checkout, go to [Your
  Server]//cgi-bin/koha/sco/sco-main.pl
- Log in with a user 'A' who has a patron image
- Copy the address of the patron image into an other browser window
- Change the borrowernumber to on of an other user 'B' having a patron
  image
- Verify that the patron image is NOT displayed

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 28 Marcel de Rooy 2017-05-01 12:29:25 UTC
Created attachment 62913 [details] [review]
Bug 7550: [QA Follow-up] Resolve param warning from sco-patron-image

Resolve this warning:
  CGI::param called in list context from package C4::Service line 212, this can lead to vulnerabilities. See the warning in "Fetching the value or values of a single named parameter" at /usr/share/perl5/CGI.pm line 436.

It comes from the require_params call in sco-patron-image.pl.

Git grepping on require_params tells me this:
  members/default_messageprefs.pl:my ($categorycode) = C4::Service->require_params('categorycode');
  opac/sco/sco-patron-image.pl:my ($borrowernumber) = C4::Service->require_params('borrowernumber');
  opac/sco/sco-patron-image.pl:my ($csrf_token) = C4::Service->require_params('csrf_token');
  svc/cataloguing/metasearch:my ( $query_string, $servers ) = C4::Service->require_params( 'q', 'servers' );

The only candidate for multi_param seems to be 'servers', but as we can see
this variable is a scalar. Additional servers returned by require_params are
lost. This should be solved on its own report.
So, we can safely add scalar to the params call, resolve the warning and
keep the same behavior.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 29 Kyle M Hall 2017-05-08 13:01:13 UTC
Pushed to master for 17.05, thanks Jonathan, Marcel!
Comment 30 Katrin Fischer 2017-05-13 12:48:13 UTC
These patches have been pushed to 16.11.x and will be in 16.11.08.
Comment 31 Mason James 2017-05-24 02:12:15 UTC
Pushed to 16.05.x, for 16.05.13 release