Bug 28735

Created attachment 123025 [details] [review]
Bug 28735: Self-checkout users can access opac-user.pl for sco user when not using AutoSelfCheckID

This patch makes the sandboxing of the selfcheckout more robust by
adding a "sco_user" session variable which is turned on when
logging into the self-checkout (either by AutoSelfCheckAllowed or manually).

If a user with this session variable turned on tries to access
other parts of the system (like the rest of the OPAC), it will
"kick out", so that the browser user will lose the authenticated session.

Test plan:
1) Apply the patch
2) koha-plack --restart kohadev
3) Go to http://localhost:8080/cgi-bin/koha/sco/sco-main.pl
4) Note that you are logged into the self-checkout
5) Go to http://localhost:8080/cgi-bin/koha/opac-main.pl
6) Note that you are not logged into the OPAC
7) Log into the staff interface and disable the
system preference AutoSelfCheckAllowed
8) Log out of the staff interface (this step is very important)
9) Go to http://localhost:8080/cgi-bin/koha/sco/sco-main.pl
10) Note that you are prompted to log into Koha
11) Login using the "koha" user (when using koha-testing-docker)
12) Note that you are logged into the self-checkout
13) Go to http://localhost:8080/cgi-bin/koha/opac-main.pl
14) Note that you are not logged into the OPAC
15) Go back to http://localhost:8080/cgi-bin/koha/sco/sco-main.pl
16) Note that you will need to log in again as you've lost your
session cookie

Voila!

+                $is_sco_user = $session->param('sco_user');

-            && C4::Context->preference('AutoSelfCheckID')
-            && $user eq C4::Context->preference('AutoSelfCheckID')

Why would a parameter that you can manipulate/remove be safer than comparing the user code ?

(In reply to Marcel de Rooy from comment #3)
> +                $is_sco_user = $session->param('sco_user');
> 
> -            && C4::Context->preference('AutoSelfCheckID')
> -            && $user eq C4::Context->preference('AutoSelfCheckID')
> 
> Why would a parameter that you can manipulate/remove be safer than comparing
> the user code ?

I'm not sure that I quite understand your question. Could you rephrase it?

The problem with checking AutoSelfCheckID is that not all self-checkout logins are done using AutoSelfCheckID, so it's just not an all-inclusive condition to check. It's impossible to know whether a session is a self-checkout session or not based on username alone (outside the scenario of a single branch using a self-checkout with one AutoSelfCheckID). 

The session variable is a robust way of tracking server-side whether the session is for a self-checkout user or a regular user.

(In reply to David Cook from comment #4)

> The problem with checking AutoSelfCheckID is that not all self-checkout
> logins are done using AutoSelfCheckID, so it's just not an all-inclusive
> condition to check. It's impossible to know whether a session is a
> self-checkout session or not based on username alone (outside the scenario
> of a single branch using a self-checkout with one AutoSelfCheckID). 
> 
> The session variable is a robust way of tracking server-side whether the
> session is for a self-checkout user or a regular user.

Thx. Looking a few seconds more, it becomes clearer ;)

     if ( $in->{type} eq 'opac' && $user ) {
+        my $is_sco_user;
+        if ($sessionID){
+            my $session = get_session($sessionID);
+            if ($session){
+                $is_sco_user = $session->param('sco_user');
+            }
+        }
         my $kick_out;

         if (
 # If the user logged in is the SCO user and they try to go out of the SCO module,
 # log the user out removing the CGISESSID cookie
             $in->{template_name} !~ m|sco/| && $in->{template_name} !~ m|errors/errorpage.tt|
-            && C4::Context->preference('AutoSelfCheckID')
-            && $user eq C4::Context->preference('AutoSelfCheckID')
+            && $is_sco_user
           )
         {
             $kick_out = 1;

Still raises questions with me.

1) You remove the check on AutoSelfCheckID. Which means that this user can now login to the OPAC. We advertize that this is not possible. There is actually no need to remove that check. You can extend it.
2) For every non-anonymous opac page hit we now have an additional database query for the sco_user in the session table. You do this to prevent access to opac-user or even opac-memberentry. Is there a cheaper way? No blocker.

(In reply to Marcel de Rooy from comment #6)
> 1) You remove the check on AutoSelfCheckID. Which means that this user can
> now login to the OPAC. We advertize that this is not possible. There is
> actually no need to remove that check. You can extend it.

That's true. I think it's a bizarre exception, but it is one that we promise, so you're right I should've kept it. 

> 2) For every non-anonymous opac page hit we now have an additional database
> query for the sco_user in the session table. You do this to prevent access
> to opac-user or even opac-memberentry. Is there a cheaper way? No blocker.

