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
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>
To note, you don't actually need shibboleth setup in full to test this.. just the Koha side configuration settings as per Nicks instructions.
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>
(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.
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.
You are totally right, I forgot to add to the existing tests in t/Auth_with_shibboleth.t
Created attachment 90772 [details] [review] Bug 23042: Add tests to catch POST params in return URL
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>
Tests added as requested, good catch Jonathan.
Hit: http://catalogue.kohadev.org/cgi-bin/koha/opac-user.pl?param1=heh❤ => boom
Also it's not correct, & must be URI escaped as well, otherwise the parameters will not considered as part of the target variable.
Created attachment 90811 [details] [review] Bug 23042: Correct shib param escaping
I think this is the way to go.
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.
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
Created attachment 90814 [details] [review] Bug 23042: Correct shib param escaping
(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.
(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?
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.
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>
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>
Created attachment 90913 [details] [review] Bug 23042: Correct shib param escaping Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
+ 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?
(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?
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.
Should this be Failed QA?
(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 ;)
(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 :(
(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?
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.
(See Bug 23526)
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.
Matts, is this a sign-off? Because then we could count Kyle as QA ;)
This is indeed a Sign-Off. Comment #23 should be addressed by QA.
Patch should be rebased though.
Please rebase the patch and answer the question in comment#23 I think we are almost there with Kyle and Matts sign-offs.
(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.
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>
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>
Created attachment 94406 [details] [review] Bug 23042: Correct shib param escaping Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Rebased, still returning to earlier comments for QA
Hi Marcel, what's missing here? comment#37?
(In reply to Katrin Fischer from comment #42) > Hi Marcel, what's missing here? comment#37? Yes I am still coming back here
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>
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>
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>
(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.
Pushed to 19.05.x for 19.05.06
this is applied to the 18.11.x security branch for 18.11.11
(In reply to Martin Renvoize from comment #48) > Pushed to 19.05.x for 19.05.06 Its 19.05.05
Nice work! Pushed to master for 19.11.00