From d5d8db7fa82d314adcd064882a57a48b3eb088e2 Mon Sep 17 00:00:00 2001 From: Sam Saffron Date: Wed, 16 Oct 2019 16:53:31 +1100 Subject: [PATCH] FEATURE: improve honeypot and challenge logic This feature amends it so instead of using one challenge and honeypot statically per site we have a rotating honeypot and challenge value which changes every hour. This means you must grab a fresh copy of honeypot and challenge value once an hour or account registration will be rejected. We also now cycle the value of the challenge when after successful account registration forcing an extra call to hp.json between account registrations Client has been made aware of these changes. Additionally this contains a JavaScript workaround for: https://bugs.chromium.org/p/chromium/issues/detail?id=987293 This is client side code that is specific to Chrome user agent and swaps a PASSWORD type honeypot with a TEXT type honeypot. --- .../controllers/create-account.js.es6 | 190 +++++++++++------- .../templates/modal/create-account.hbs | 2 +- app/controllers/users_controller.rb | 30 ++- lib/secure_session.rb | 27 ++- spec/requests/users_controller_spec.rb | 47 ++++- 5 files changed, 198 insertions(+), 98 deletions(-) diff --git a/app/assets/javascripts/discourse/controllers/create-account.js.es6 b/app/assets/javascripts/discourse/controllers/create-account.js.es6 index 1c8ecb254a1..0db34a4e949 100644 --- a/app/assets/javascripts/discourse/controllers/create-account.js.es6 +++ b/app/assets/javascripts/discourse/controllers/create-account.js.es6 @@ -24,7 +24,6 @@ export default Ember.Controller.extend( login: Ember.inject.controller(), complete: false, - accountPasswordConfirm: 0, accountChallenge: 0, formSubmitted: false, rejectedEmails: Ember.A([]), @@ -191,8 +190,36 @@ export default Ember.Controller.extend( @on("init") fetchConfirmationValue() { return ajax(userPath("hp.json")).then(json => { + this._challengeDate = new Date(); + // remove 30 seconds for jitter, make sure this works for at least + // 30 seconds so we don't have hard loops + this._challengeExpiry = parseInt(json.expires_in, 10) - 30; + if (this._challengeExpiry < 30) { + this._challengeExpiry = 30; + } + + const confirmation = document.getElementById( + "new-account-confirmation" + ); + if (confirmation) { + confirmation.value = json.value; + } + + // Chrome autocomplete is buggy per: + // https://bugs.chromium.org/p/chromium/issues/detail?id=987293 + // work around issue while leaving a semi useable honeypot for + // bots that are running full Chrome + if (confirmation && navigator.userAgent.indexOf("Chrome") > 0) { + const newConfirmation = document.createElement("input"); + + newConfirmation.type = "text"; + newConfirmation.id = "new-account-confirmation"; + newConfirmation.value = json.value; + + confirmation.parentNode.replaceChild(newConfirmation, confirmation); + } + this.setProperties({ - accountPasswordConfirm: json.value, accountChallenge: json.challenge .split("") .reverse() @@ -201,85 +228,102 @@ export default Ember.Controller.extend( }); }, + performAccountCreation() { + const attrs = this.getProperties( + "accountName", + "accountEmail", + "accountPassword", + "accountUsername", + "accountChallenge" + ); + + attrs["accountPasswordConfirm"] = document.getElementById( + "new-account-confirmation" + ).value; + + const userFields = this.userFields; + const destinationUrl = this.get("authOptions.destination_url"); + + if (!Ember.isEmpty(destinationUrl)) { + $.cookie("destination_url", destinationUrl, { path: "/" }); + } + + // Add the userfields to the data + if (!Ember.isEmpty(userFields)) { + attrs.userFields = {}; + userFields.forEach( + f => (attrs.userFields[f.get("field.id")] = f.get("value")) + ); + } + + this.set("formSubmitted", true); + return Discourse.User.createAccount(attrs).then( + result => { + this.set("isDeveloper", false); + if (result.success) { + // invalidate honeypot + this._challengeExpiry = 1; + + // Trigger the browser's password manager using the hidden static login form: + const $hidden_login_form = $("#hidden-login-form"); + $hidden_login_form + .find("input[name=username]") + .val(attrs.accountUsername); + $hidden_login_form + .find("input[name=password]") + .val(attrs.accountPassword); + $hidden_login_form + .find("input[name=redirect]") + .val(userPath("account-created")); + $hidden_login_form.submit(); + } else { + this.flash( + result.message || I18n.t("create_account.failed"), + "error" + ); + if (result.is_developer) { + this.set("isDeveloper", true); + } + if ( + result.errors && + result.errors.email && + result.errors.email.length > 0 && + result.values + ) { + this.rejectedEmails.pushObject(result.values.email); + } + if ( + result.errors && + result.errors.password && + result.errors.password.length > 0 + ) { + this.rejectedPasswords.pushObject(attrs.accountPassword); + } + this.set("formSubmitted", false); + $.removeCookie("destination_url"); + } + }, + () => { + this.set("formSubmitted", false); + $.removeCookie("destination_url"); + return this.flash(I18n.t("create_account.failed"), "error"); + } + ); + }, + actions: { externalLogin(provider) { this.login.send("externalLogin", provider); }, createAccount() { - const attrs = this.getProperties( - "accountName", - "accountEmail", - "accountPassword", - "accountUsername", - "accountPasswordConfirm", - "accountChallenge" - ); - const userFields = this.userFields; - const destinationUrl = this.get("authOptions.destination_url"); - - if (!Ember.isEmpty(destinationUrl)) { - $.cookie("destination_url", destinationUrl, { path: "/" }); - } - - // Add the userfields to the data - if (!Ember.isEmpty(userFields)) { - attrs.userFields = {}; - userFields.forEach( - f => (attrs.userFields[f.get("field.id")] = f.get("value")) + if (new Date() - this._challengeDate > 1000 * this._challengeExpiry) { + this.fetchConfirmationValue().then(() => + this.performAccountCreation() ); + } else { + this.performAccountCreation(); } - - this.set("formSubmitted", true); - return Discourse.User.createAccount(attrs).then( - result => { - this.set("isDeveloper", false); - if (result.success) { - // Trigger the browser's password manager using the hidden static login form: - const $hidden_login_form = $("#hidden-login-form"); - $hidden_login_form - .find("input[name=username]") - .val(attrs.accountUsername); - $hidden_login_form - .find("input[name=password]") - .val(attrs.accountPassword); - $hidden_login_form - .find("input[name=redirect]") - .val(userPath("account-created")); - $hidden_login_form.submit(); - } else { - this.flash( - result.message || I18n.t("create_account.failed"), - "error" - ); - if (result.is_developer) { - this.set("isDeveloper", true); - } - if ( - result.errors && - result.errors.email && - result.errors.email.length > 0 && - result.values - ) { - this.rejectedEmails.pushObject(result.values.email); - } - if ( - result.errors && - result.errors.password && - result.errors.password.length > 0 - ) { - this.rejectedPasswords.pushObject(attrs.accountPassword); - } - this.set("formSubmitted", false); - $.removeCookie("destination_url"); - } - }, - () => { - this.set("formSubmitted", false); - $.removeCookie("destination_url"); - return this.flash(I18n.t("create_account.failed"), "error"); - } - ); } } } diff --git a/app/assets/javascripts/discourse/templates/modal/create-account.hbs b/app/assets/javascripts/discourse/templates/modal/create-account.hbs index 4615fb74bf4..ad5dacdfc82 100644 --- a/app/assets/javascripts/discourse/templates/modal/create-account.hbs +++ b/app/assets/javascripts/discourse/templates/modal/create-account.hbs @@ -83,7 +83,7 @@ - {{input type="password" value=accountPasswordConfirm id="new-account-confirmation" autocomplete="new-password"}} + {{input value=accountChallenge id="new-account-challenge"}} diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 5ffc97b7121..a696aa4ab12 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -414,6 +414,9 @@ class UsersController < ApplicationController authentication.finish activation.finish + secure_session[HONEYPOT_KEY] = nil + secure_session[CHALLENGE_KEY] = nil + # save user email in session, to show on account-created page session["user_created_message"] = activation.message session[SessionController::ACTIVATE_USER_KEY] = user.id @@ -467,7 +470,14 @@ class UsersController < ApplicationController end def get_honeypot_value - render json: { value: honeypot_value, challenge: challenge_value } + secure_session.set(HONEYPOT_KEY, honeypot_value, expires: 1.hour) + secure_session.set(CHALLENGE_KEY, challenge_value, expires: 1.hour) + + render json: { + value: honeypot_value, + challenge: challenge_value, + expires_in: SecureSession.expiry + } end def password_reset @@ -660,7 +670,6 @@ class UsersController < ApplicationController security_keys_enabled = email_token_user&.security_keys_enabled? second_factor_token = params[:second_factor_token] second_factor_method = params[:second_factor_method].to_i - security_key_credential = params[:security_key_credential] confirm_email = false @security_key_required = security_keys_enabled @@ -1368,22 +1377,21 @@ class UsersController < ApplicationController render json: success_json end - private + HONEYPOT_KEY ||= 'HONEYPOT_KEY' + CHALLENGE_KEY ||= 'CHALLENGE_KEY' + + protected def honeypot_value - Digest::SHA1::hexdigest("#{Discourse.current_hostname}:#{GlobalSetting.safe_secret_key_base}")[0, 15] + secure_session[HONEYPOT_KEY] ||= SecureRandom.hex end def challenge_value - challenge = $redis.get('SECRET_CHALLENGE') - unless challenge && challenge.length == 16 * 2 - challenge = SecureRandom.hex(16) - $redis.set('SECRET_CHALLENGE', challenge) - end - - challenge + secure_session[CHALLENGE_KEY] ||= SecureRandom.hex end + private + def respond_to_suspicious_request if suspicious?(params) render json: { diff --git a/lib/secure_session.rb b/lib/secure_session.rb index bced4807d90..6b8efe57c1e 100644 --- a/lib/secure_session.rb +++ b/lib/secure_session.rb @@ -6,15 +6,36 @@ class SecureSession @prefix = prefix end + def self.expiry + @expiry ||= 1.hour.to_i + end + + def self.expiry=(val) + @expiry = val + end + + def set(key, val, expires: nil) + expires ||= SecureSession.expiry + $redis.setex(prefixed_key(key), SecureSession.expiry.to_i, val.to_s) + true + end + def [](key) - $redis.get("#{@prefix}#{key}") + $redis.get(prefixed_key(key)) end def []=(key, val) if val == nil - $redis.del("#{@prefix}#{key}") + $redis.del(prefixed_key(key)) else - $redis.setex("#{@prefix}#{key}", 1.hour, val.to_s) + $redis.setex(prefixed_key(key), SecureSession.expiry.to_i, val.to_s) end + val + end + + private + + def prefixed_key(key) + "#{@prefix}#{key}" end end diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 26a952fca5d..40bc5322eb5 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -5,6 +5,41 @@ require 'rails_helper' describe UsersController do let(:user) { Fabricate(:user) } + describe "#full account registration flow" do + it "will correctly handle honeypot and challenge" do + + get '/u/hp.json' + expect(response.status).to eq(200) + + json = JSON.parse(response.body) + + params = { + email: 'jane@jane.com', + name: 'jane', + username: 'jane', + password_confirmation: json['value'], + challenge: json['challenge'].reverse, + password: SecureRandom.hex + } + + secure_session = SecureSession.new(session["secure_session_id"]) + + expect(secure_session[UsersController::HONEYPOT_KEY]).to eq(json["value"]) + expect(secure_session[UsersController::CHALLENGE_KEY]).to eq(json["challenge"]) + + post '/u.json', params: params + + expect(response.status).to eq(200) + + jane = User.find_by(username: 'jane') + + expect(jane.email).to eq('jane@jane.com') + + expect(secure_session[UsersController::HONEYPOT_KEY]).to eq(nil) + expect(secure_session[UsersController::CHALLENGE_KEY]).to eq(nil) + end + end + describe '#perform_account_activation' do let(:token) do return @token if @token.present? @@ -1020,22 +1055,15 @@ describe UsersController do shared_examples 'honeypot fails' do it 'should not create a new user' do + User.any_instance.expects(:enqueue_welcome_message).never + expect { post "/u.json", params: create_params }.to_not change { User.count } - expect(response.status).to eq(200) - end - it 'should not send an email' do - User.any_instance.expects(:enqueue_welcome_message).never - post "/u.json", params: create_params expect(response.status).to eq(200) - end - it 'should say it was successful' do - post "/u.json", params: create_params json = JSON::parse(response.body) - expect(response.status).to eq(200) expect(json["success"]).to eq(true) # should not change the session @@ -3361,7 +3389,6 @@ describe UsersController do end it 'succeeds on correct password' do - session = {} ApplicationController.any_instance.stubs(:secure_session).returns("confirmed-password-#{user.id}" => "true") post "/users/create_second_factor_totp.json"