Session Logic for User Verification in Rails Migration

Question

This is a logic to create sessions on RoR 7.1.2 based on the last version of the website which was in vanilla PHP, with the upgrade I have to deal with the users that were already signed up but not have a password_digest attribute on his database just a password hashed by PHP which RoR with the gem Bcrypt cant understand, so with this logic it can access no matter were old or new users.

class SessionsController < ApplicationController
  def create
    @user = find_user_by(params[:user][:username])

    if @user&.authenticate(params[:user][:password])
      success_login
    elsif php_authenticate?(params[:user][:password])
      success_login
    else
      display_error('Invalid user or password')
    end
  end
  
  private

  def find_user_by(username)
    user = User.find_by(username: username)
    return user if user

    user = User.find_by(email: username)
    return user if user

    nil
  end

  def php_authenticate?(password)
    @user&.password_php && php_auth(password)
  end

  def php_auth(password)
    php_file = 'app/controllers/helpers/auth_password.php'
    output = `php #{php_file} '#{password}' '#{@user&.password_php}'`

    output == 'true'
  end

  def display_error(error)
    flash[:error] = error
    redirect_to login_path
  end

  def success_login
    session[:user_id] = @user.id
    redirect_to root_path
  end
end

auth_password.php:

$password = $argv[1];
$hased_password = $argv[2];

if (password_verify($password, $hased_password)) {
  echo 'true';
} else {
  echo 'false';
}

Answer 1

This mostly looks good and sensible, but it does raise some serious concerns.

naming

Using a php_ prefix is accurate, and perhaps a good name. But consider using a legacy_ or v1_ prefix instead. Then we'll be in good shape if PHP rears its undying head again and muscles its way into the codebase in some different form.

hash pattern

    if @user&.authenticate(params[:user][:password])
      success_login
    elsif php_authenticate?(params[:user][:password])
      success_login

We're doing if A --> success elsif B --> success, which is more succinctly phrased as
if A || B --> success.

But this is security critical code, and we're expanding the attack surface to be larger than necessary. Can't we examine some prefix of the hash, or test a regex, and dispatch on that to the appropriate authentication routine?

For example, if you look at a FreeBSD or Linux /etc/passwd or /etc/shadow file, there may be half a dozen back-compatible hashing schemes supported. Does the OS attempt if A or B or C or D or ... ? No, it dispatches based on the hash scheme version, which was carefully encoded to support such dispatching.

collapsed namespaces

  def find_user_by(username)
    user = User.find_by(username: username)
    return user if user

    user = User.find_by(email: username)
    return user if user

Usernames are drawn from one namespace; roughly they match /^\w+$/. Email addresses drawn from another, and we definitely expect they shall contain an @ at-sign, e.g. /^[\w.-]+@[\w.-]+$/. (Yeah, yeah, I know, localpart can contain more valid characters, and with IDN so can globalpart.)

I would prefer to only probe by email address for inputs that are known to contain an @ at-sign.

More generally, there's a regex that describes valid username spellings -- only probe the database upon a match. And similarly for valid email spellings.

shell escapes

This is the one that makes me, and little Bobby Tables, very nervous:

  def php_auth(password)
    ...
    output = `php #{php_file} '#{password}' '#{@user&.password_php}'`

There's two different interpreters involved: bash + php. Each is powerful. Each does their own interpretation of argument characters. There is room to lose.

Now, if we just ask for ls output, there's no shell interpreter involved, as ruby does fork / exec of an ls child. But if the backtick command can contain e.g. a | pipe symbol, then ruby definitely delegates to the shell, which in turn delegates to spawned children.

You should have a regex that describes a valid password hash, e.g. /^[0-9a-f]+$/. Only pass that password argument to a child if it matches the regex, that is, only if it could possibly be valid.

Php accepts a wide variety of command line options, which can potentially do interesting things for an attacker. I don't want to know about them. I don't want to have to reason about the properties of the attack surface the current proposed code opens up for an attacker. Much better to be conservative in what you send down to the child process.

specification

The OP question didn't really flesh out the input space in ways that would let an implementer distinguish between good and bad scenarios.

automated tests

This submission contains no integration or unit tests that would increase our confidence in the ability of target code to reject carefully crafted attack inputs.

---

This code fails to accomplish its stated goals, which is to securely verify client credentials in the presence of attacker clients.

I would not be willing to delegate or accept maintenance tasks on this codebase in its current form.