I think that would be a premature optimization, but there's a number of ways you could do it.

We could set a L1 cache in checkauth() with the session object, and then update get_session() to use that cache. 

Another option would be to return the session object from checkauth(). At the moment we just return the session ID which is why we have to fetch it. I think Jonathan might have a patch for that somewhere but I'm not sure where...

You could use C4::Context->set_userenv, but my long-term goal is to actually remove that that function, so I rather not add more to it due to inertia / existing technical debt.

Other options would probably require a lot more work. 

Further in get_user_and_template() we use get_session with OpacBrowseResults as well. that should just use the session object returned by checkauth(). 

There are probably other calls in get_user_and_template which have get_session() nested in them as well like C4::Search::History::get_from_session which gets called a couple separate times in different blocks.

Created attachment 125918 [details] [review]
Bug 28735: Self-checkout users can access opac-user.pl for sco user when not using AutoSelfCheckID

This patch makes the sandboxing of the selfcheckout more robust by
adding a "sco_user" session variable which is turned on when
logging into the self-checkout (either by AutoSelfCheckAllowed or manually).

If a user with this session variable turned on tries to access
other parts of the system (like the rest of the OPAC), it will
"kick out", so that the browser user will lose the authenticated session.

Test plan:
1) Apply the patch
2) koha-plack --restart kohadev
3) Go to http://localhost:8080/cgi-bin/koha/sco/sco-main.pl
4) Note that you are logged into the self-checkout
5) Go to http://localhost:8080/cgi-bin/koha/opac-main.pl
6) Note that you are not logged into the OPAC
7) Log into the staff interface and disable the
system preference AutoSelfCheckAllowed
8) Log out of the staff interface (this step is very important)
9) Go to http://localhost:8080/cgi-bin/koha/sco/sco-main.pl
10) Note that you are prompted to log into Koha
11) Login using the "koha" user (when using koha-testing-docker)
12) Note that you are logged into the self-checkout
13) Go to http://localhost:8080/cgi-bin/koha/opac-main.pl
14) Note that you are not logged into the OPAC
15) Go back to http://localhost:8080/cgi-bin/koha/sco/sco-main.pl
16) Note that you will need to log in again as you've lost your
session cookie

Voila!

That should resolve the only blocker I think

> 4) Note that you are logged into the self-checkout

Where should have I been logged before and with which user?


Which step should be different without the patch?

(In reply to Victor Grousset/tuxayo from comment #10)
> > 4) Note that you are logged into the self-checkout
> 
> Where should have I been logged before and with which user?
> 

Logged in using the AutoSelfCheckID user, which will be invisible to you as a self-checkout user. Basically this just means you're not prompted to log into the self-checkout as an admin user (which is different to identifying yourself to the self-checkout at the borrower). 

> 
> Which step should be different without the patch?

Without the patch, step 14 would be different. For step 14 you would be logged into the OPAC as the "koha" user which was used to log into the self-checkout (as an admin user).

Thanks for the details.

Whether on master or with the patch, when going there , I'm not logged in :o

I'm also using koha-testing-docker. Does step 4) still works for you?

