SECURITY: Limit passwords to 200 characters

Prevents layer 8 attack.

app/controllers/session_controller.rb

@@ -54,6 +54,8 @@ class SessionController < ApplicationController
     params.require(:login)
     params.require(:password)
 
+    return invalid_credentials if params[:password].length > User.max_password_length
+
     login = params[:login].strip
     login = login[1..-1] if login[0] == "@"
 

app/controllers/users_controller.rb

@@ -151,6 +151,11 @@ class UsersController < ApplicationController
       return
     end
 
+    if params[:password] && params[:password].length > User.max_password_length
+      render json: { success: false, message: I18n.t("login.password_too_long") }
+      return
+    end
+
     user = User.new(user_params)
 
     authentication = UserAuthenticator.new(user, session)
@@ -221,12 +226,17 @@ class UsersController < ApplicationController
     if !@user
       flash[:error] = I18n.t('password_reset.no_token')
     elsif request.put?
-      raise Discourse::InvalidParameters.new(:password) unless params[:password].present?
-      @user.password = params[:password]
-      @user.password_required!
-      if @user.save
-        Invite.invalidate_for_email(@user.email) # invite link can't be used to log in anymore
-        logon_after_password_reset
+      @invalid_password = params[:password].blank? || params[:password].length > User.max_password_length
+
+      if @invalid_password
+        @user.errors.add(:password, :invalid)
+      else
+        @user.password = params[:password]
+        @user.password_required!
+        if @user.save
+          Invite.invalidate_for_email(@user.email) # invite link can't be used to log in anymore
+          logon_after_password_reset
+        end
       end
     end
     render layout: 'no_js'

app/models/user.rb

@@ -111,6 +111,10 @@ class User < ActiveRecord::Base
     LAST_VISIT = -2
   end
 
+  def self.max_password_length
+    200
+  end
+
   def self.username_length
     SiteSetting.min_username_length.to_i..SiteSetting.max_username_length.to_i
   end
@@ -669,6 +673,7 @@ class User < ActiveRecord::Base
   end
 
   def hash_password(password, salt)
+    raise "password is too long" if password.size > User.max_password_length
     Pbkdf2.hash_password(password, salt, Rails.configuration.pbkdf2_iterations, Rails.configuration.pbkdf2_algorithm)
   end
 

app/views/users/password_reset.html.erb

@@ -36,7 +36,8 @@
 
       <%=form_tag({}, method: :put) do %>
         <p>
-        <input id="user_password" name="password" size="30" type="password">
+        <input id="user_password" name="password" size="30" type="password" maxlength="<%= User.max_password_length %>">
+        <label><%= t('js.user.password.instructions', count: SiteSetting.min_password_length) %></label>
         </p>
         <p>
         <%=submit_tag( @user.has_password? ? t('password_reset.update') : t('password_reset.save'), class: 'btn')%>

config/locales/server.en.yml

@@ -1088,6 +1088,7 @@ en:
     omniauth_error: "Sorry, there was an error authorizing your %{strategy} account. Perhaps you did not approve authorization?"
     omniauth_error_unknown: "Something went wrong processing your log in, please try again."
     new_registrations_disabled: "New account registrations are not allowed at this time."
+    password_too_long: "Passwords are limited to 200 characters."
 
   user:
     username:

spec/controllers/session_controller_spec.rb

@@ -186,6 +186,14 @@ describe SessionController do
         end
       end
 
+      describe 'invalid password' do
+        it "should return an error with an invalid password if too long" do
+          User.any_instance.expects(:confirm_password?).never
+          xhr :post, :create, login: user.username, password: ('s' * (User.max_password_length + 1))
+          ::JSON.parse(response.body)['error'].should be_present
+        end
+      end
+
       describe 'suspended user' do
         it 'should return an error' do
           User.any_instance.stubs(:suspended?).returns(true)

spec/controllers/users_controller_spec.rb

@@ -287,14 +287,28 @@ describe UsersController do
         EmailToken.expects(:confirm).with(token).returns(user)
       end
 
+      it "fails when the password is blank" do
+        put :password_reset, token: token, password: ''
+        assigns(:user).errors.should be_present
+        session[:current_user_id].should be_blank
+      end
+
+      it "fails when the password is too long" do
+        put :password_reset, token: token, password: ('x' * (User.max_password_length + 1))
+        assigns(:user).errors.should be_present
+        session[:current_user_id].should be_blank
+      end
+
       it "logs in the user" do
         put :password_reset, token: token, password: 'newpassword'
+        assigns(:user).errors.should be_blank
         session[:current_user_id].should be_present
       end
 
       it "doesn't log in the user when not approved" do
         SiteSetting.expects(:must_approve_users?).returns(true)
         put :password_reset, token: token, password: 'newpassword'
+        assigns(:user).errors.should be_blank
         session[:current_user_id].should be_blank
       end
     end
@@ -508,6 +522,11 @@ describe UsersController do
       include_examples 'failed signup'
     end
 
+    context 'when password is too long' do
+      let(:create_params) { {name: @user.name, username: @user.username, password: "x" * (User.max_password_length + 1), email: @user.email} }
+      include_examples 'failed signup'
+    end
+
     context 'when password param is missing' do
       let(:create_params) { {name: @user.name, username: @user.username, email: @user.email} }
       include_examples 'failed signup'

spec/models/user_spec.rb

@@ -1150,4 +1150,30 @@ describe User do
     end
   end
 
+  describe "hash_passwords" do
+
+    let(:too_long) { "x" * (User.max_password_length + 1) }
+
+    def hash(password, salt)
+      User.new.send(:hash_password, password, salt)
+    end
+
+    it "returns the same hash for the same password and salt" do
+      hash('poutine', 'gravy').should == hash('poutine', 'gravy')
+    end
+
+    it "returns a different hash for the same salt and different password" do
+      hash('poutine', 'gravy').should_not == hash('fries', 'gravy')
+    end
+
+    it "returns a different hash for the same password and different salt" do
+      hash('poutine', 'gravy').should_not == hash('poutine', 'cheese')
+    end
+
+    it "raises an error when passwords are too long" do
+      -> { hash(too_long, 'gravy') }.should raise_error
+    end
+
+  end
+
 end