Defect #16107
closedApplicationController mishandles non-Basic authentication information, causing an internal error
0%
Description
I am currently using a 2.3.1 installation (running on Ruby 1.9.3, with a Postgres database), with the Redmine HTTP Auth plug-in, and a mod_auth_kerb setup based on AD, for authentication.
In some cases, accessing from Internet Explorer provokes the following error log :
ActiveRecord::StatementInvalid (PG::Error: ERROR: invalid byte sequence for encoding "UTF8": 0x82 : SELECT "users".* FROM "users" WHERE "users"."type" IN ('User', 'AnonymousUser') AND "users"."login" = '`<82>^Gj^F^F+^F^A^E^E^B <82>^G^0<82>^GZ 00.^F *<86>H<82>÷^R^A^B^B^F *<86>H<86>÷^R^A^B^B^F +^F^A^D^A<82>7^B^B^^^F +^F^A^D^A<82>7^B^B ¢<82>^G$^D<82>^G `<82>^G^\^F *<86>H<86>÷^R^A^B^B^A'): app/models/user.rb:352:in `find_by_login' app/models/user.rb:163:in `try_to_login' app/controllers/application_controller.rb:113:in `block in find_current_user' app/controllers/application_controller.rb:112:in `find_current_user' app/controllers/application_controller.rb:87:in `user_setup'
I have retraced it succesfully to the find_current_user method in "app/controllers/application_controller.rb" (line 111) :
# HTTP Basic, either username/password or API key/random
authenticate_with_http_basic do |username, password|
user = User.try_to_login(username, password) || User.find_by_api_key(username)
end
The "authenticate_with_http_basic" code is provided by the actionpack gem, which will access the Authorization header and cut it to get what it thinks is the username and the password. The problem is, since I am not using Basic auth, but Negotiate auth, it is cutting through Kerberos ticket information, and therefore that the resulting [ username, password ] values will make no sense whatsoever.
After this, it will try using the username value (a binary blob at this point) to look for the corresponding User object, and it is at this point that the database refuses to process the SQL query (which would fail anyway), resulting in an internal error.
I could fix the code on my side with the following fix to app/models/user.rb :
--- /usr/local/share/redmine/app/models/user.rb.orig 2014-02-17 16:36:21.246082730 +0900
+++ /usr/local/share/redmine/app/models/user.rb 2014-02-17 17:06:49.078946687 +0900
@@ -155,8 +155,12 @@
# Returns the user that matches provided login and password, or nil
def self.try_to_login(login, password)
- login = login.to_s
- password = password.to_s
+### IN-HOUSE PATCH BY STEPHANE LAPIE <stephane.lapie@asahinet.com> ON 2014/02/17
+# This patch here sanitizes the "login" and "password" information.
+# These are derived from parsing the Authorization request header, and they are valid only for Basic authentication.
+# If this info comes from a Negotiate (Kerberos) authentication attempt, they will contain mangled ticket information.
+ login = login.to_s.encode('UTF-8', :invalid => :replace, :undef => :replace)
+ password = password.to_s.encode('UTF-8', :invalid => :replace, :undef => :replace)
# Make sure no one can sign in with an empty login or password
return nil if login.empty? || password.empty?
By sanitizing the variables like this, the SQL query goes through properly, and even though it fails, authentication is then handed over to the HTTP Auth plugin (using the name stored in the REMOTE_USER variable).
However, at the core, it sounds to me there is a problem in making the assumption that any HTTP auth attempt is going to be Basic, both in Redmine, and in Rails. It would probably be more sensible to check if we are using Basic auth (just check the first word of request.authorization), and if we are not, to right away consider we don't have valid user identification info as we would otherwise anyway, which is why I also patched app/controllers/application_controller.rb :
--- /usr/local/share/redmine/app/controllers/application_controller.rb.orig 2014-02-17 18:21:53.497327785 +0900
+++ /usr/local/share/redmine/app/controllers/application_controller.rb 2014-02-17 18:21:32.605134171 +0900
@@ -109,8 +109,12 @@
user = User.find_by_api_key(key)
else
# HTTP Basic, either username/password or API key/random
- authenticate_with_http_basic do |username, password|
- user = User.try_to_login(username, password) || User.find_by_api_key(username)
+ ### IN-HOUSE PATCH BY STEPHANE LAPIE <stephane.lapie@asahinet.com> ON 2014/02/17
+ # This patch ensures we don't try Basic authentication in cases where we use any other mechanism
+ if (request.authorization.split(' ') == "Basic")
+ authenticate_with_http_basic do |username, password|
+ user = User.try_to_login(username, password) || User.find_by_api_key(username)
+ end
end
end
# Switch user if requested by an admin user