(In reply to Victor Grousset/tuxayo from comment #12)
> Thanks for the details.
> 
> Whether on master or with the patch, when going there , I'm not logged in :o
> 
> I'm also using koha-testing-docker. Does step 4) still works for you?

It is possible that koha-testing-docker has been  updated since I did the work on this.

I'll have to take another look with a more up-to-date koha-testing-docker. It could be that my testing steps are outdated!

(In reply to Victor Grousset/tuxayo from comment #12)
> Thanks for the details.
> 
> Whether on master or with the patch, when going there , I'm not logged in :o
> 
> I'm also using koha-testing-docker. Does step 4) still works for you?

Ok I've rebased and re-tried this and yes step 4 still works for me.

I think I haven't described it well enough for you.

--

For step 4, you should see "Self checkout system" at the top of the page and then see "Log in to your account" with a "Login" box and a "Password" box. 

That means that you're logged into the self checkout already using the self_checkout user defined in AutoSelfCheckID.

--

If you remove the value from AutoSelfCheckID and try again, you'll see a normal Koha login screen that doesn't show "Self checkout system" at all. 

This means that you're not logged into the self checkout system.

If you turn off AutoSelfCheckAllowed as well, you can log in using the "koha" account here, which logs you into the "Self checkout system", which then prompts you to "Log in" as a self checkout patron.

--

Does that make sense?

Note that *without this patch*... Step 13 would be different. If you log into the self checkout as the "koha" user (when AutoSelfCheckAllowed is disabled), and then go to http://localhost:8080/cgi-bin/koha/opac-main.pl, you'll be logged in as that user, which is bad.

In this scenario, a person could go to a self checkout terminal (which isn't using AutoSelfCheckAllowed) and then get into http://localhost:8080/cgi-bin/koha/opac-user.pl 

Depending on the Koha settings, a malicious user could get up to a number of problematic activities (which I won't list here).

> For step 4, you should see "Self checkout system" at the top of the page and then see "Log in to your account" with a "Login" box and a "Password" box. 

> That means that you're logged into the self checkout already using the self_checkout user defined in AutoSelfCheckID.


Okkkkkk it's nested authentication, I get it now! *mind blown gif*

Created attachment 128936 [details] [review]
Bug 28735: Self-checkout users can access opac-user.pl for sco user when not using AutoSelfCheckID

This patch makes the sandboxing of the selfcheckout more robust by
adding a "sco_user" session variable which is turned on when
logging into the self-checkout (either by AutoSelfCheckAllowed or manually).

If a user with this session variable turned on tries to access
other parts of the system (like the rest of the OPAC), it will
"kick out", so that the browser user will lose the authenticated session.

Test plan:
1) Apply the patch
2) koha-plack --restart kohadev
3) Go to http://localhost:8080/cgi-bin/koha/sco/sco-main.pl
4) Note that you are logged into the self-checkout
     So you see the login screen specific to the self-checkout.
     To log with the actual patron. It's a nested auth.
5) Go to http://localhost:8080/cgi-bin/koha/opac-main.pl
6) Note that you are not logged into the OPAC
7) Log into the staff interface and disable the
system preference AutoSelfCheckAllowed
8) Log out of the staff interface (this step is very important)
9) Go to http://localhost:8080/cgi-bin/koha/sco/sco-main.pl
10) Note that you are prompted to log into Koha
11) Login using the "koha" user (when using koha-testing-docker)
12) Note that you are logged into the self-checkout
13) Go to http://localhost:8080/cgi-bin/koha/opac-main.pl
14) Note that you are not logged into the OPAC
      Without the patch you would still be logged as "koha"
15) Go back to http://localhost:8080/cgi-bin/koha/sco/sco-main.pl
16) Note that you will need to log in again as you've lost your
session cookie
      Without the patch you will still be logged in the self-checkout
Voila!

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

It works as advertised :)

A few test steps were detailed to explicit the changes of the patch.
And in the rare case someone doesn't know about the self-checkout nested auth.

4) Note that you are logged into the self-checkout
     So you see the login screen specific to the self-checkout.
     To log with the actual patron. It's a nested auth.

