Bug 23042 - Local login attempt populates shibboleth url with userid and password in plain text
Summary: Local login attempt populates shibboleth url with userid and password in plai...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 24779 24199
  Show dependency treegraph
 
Reported: 2019-06-03 19:22 UTC by Nick Clemens
Modified: 2021-06-14 21:28 UTC (History)
12 users (show)

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


Attachments
Bug 23042: Only include GET params in return URL for Shibboleth (2.43 KB, patch)
2019-06-13 16:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23042: Only include GET params in return URL for Shibboleth (2.49 KB, patch)
2019-06-15 04:06 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 23042: Add tests to catch POST params in return URL (2.56 KB, patch)
2019-06-19 09:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23042: Only include GET params in return URL for Shibboleth (2.42 KB, patch)
2019-06-19 09:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23042: Correct shib param escaping (4.71 KB, patch)
2019-06-19 17:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23042: Correct shib param escaping (4.71 KB, patch)
2019-06-19 18:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23042: Add tests to catch POST params in return URL (2.63 KB, patch)
2019-06-21 15:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23042: Only include GET params in return URL for Shibboleth (2.49 KB, patch)
2019-06-21 15:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23042: Correct shib param escaping (4.78 KB, patch)
2019-06-21 15:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23042: Add tests to catch POST params in return URL (2.66 KB, patch)
2019-10-18 12:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23042: Only include GET params in return URL for Shibboleth (2.52 KB, patch)
2019-10-18 12:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23042: Correct shib param escaping (4.81 KB, patch)
2019-10-18 12:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23042: Add tests to catch POST params in return URL (2.71 KB, patch)
2019-10-21 10:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23042: Only include GET params in return URL for Shibboleth (2.57 KB, patch)
2019-10-21 10:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23042: Correct shib param escaping (4.86 KB, patch)
2019-10-21 10:49 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 Nick Clemens 2019-06-03 19:22:11 UTC
To recreate:
1 - Enable shibboleth in your koha-conf.xml by setting a section like:
 <useshibboleth>1</useshibboleth><!-- see C4::Auth_with_shibboleth for extra configs you must do to turn this on -->
 <shibboleth>
     <matchpoint>userid</matchpoint>
     <mapping>
         <userid is="uid"></userid>
     </mapping>
 </shibboleth>
2 - Go to the opac
3 - Enter a bad username and password "please/dontwork"
4 - Login fails, but hovering over the shibboleth link shows the userid and password entered
5 - This is problematic in the case where a user attempts to use their shibboelth credentials directly in koha, and just in general
Comment 1 Martin Renvoize 2019-06-13 16:05:22 UTC
Created attachment 90579 [details] [review]
Bug 23042: Only include GET params in return URL for Shibboleth

The shibboleth return target included POST parameters in the URL string,
this meant that a failed local login POST would include the username and
password used in the attemtped login in plaintext in the redirect URL
that is appended to the shibboleth login URL.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 2 Martin Renvoize 2019-06-14 07:03:03 UTC
To note, you don't actually need shibboleth setup in full to test this.. just the Koha side configuration settings as per Nicks instructions.
Comment 3 Mark Tompsett 2019-06-15 04:06:40 UTC
Created attachment 90636 [details] [review]
Bug 23042: Only include GET params in return URL for Shibboleth

