Bug 7550

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">

(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'.

Still valid.

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

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

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.

Created attachment 62311 [details] [review]
Bug 7550: (follow-up) Use image file to create hash

Follow-up as requested by commment #7

Hope it's OK to switch back to Signed Off.

Marcel, Do not you think Koha::Token could/should be extended to generate this kind of token?

(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.

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".

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?

(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"

(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.

(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.

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.

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?

(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.

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.

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>

(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.

(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.

(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?

(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".
"""

(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.

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>

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>

Pushed to master for 17.05, thanks Jonathan, Marcel!

These patches have been pushed to 16.11.x and will be in 16.11.08.

Pushed to 16.05.x, for 16.05.13 release