14) Note that you are not logged into the OPAC
      Without the patch you would still be logged as "koha"

16) Note that you will need to log in again as you've lost your
session cookie
      Without the patch you will still be logged in the self-checkout

This applies cleanly on top of 29543 which has already passed QA, so I am going to test them together.

Created attachment 129229 [details] [review]
Bug 28735: Self-checkout users can access opac-user.pl for sco user when not using AutoSelfCheckID

This patch makes the sandboxing of the selfcheckout more robust by
adding a "sco_user" session variable which is turned on when
logging into the self-checkout (either by AutoSelfCheckAllowed or manually).

If a user with this session variable turned on tries to access
other parts of the system (like the rest of the OPAC), it will
"kick out", so that the browser user will lose the authenticated session.

Test plan:
1) Apply the patch
2) koha-plack --restart kohadev
3) Go to http://localhost:8080/cgi-bin/koha/sco/sco-main.pl
4) Note that you are logged into the self-checkout
     So you see the login screen specific to the self-checkout.
     To log with the actual patron. It's a nested auth.
5) Go to http://localhost:8080/cgi-bin/koha/opac-main.pl
6) Note that you are not logged into the OPAC
7) Log into the staff interface and disable the
system preference AutoSelfCheckAllowed
8) Log out of the staff interface (this step is very important)
9) Go to http://localhost:8080/cgi-bin/koha/sco/sco-main.pl
10) Note that you are prompted to log into Koha
11) Login using the "koha" user (when using koha-testing-docker)
12) Note that you are logged into the self-checkout
13) Go to http://localhost:8080/cgi-bin/koha/opac-main.pl
14) Note that you are not logged into the OPAC
      Without the patch you would still be logged as "koha"
15) Go back to http://localhost:8080/cgi-bin/koha/sco/sco-main.pl
16) Note that you will need to log in again as you've lost your
session cookie
      Without the patch you will still be logged in the self-checkout
Voila!

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 129680 [details] [review]
Bug 28735: [19.11] Self-checkout users can access opac-user.pl for sco user when not using AutoSelfCheckID

This patch makes the sandboxing of the selfcheckout more robust by
adding a "sco_user" session variable which is turned on when
logging into the self-checkout (either by AutoSelfCheckAllowed or manually).

If a user with this session variable turned on tries to access
other parts of the system (like the rest of the OPAC), it will
"kick out", so that the browser user will lose the authenticated session.

Test plan:
1) Apply the patch
2) koha-plack --restart kohadev
3) Go to http://localhost:8080/cgi-bin/koha/sco/sco-main.pl
4) Note that you are logged into the self-checkout
     So you see the login screen specific to the self-checkout.
     To log with the actual patron. It's a nested auth.
5) Go to http://localhost:8080/cgi-bin/koha/opac-main.pl
6) Note that you are not logged into the OPAC
7) Log into the staff interface and disable the
system preference AutoSelfCheckAllowed
8) Log out of the staff interface (this step is very important)
9) Go to http://localhost:8080/cgi-bin/koha/sco/sco-main.pl
10) Note that you are prompted to log into Koha
11) Login using the "koha" user (when using koha-testing-docker)
12) Note that you are logged into the self-checkout
13) Go to http://localhost:8080/cgi-bin/koha/opac-main.pl
14) Note that you are not logged into the OPAC
      Without the patch you would still be logged as "koha"
15) Go back to http://localhost:8080/cgi-bin/koha/sco/sco-main.pl
16) Note that you will need to log in again as you've lost your
session cookie
      Without the patch you will still be logged in the self-checkout
Voila!

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz>

Seems to be working as expected, backporting to 19.11.x-security branch on the Security repo for 19.11.25 security release.

I did get some conflicts though when merging but I seemed to be able to fix them...

Backported: Pushed to 20.11.x-security branch for 20.11.14

> I did get some conflicts though when merging but I seemed to be able to fix them...

