Simple Password Utility

Question

I am somewhat new to ruby (but not to programming) and I want to develop a small web-app with Sinatra. I started creating a small password utility script that hashes and secures entered password. I am not sure it is finished. I would like to know if there are any ruby tricks to make my code better or more verbose, and what else needs to be done / changed to make passwords more secure. Any criticism is appreciated. here is my code:

require 'digest'

def random_salt
    hash = ''
    for i in 1..64  
        rand_seed = rand(0..61)
        hash << 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890'[rand_seed]
    end
    return hash
end

# password format: salt + password + length_of_salt_and_hash
def hash_password(password)
    salt = random_salt
    hashed_pass = Digest::SHA512.hexdigest password.chomp
    length = salt.length + hashed_pass.length

    return salt + hashed_pass + length.to_s
end

# just a test
print 'Enter a password: '
word = gets
new_word = hash_password word
puts new_word   

Answer 1

My critique is mostly on the crypto.

1. Never invent your own crypto. It's difficult to do correctly, and there are already good libraries for this sort of thing. See sinatra-authentication or bcrypt-ruby.
2. rand() is not a cryptographically secure random number generator, so it's a poor choice for use in crypto. Also see point #1.
3. Your password salting is incorrect. The salt must be mixed with the password before hashing, not after. Also see point #1.
4. Why append the length_of_salt_and_hash? That seems pointless.
5. Why append salt + hashed_pass? These are usually stored as separate fields in a database record.
6. Avoid magic numbers like 61 in your code. What does Roger Maris have to do with this? It's unclear and redundant that this 61 refers to the length of the string on the next line.
7. You ask for "better or more verbose." Those are often opposite goals. More verbose code means more code to read and more code to maintain. (That does not mean you should prefer cryptic one-liners.)

Answer 2

ScottJ's answer hits all the right points. As he says, never invent your own crypto.

However, you say you're new to Ruby, so if we pretend you're going with this code, here are some notes. I'll focus on the #random_salt method, though some of it is more general.

• The ruby convention is 2 spaces of indentation. Not 4, not tabs.
• You don't need return, so don't write it.
• for..in is very rarely necessary
• As ScottJ mentioned, avoid magic numbers.

So let's do a few iterations on #random_salt.

• for i in 1..64 can be written as 64.times do .. end
• 0..61 would be clearer and more robust if you did something like 0..alphabet.length where alphabet would be the long string.
• Of course, the string should probably be a constant.
• Don't push stuff to an array, when you can use things like Enumerable#map
• If you want to pick something at random from an array, you can use Array#sample (note: not cryptographically secure, so read on!). And to get an array of characters in a string, you can use String#chars

So in all you get:

ALPHABET = 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890'.freeze

def random_salt
  64.times.map { alphabet.chars.sample }.join
end

Buuut as mentioned, you'll want better randomness, so you should use Ruby's SecureRandom (part of the stdlib):

require 'securerandom'

SecureRandom.hex(32) # => "234a745cb6a98a0aa17c37c74fea4736950c3475827f677f6f1487ec95244b87"

Yes, 32, because we want 32 random bytes, which - when expressed as hex string - becomes 64 letters and digits. You can also get the random bytes as a Base64 encoded string.

Of course, all of this is moot, since you'll want to use something like bcrypt instead, but still.