The shibboleth return target included POST parameters in the URL string,
this meant that a failed local login POST would include the username and
password used in the attemtped login in plaintext in the redirect URL
that is appended to the shibboleth login URL.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 4 Mark Tompsett 2019-06-15 04:14:52 UTC
(In reply to Martin Renvoize from comment #2)
> To note, you don't actually need shibboleth setup in full to test this..
> just the Koha side configuration settings as per Nicks instructions.

I was going to ask how, but then I just did a regular log in, and inspected the Shibboleth URL... YIKES! This fixes it.
Comment 5 Jonathan Druart 2019-06-19 00:49:13 UTC
Martin, I think we should start writing tests for login_shib_url. I wanted to rewrite _get_return to use a join instead and make it more readable.
Comment 6 Martin Renvoize 2019-06-19 09:02:16 UTC
You are totally right, I forgot to add to the existing tests in t/Auth_with_shibboleth.t
Comment 7 Martin Renvoize 2019-06-19 09:58:17 UTC
Created attachment 90772 [details] [review]
Bug 23042: Add tests to catch POST params in return URL
Comment 8 Martin Renvoize 2019-06-19 09:58:20 UTC
Created attachment 90773 [details] [review]
Bug 23042: Only include GET params in return URL for Shibboleth

The shibboleth return target included POST parameters in the URL string,
this meant that a failed local login POST would include the username and
password used in the attemtped login in plaintext in the redirect URL
that is appended to the shibboleth login URL.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 9 Martin Renvoize 2019-06-19 09:59:23 UTC
Tests added as requested, good catch Jonathan.
Comment 10 Jonathan Druart 2019-06-19 16:55:51 UTC
Hit: http://catalogue.kohadev.org/cgi-bin/koha/opac-user.pl?param1=heh❤

=> boom
Comment 11 Jonathan Druart 2019-06-19 17:06:06 UTC
Also it's not correct, & must be URI escaped as well, otherwise the parameters will not considered as part of the target variable.
Comment 12 Jonathan Druart 2019-06-19 17:13:49 UTC
Created attachment 90811 [details] [review]
Bug 23042: Correct shib param escaping
Comment 13 Jonathan Druart 2019-06-19 17:14:14 UTC
I think this is the way to go.
Comment 14 Mark Tompsett 2019-06-19 17:36:42 UTC
Comment on attachment 90811 [details] [review]
Bug 23042: Correct shib param escaping

Review of attachment 90811 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/opac-tmpl/bootstrap/en/includes/masthead.inc
@@ +318,4 @@
>                          </div>
>                      [% ELSE %]
>                          <h4>Shibboleth login</h4>
> +                        <p>If you have a Shibboleth account, please <a href="[% shibbolethLoginUrl | raw %]">click here to login</a>.</p>

$raw, not raw.
Comment 15 Martin Renvoize 2019-06-19 18:04:56 UTC
Perhaps a followup bug, but we should probably unify the routine that gives us a return uri as it's now nearly the same in at least two places: https://gitlab.com/koha-community/Koha/blob/master/C4/Auth_with_cas.pm#L209
Comment 16 Jonathan Druart 2019-06-19 18:08:22 UTC
Created attachment 90814 [details] [review]
Bug 23042: Correct shib param escaping
Comment 17 Jonathan Druart 2019-06-19 18:12:01 UTC
(In reply to Martin Renvoize from comment #15)
> Perhaps a followup bug, but we should probably unify the routine that gives
> us a return uri as it's now nearly the same in at least two places:
> https://gitlab.com/koha-community/Koha/blob/master/C4/Auth_with_cas.pm#L209

Erk, did not see that. So yes seems that it must be fixed on its own bug report.
Comment 18 Jonathan Druart 2019-06-20 00:38:33 UTC
(In reply to Jonathan Druart from comment #17)
> (In reply to Martin Renvoize from comment #15)
> > Perhaps a followup bug, but we should probably unify the routine that gives
> > us a return uri as it's now nearly the same in at least two places:
> > https://gitlab.com/koha-community/Koha/blob/master/C4/Auth_with_cas.pm#L209
> 
> Erk, did not see that. So yes seems that it must be fixed on its own bug
> report.

On the other hand these patches are copy-pasting existing code, which is bad.

Martin, what do you think is best?
Comment 19 Martin Renvoize 2019-06-20 07:27:11 UTC
Well, the Shibboleth side is now pretty well tested, but I don't think the CAS side is yet.. I'd be wary of pulling it out here (as it's not exactly the same code as it stands, just very similar).

I feel we should fix the security issue here and followup with an Architecture bug soon after where we have less of a time constriction and can pull matts in to verify the tests that would be needed for the CAS side.
Comment 20 Kyle M Hall 2019-06-21 15:23:31 UTC
Created attachment 90911 [details] [review]
Bug 23042: Add tests to catch POST params in return URL

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Kyle M Hall 2019-06-21 15:24:05 UTC
Created attachment 90912 [details] [review]
Bug 23042: Only include GET params in return URL for Shibboleth

The shibboleth return target included POST parameters in the URL string,
this meant that a failed local login POST would include the username and
password used in the attemtped login in plaintext in the redirect URL
that is appended to the shibboleth login URL.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Kyle M Hall 2019-06-21 15:24:08 UTC
Created attachment 90913 [details] [review]
Bug 23042: Correct shib param escaping

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Marcel de Rooy 2019-07-05 07:16:08 UTC
+    foreach my $param ( $query->url_param() ) {
+        # url_param() always returns parameters that were deleted by delete()
+        # This additional check ensure that parameter was not deleted.
+        my $uriPiece = $query->param($param);
+        if ($uriPiece) {
+            $uri_params_part .= '&' if $uri_params_part;
+            $uri_params_part .= $param . '=';
+            $uri_params_part .= URI::Escape::uri_escape( $uriPiece );
+        }
+    }

I am not sure about the magical mix of url_param and param in this part of the code. "If you try to mix a URL query string with a form submitted with the GET method, the results will not be what you expect." ?
Btw where are these CGI parameters deleted?
Comment 24 Katrin Fischer 2019-07-09 06:11:48 UTC
(In reply to Marcel de Rooy from comment #23)
> +    foreach my $param ( $query->url_param() ) {
> +        # url_param() always returns parameters that were deleted by
> delete()
> +        # This additional check ensure that parameter was not deleted.
> +        my $uriPiece = $query->param($param);
> +        if ($uriPiece) {
> +            $uri_params_part .= '&' if $uri_params_part;
> +            $uri_params_part .= $param . '=';
> +            $uri_params_part .= URI::Escape::uri_escape( $uriPiece );
> +        }
> +    }
> 
> I am not sure about the magical mix of url_param and param in this part of
> the code. "If you try to mix a URL query string with a form submitted with
> the GET method, the results will not be what you expect." ?
> Btw where are these CGI parameters deleted?

Martin, can you comment on this please?
Comment 25 Matthias Meusburger 2019-07-25 14:05:50 UTC
I can confirm the problem on master, and I can confirm that the patch fixes it.

I can login to the Opac using Shibboleth, both with and without the patch.

However, I compared the Shibboleth links URLS without and with the patch, from an opac search:

Starting from:
/cgi-bin/koha/opac-search.pl?idx=&q=a

Without the patch, I have:
/Shibboleth.sso/Login?target=https://catalogue.koha.local/opac/opac-search.pl%253Fidx=;q=a (incorrect URL encoding)

With the patch, I have:
/Shibboleth.sso/Login?target=https://catalogue.koha.local/opac/opac-search.pl%3Fq%3Da (correct URL encoding)

Which shows 2 things:
 - We have an URLencoding issue on master, just like we had for CAS (see Bug 22585).
 - The empty parameters (idx in this case) are now removed.

The login results are the following:

Without the patch, I end up on a 404: /cgi-bin/koha/opac-search.pl%3Fidx=;q=a (seems one URL decoding was done, but the ? was double encoded at first)

With the patch, I also end up on a 404: /cgi-bin/koha/opac-search.pl%3Fq%3Da (seems not URL decoding was done at all)

I also want to mention that removing empty parameters is fine with Shibboleth, but will not be with CAS, in case, as you mentioned, we want to refactor Shib and CAS return URL code.
Comment 26 Katrin Fischer 2019-08-15 06:05:56 UTC
Should this be Failed QA?
Comment 27 Marcel de Rooy 2019-08-16 06:53:03 UTC
(In reply to Katrin Fischer from comment #26)
> Should this be Failed QA?

Well, at least we need a response on comments 23 and 25.
Changing status to reflect need for feedback ;)
Comment 28 Martin Renvoize 2019-09-09 11:39:47 UTC
(In reply to Marcel de Rooy from comment #23)
> +    foreach my $param ( $query->url_param() ) {
> +        # url_param() always returns parameters that were deleted by
> delete()
> +        # This additional check ensure that parameter was not deleted.
> +        my $uriPiece = $query->param($param);
> +        if ($uriPiece) {
> +            $uri_params_part .= '&' if $uri_params_part;
> +            $uri_params_part .= $param . '=';
> +            $uri_params_part .= URI::Escape::uri_escape( $uriPiece );
> +        }
> +    }
> 
> I am not sure about the magical mix of url_param and param in this part of
> the code. "If you try to mix a URL query string with a form submitted with
> the GET method, the results will not be what you expect." ?
> Btw where are these CGI parameters deleted?

This is taken directly from the CAS implementation of the same... I'm not sure I can articulate it any better :(
Comment 29 Martin Renvoize 2019-09-09 11:49:09 UTC
(In reply to Matthias Meusburger from comment #25)
> With the patch, I also end up on a 404: /cgi-bin/koha/opac-search.pl%3Fq%3Da
> (seems not URL decoding was done at all)
> 

Hmm, is this working against a particular Shib SSO provider?.. it feels like the URI decoding should have been done at their end on redirect back rather than us doing any further decoding at our end.. or am I not understanding what you're highlighting here?
Comment 30 Matthias Meusburger 2019-09-10 10:02:02 UTC
Fair point. 

I tried to search in the shibboleth documentation whether the decoding should be done on the SP (Koha) or on the IdP (shib server) side but didn't find anything so far. If someone manage to find some documentation about this, I'd be happy to read it.

However, what I can say, is that we used to decode the parameters:

Testing against the same IdP:

18.05.14 => parameters are decoded
18.11.09 => parameters aren't decoded anymore

At some point we modified this behavior, and I'm not sure it was on purpose.

That being said, the lack of decoding precedes this patch, so I think this problem can be addressed in another bug.
Comment 31 Matthias Meusburger 2019-09-10 10:17:42 UTC
(See Bug 23526)
Comment 32 Matthias Meusburger 2019-09-23 15:08:22 UTC
My first analysis on the encoding problem was erroneous, it was a simple double-encoding problem, as stated in Bug 23526.

That being said, I can confirm that the return url is now correct when using this patch in conjunction with the patch provided in Bug 23526.
Comment 33 Katrin Fischer 2019-10-03 11:19:54 UTC
Matts, is this a sign-off? Because then we could count Kyle as QA ;)
Comment 34 Matthias Meusburger 2019-10-03 11:35:11 UTC
This is indeed a Sign-Off. Comment #23 should be addressed by QA.
Comment 35 Matthias Meusburger 2019-10-03 11:41:10 UTC
Patch should be rebased though.
Comment 36 Katrin Fischer 2019-10-10 06:04:55 UTC
Please rebase the patch and answer the question in comment#23
I think we are almost there with Kyle and Matts sign-offs.
Comment 37 Jonathan Druart 2019-10-15 10:07:18 UTC
(In reply to Marcel de Rooy from comment #23)
> +    foreach my $param ( $query->url_param() ) {
> +        # url_param() always returns parameters that were deleted by
> delete()
> +        # This additional check ensure that parameter was not deleted.
> +        my $uriPiece = $query->param($param);
> +        if ($uriPiece) {
> +            $uri_params_part .= '&' if $uri_params_part;
> +            $uri_params_part .= $param . '=';
> +            $uri_params_part .= URI::Escape::uri_escape( $uriPiece );
> +        }
> +    }
> 
> I am not sure about the magical mix of url_param and param in this part of
> the code. 

Where do you think the magic is? We are processing the different params contained in the url and concat them to generate uri_params_part.

> "If you try to mix a URL query string with a form submitted with
> the GET method, the results will not be what you expect." ?

Can you detail why you are quoting that?

> Btw where are these CGI parameters deleted?

It is "in case they are deleted". At least it is how I understand the comment.
Comment 38 Marcel de Rooy 2019-10-18 12:27:33 UTC
Created attachment 94404 [details] [review]
Bug 23042: Add tests to catch POST params in return URL

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 39 Marcel de Rooy 2019-10-18 12:27:37 UTC
Created attachment 94405 [details] [review]
Bug 23042: Only include GET params in return URL for Shibboleth

The shibboleth return target included POST parameters in the URL string,
this meant that a failed local login POST would include the username and
password used in the attemtped login in plaintext in the redirect URL
that is appended to the shibboleth login URL.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 40 Marcel de Rooy 2019-10-18 12:27:41 UTC
Created attachment 94406 [details] [review]
Bug 23042: Correct shib param escaping

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 41 Marcel de Rooy 2019-10-18 12:28:27 UTC
Rebased, still returning to earlier comments for QA
Comment 42 Katrin Fischer 2019-10-21 06:24:26 UTC
Hi Marcel, what's missing here? comment#37?
Comment 43 Marcel de Rooy 2019-10-21 06:28:10 UTC
(In reply to Katrin Fischer from comment #42)
> Hi Marcel, what's missing here? comment#37?

Yes I am still coming back here
Comment 44 Marcel de Rooy 2019-10-21 10:48:53 UTC
Created attachment 94431 [details] [review]
Bug 23042: Add tests to catch POST params in return URL

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 45 Marcel de Rooy 2019-10-21 10:48:58 UTC
Created attachment 94432 [details] [review]
Bug 23042: Only include GET params in return URL for Shibboleth

The shibboleth return target included POST parameters in the URL string,
this meant that a failed local login POST would include the username and
password used in the attemtped login in plaintext in the redirect URL
that is appended to the shibboleth login URL.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 46 Marcel de Rooy 2019-10-21 10:49:03 UTC
Created attachment 94433 [details] [review]
Bug 23042: Correct shib param escaping

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 47 Marcel de Rooy 2019-10-21 10:53:34 UTC
(In reply to Martin Renvoize from comment #28)
> (In reply to Marcel de Rooy from comment #23)
> This is taken directly from the CAS implementation of the same... I'm not
> sure I can articulate it any better :(

(In reply to Jonathan Druart from comment #37)
> (In reply to Marcel de Rooy from comment #23)
> Where do you think the magic is? We are processing the different params
> contained in the url and concat them to generate uri_params_part.

I think we should better document for both CAS and Shibboleth which parameters are expected via GET or POST.
Comment 48 Martin Renvoize 2019-11-22 10:47:56 UTC
Pushed to 19.05.x for 19.05.06
Comment 49 Lucas Gass 2019-11-22 19:03:37 UTC
this is applied to the 18.11.x security branch for 18.11.11
Comment 50 Fridolin Somers 2019-11-26 08:41:16 UTC
(In reply to Martin Renvoize from comment #48)
> Pushed to 19.05.x for 19.05.06

Its 19.05.05
Comment 51 Martin Renvoize 2019-11-27 10:33:33 UTC
Nice work!

Pushed to master for 19.11.00