I got the same when doing my backport in parallel and double checked with your branch to see if I missed something and IIUC in 19.11.x-security there is removal of unrelated lines that are from 19.11.x and should be kept and addition of line that are from next versions of Koha.

The issue is the default conflictstyle setting of git that doesn't show full picture and forces us to makes guesses.
diff3 is a more complete conflictstyle that is a tremendous help to solve conflicts.
https://stackoverflow.com/questions/27417656/should-diff3-be-default-conflictstyle-on-git
tl;dr it adds a middle conflict section that shows the code before the patch being applied/cherry-picked/merged.
So for RMaint we can see what are the changes that are due from the different Koha versions and not the patch changes.

For this bug here is what I got when I cherry picked the patch from Kyle's security branch to 20.11.x

<<<<<<< HEAD
                    $debug and printf STDERR "AUTH_4: (%s)\t%s %s - %s\n", map { $session->param($_) } qw(cardnumber firstname surname branch);
||||||| parent of 6d022889a2 (Bug 28735: Self-checkout users can access opac-user.pl for sco user when not using AutoSelfCheckID)
=======
                    $session->param( 'sco_user', $is_sco_user );
>>>>>>> 6d022889a2 (Bug 28735: Self-checkout users can access opac-user.pl for sco user when not using AutoSelfCheckID)




=== Clarification of the diff3 conflictStyle display ===
<<<<<<< HEAD

# code from current 20.11.x-security

=======

# code from upstream 21.11.x before the patch

||||||| parent of 6d022889a2 (Bug 28735: Self-checkout users can access opac-user.pl for sco user when not using AutoSelfCheckID)

# code from upstream 21.11.x after the patch

>>>>>>> 6d022889a2 (Bug 28735: Self-checkout users can access opac-user.pl for sco user when not using AutoSelfCheckID)



=== Interpretation ===
1st section tells that "debug and printf..." are in 20.11.x

Comparing it with the 2nd section tells that it got removed in a future version of Koha, since it's not there is vanilla 21.11.x
External checking of the vanilla branches confirms this:
20.11.x: https://gitlab.com/koha-community/Koha/-/blob/68428d0eeb9f2f4c66083694fe0f0bfe55e3bef5/C4/Auth.pm#L1235
21.11.x: https://gitlab.com/koha-community/Koha/-/blob/db10542d33056a07563f151fb20dfdd168f9a064/C4/Auth.pm#L1188

Comparing the 2nd and 3rd section tells that this ticket added the "sco_user" param.

So in conclusion I had to keep "debug and printf..." line since it's part of 20.11.x and the backported patch doesn't remove it.
And add the "sco_user" param.

The default conflictStyle would have shown as if "debug and printf..." was replaced by "sco_user" param.

=== back to 19.11.x ===
So in 19.11.x "debug and printf" should be kept also.
Comparing 19.11.x and 20.11.x shows that your conflict should be also more tricky due to register_id and register_name params that have been added between the two Koha releases. So these params should not be kept for backporting to 19.11.x
19.11.x: https://gitlab.com/koha-community/Koha/-/blob/ecfdb05e9e03728be6d9ad02dc25c748a056748b/C4/Auth.pm#L1201
20.11.x: https://gitlab.com/koha-community/Koha/-/blob/68428d0eeb9f2f4c66083694fe0f0bfe55e3bef5/C4/Auth.pm#L1235

Using the diff3 conflictStyle should have made this visible and reliably solvable. So big recommendation to have it enabled [1] I didn't know this for many years and had to often check externally to differentiate the changes from the patch and those from the diverging changes of the codebases. Or make guesses to be quicker and risked adding or deleting wrong stuff.

[1] run `git config --global merge.conflictstyle diff3`

Comment on attachment 129680 [details] [review]
Bug 28735: [19.11] Self-checkout users can access opac-user.pl for sco user when not using AutoSelfCheckID

19.11 version replaced by mistake the master version.

Thanks Victor for your help with this and with explaining the diffs.

Backported to 19.11.x-security branch on the Security repo for 19.11.25 security release.