technion / ruby-argon2

A Ruby gem offering bindings for Argon2 password hashing
MIT License
229 stars 30 forks source link

Incorrect initialization checks: `ARGON2_MEMORY_TOO_LITTLE` raised when m_cost < 3 #61

Closed f3ndot closed 1 year ago

f3ndot commented 1 year ago

Looking at the source code for v2.2.0 and even master's not-yet-released MIN_M_COST constant, the library checks that an initialization kwarg m_cost value isn't less than 1:

https://github.com/technion/ruby-argon2/blob/3282118946d4aba1ba5138fc11f36833edea5692/lib/argon2.rb#L19

https://github.com/technion/ruby-argon2/blob/3282118946d4aba1ba5138fc11f36833edea5692/lib/argon2.rb#L28-L29

The minimum value should be 3, not 1-- At least for a Debian 12 docker image (Windows 10 host):

PS C:\Users\Justin> docker run --rm -it ruby:3.2.2 bash
root@dfbcabeafa6d:/# gem install argon2
Fetching ffi-1.15.5.gem
Fetching ffi-compiler-1.0.1.gem
Fetching argon2-2.2.0.gem
Building native extensions. This could take a while...
Successfully installed ffi-1.15.5
Successfully installed ffi-compiler-1.0.1
Building native extensions. This could take a while...
Successfully installed argon2-2.2.0
3 gems installed

A new release of RubyGems is available: 3.4.10 → 3.4.19!
Run `gem update --system 3.4.19` to update your installation.

root@dfbcabeafa6d:/# irb
irb(main):001:0> require 'argon2'
=> true
irb(main):002:0> Argon2::Password.new(t_cost: 1, m_cost: 3, p_cost: 1).create("password")
=> "$argon2id$v=19$m=8,t=1,p=1$RzeGPHz7Fw3eYnpYalvPWg$JXMw3AUrDpFYfuvu2Gmj7yXdofLOXpAQYBzNGAVPJrM"
irb(main):003:0> Argon2::Password.new(t_cost: 1, m_cost: 2, p_cost: 1).create("password")
/usr/local/bundle/gems/argon2-2.2.0/lib/argon2/ffi_engine.rb:91:in `block in hash_argon2id_encode': ARGON2_MEMORY_TOO_LITTLE (Argon2::ArgonHashFail)
        from /usr/local/bundle/gems/argon2-2.2.0/lib/argon2/ffi_engine.rb:87:in `initialize'
        from /usr/local/bundle/gems/argon2-2.2.0/lib/argon2/ffi_engine.rb:87:in `new'
        from /usr/local/bundle/gems/argon2-2.2.0/lib/argon2/ffi_engine.rb:87:in `hash_argon2id_encode'
        from /usr/local/bundle/gems/argon2-2.2.0/lib/argon2.rb:34:in `create'
        from (irb):3:in `<main>'
        from /usr/local/lib/ruby/gems/3.2.0/gems/irb-1.6.2/exe/irb:11:in `<top (required)>'
        from /usr/local/bin/irb:25:in `load'
        from /usr/local/bin/irb:25:in `<main>'
irb(main):004:0> Argon2::Password.new(t_cost: 1, m_cost: 1, p_cost: 1).create("password")
/usr/local/bundle/gems/argon2-2.2.0/lib/argon2/ffi_engine.rb:91:in `block in hash_argon2id_encode': ARGON2_MEMORY_TOO_LITTLE (Argon2::ArgonHashFail)
        from /usr/local/bundle/gems/argon2-2.2.0/lib/argon2/ffi_engine.rb:87:in `initialize'
        from /usr/local/bundle/gems/argon2-2.2.0/lib/argon2/ffi_engine.rb:87:in `new'
        from /usr/local/bundle/gems/argon2-2.2.0/lib/argon2/ffi_engine.rb:87:in `hash_argon2id_encode'
        from /usr/local/bundle/gems/argon2-2.2.0/lib/argon2.rb:34:in `create'
        from (irb):4:in `<main>'
        from /usr/local/lib/ruby/gems/3.2.0/gems/irb-1.6.2/exe/irb:11:in `<top (required)>'
        from /usr/local/bin/irb:25:in `load'
        from /usr/local/bin/irb:25:in `<main>'
irb(main):005:0>

Now I will concede that argon2's minimum memory values is dependent on certain factors around the machine running it. For portability I understand that it may be best to let the ffi'd argon2 engine be responsible for the errors and validation.

That being said, if you were to release the current state of master the following would raise an exception which would violate many author's/developer's assumptions:

Argon2::Password.new(m_cost: Argon2::Password::MIN_M_COST).create("somepassword")
# raises ARGON2_MEMORY_TOO_LITTLE (Argon2::ArgonHashFail)

If possible, the ruby constants should map to or extract from the C constants ARGON2_MIN_MEMORY and its friends in argon2.h: https://github.com/P-H-C/phc-winner-argon2/blob/f57e61e19229e23c4445b85494dbf7c07de721cb/include/argon2.h#L46-L88

technion commented 1 year ago

Thanks for this report!

I believe the original intention was to use "minimums" that basically ensured the C library was being passed something sane enough that we didn't get a UB, and letting the exception you've shown be the exception the C library raised for you. Now that a recent commit exposed these values, they should be better. I'm pushing an increase now.