Project

General

Profile

Feature #14915

Deprecate String#crypt

Added by jeremyevans0 (Jeremy Evans) 6 months ago. Updated 7 days ago.

Status:
Open
Priority:
Normal
Assignee:
-
Target version:
-
[ruby-core:87959]

Description

This method is system and implementation dependent, and the
portable usage mentioned in the documentation is not truly
portable (doesn't work on OpenBSD) and insecure as it uses DES.
For systems that lack a crypt(3) implementation, Ruby will
happily substitute a version that only supports DES. It's 2018,
using DES should be avoided if at all possible.

The only internal usage of String#crypt in Ruby is in Webrick,
where it uses DES for basic authentication with an htpasswd file.
That could and should be changed to use a more secure hash by
default (bcrypt since that's the most secure htpasswd format),
or at least allow the user to customize Webrick's authentication.
I expect there are few if any users actively using Webrick's
htpasswd support.

This moves the String#crypt implementation to the string/crypt
extension, but leaves the String#crypt core method. The core
method prints a deprecation warning, then loads the string/crypt
extension. The string/crypt extension undefines the String#crypt
core method, then defines the previous implementation.

Because extensions use extconf.rb instead of configure for their
configuration, this ports the related configure.ac code to
extconf.rb. I'm not sure that is done correctly and works on
all platforms, it will need testing.

For systems that lack a crypt(3) implementation, this modifies the
fallback code to only define crypt_r, since that is the only
function that String#crypt will call in that case.

While the patch just deprecates String#crypt, I think
we should plan to remove support from ruby:

2.6: core method deprecated
2.7: core method removed, string/crypt extension ships with ruby
2.8: string/crypt extension moves to external gem, not shipped


Files

0001-Deprecate-String-crypt-move-implementation-to-string.patch (20.5 KB) 0001-Deprecate-String-crypt-move-implementation-to-string.patch jeremyevans0 (Jeremy Evans), 07/16/2018 05:47 PM
0001-Deprecate-String-crypt.patch (7.48 KB) 0001-Deprecate-String-crypt.patch jeremyevans0 (Jeremy Evans), 07/26/2018 07:42 PM
0001-Deprecate-String-crypt.patch (7.35 KB) 0001-Deprecate-String-crypt.patch jeremyevans0 (Jeremy Evans), 11/29/2018 08:27 AM
0001-Deprecate-String-crypt.patch (7.43 KB) 0001-Deprecate-String-crypt.patch jeremyevans0 (Jeremy Evans), 11/29/2018 03:20 PM

Associated revisions

Revision 37c22bd9
Added by shyouhei (Shyouhei Urabe) about 2 months ago

string.c: [DOC] deprecate String#crypt [ci skip] [Feature #14915]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@66154 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 66154
Added by shyouhei (Shyouhei Urabe) about 2 months ago

string.c: [DOC] deprecate String#crypt [ci skip] [Feature #14915]

Revision 66154
Added by shyouhei (Shyouhei Urabe) about 2 months ago

string.c: [DOC] deprecate String#crypt [ci skip] [Feature #14915]

History

Updated by normalperson (Eric Wong) 6 months ago

merch-redmine@jeremyevans.net wrote:

Because extensions use extconf.rb instead of configure for their
configuration, this ports the related configure.ac code to
extconf.rb. I'm not sure that is done correctly and works on
all platforms, it will need testing.

More exts increase build and maintenance time. Right now, the
easiest and safest step would be to only deprecate it, but
keep it in core.

While I don't care for #crypt, I'd like to move some tiny exts
like fiber, io/wait, io/nonblock directly into core; because
DSOs increase memory usage and slow down startup:

https://udrepper.livejournal.com/8790.html

While the patch just deprecates String#crypt, I think
we should plan to remove support from ruby:

2.6: core method deprecated
2.7: core method removed, string/crypt extension ships with ruby
2.8: string/crypt extension moves to external gem, not shipped

Way too fast. I agree with deprecation, but any removal should
take long-term distro release cycles (5-10 years) into account.

Updated by jeremyevans0 (Jeremy Evans) 6 months ago

normalperson (Eric Wong) wrote:

merch-redmine@jeremyevans.net wrote:

Because extensions use extconf.rb instead of configure for their
configuration, this ports the related configure.ac code to
extconf.rb. I'm not sure that is done correctly and works on
all platforms, it will need testing.

More exts increase build and maintenance time. Right now, the
easiest and safest step would be to only deprecate it, but
keep it in core.

I agree that would be easiest and safest. However, at some point it should be moved out, and I think it's better to start that process early.

While I don't care for #crypt, I'd like to move some tiny exts
like fiber, io/wait, io/nonblock directly into core; because
DSOs increase memory usage and slow down startup:

https://udrepper.livejournal.com/8790.html

Note that DSOs only increase memory usage and slow down startup if they are actually used. This would increase memory usage and slow down startup for users of String#crypt. However, this would decrease memory usage and speed up startup for the 99.9%+ of ruby users that do not use String#crypt, since it would result in a smaller libruby.

FWIW, I'm fine including fiber, io/wait, and io/nonblock directly in core, because those extensions are small and the functionality they provide is useful in a large number of situations.

While the patch just deprecates String#crypt, I think
we should plan to remove support from ruby:

2.6: core method deprecated
2.7: core method removed, string/crypt extension ships with ruby
2.8: string/crypt extension moves to external gem, not shipped

Way too fast. I agree with deprecation, but any removal should
take long-term distro release cycles (5-10 years) into account.

That's fine. In the patch I don't specify it will be removed, just that it is deprecated.

Note that not removing String#crypt also has risks. Mainly it risks unsuspecting users using it without understanding that doesn't really provide security. I think we are doing a disservice to those users by basically promoting an insecure approach. Most code that would be broken by the removal of String#crypt is insecure and should probably be updated.

Updated by shyouhei (Shyouhei Urabe) 6 months ago

I agree that crypt(3) is too difficult to use correctly. Properly avoiding traditional modes to generate adequate hash are up to users, which is not how we do security these days. OpenBSD splits the functionality into crypt_checkpass(3) and crypt_newhash(3), which are far better (meseems).

Said that, Ruby's String#crypt has been there for a while and we should expect wild applications. You have to carefully watch your step for its removal.

normalperson (Eric Wong) wrote:

While I don't care for #crypt, I'd like to move some tiny exts
like fiber, io/wait, io/nonblock directly into core; because
DSOs increase memory usage and slow down startup:

For String#crypt, because crypt(3) typically resides in libc, no extra DSO shall be necessary by theory. How about call it from pure ruby (maybe using fiddle)?

Updated by jeremyevans0 (Jeremy Evans) 6 months ago

shyouhei (Shyouhei Urabe) wrote:

I agree that crypt(3) is too difficult to use correctly. Properly avoiding traditional modes to generate adequate hash are up to users, which is not how we do security these days. OpenBSD splits the functionality into crypt_checkpass(3) and crypt_newhash(3), which are far better (meseems).

Said that, Ruby's String#crypt has been there for a while and we should expect wild applications. You have to carefully watch your step for its removal.

Definitely. I don't think we should remove String#crypt without deprecating it. It's mostly a question of how long the deprecation period should be. Maybe 1 year is too short. However, I think 10 years is definitely too long.

normalperson (Eric Wong) wrote:

While I don't care for #crypt, I'd like to move some tiny exts
like fiber, io/wait, io/nonblock directly into core; because
DSOs increase memory usage and slow down startup:

For String#crypt, because crypt(3) typically resides in libc, no extra DSO shall be necessary by theory.

I think Eric means that if we move String#crypt to string/crypt, that's an extra string/crypt.so DSO you have to load if you use String#crypt.

How about call it from pure ruby (maybe using fiddle)?

String#crypt can be easily emulated using fiddle if crypt is in libc:

require 'fiddle'
libc = Fiddle.dlopen('/path/to/libc.so')
crypt = Fiddle::Function.new(
  libc['crypt'],
  [Fiddle::TYPE_VOIDP, Fiddle::TYPE_VOIDP],
  Fiddle::TYPE_VOIDP
)
crypt.call('password', 'sl').to_s

It's more work if crypt_r is used, though.

Updated by mame (Yusuke Endoh) 6 months ago

I agree with deprecation and removal of String#crypt. If it were not included in the core, and if it were proposed today, the proposal would be definitely rejected as "it should be implemented as a gem".

This moves the String#crypt implementation to the string/crypt
extension, but leaves the String#crypt core method. The core
method prints a deprecation warning, then loads the string/crypt
extension.

It looks complex a bit to me, and I don't see its advantage. How about:

2.6: core method deprecated, and a compatibility-layer gem released
3.0?: core method removed

?

Unfortunately, the name "crypt" is already taken, though...

Incidentally, I found some usages of String#crypt in some gems.

https://github.com/eventmachine/eventmachine/blob/537127fa22a058e3d7b248a214df210e7c77dc95/lib/em/protocols/postgres3.rb#L187
https://github.com/activeldap/activeldap/blob/a63e89c384cf7f5bb95451a3749e6dc4aa799969/lib/active_ldap/user_password.rb#L38

Updated by normalperson (Eric Wong) 6 months ago

merch-redmine@jeremyevans.net wrote:

normalperson (Eric Wong) wrote:

While I don't care for #crypt, I'd like to move some tiny exts
like fiber, io/wait, io/nonblock directly into core; because
DSOs increase memory usage and slow down startup:

https://udrepper.livejournal.com/8790.html

Note that DSOs only increase memory usage and slow down
startup if they are actually used. This would increase memory
usage and slow down startup for users of String#crypt.
However, this would decrease memory usage and speed up startup
for the 99.9%+ of ruby users that do not use String#crypt,
since it would result in a smaller libruby.

Problem is right now we have no idea how many people use
String#crypt and how they will be affected by its deprecation.
It will take several years to know that, as we have seen
regressions which don't get reported for several releases
because distros (and users) are slow to upgrade.

Even with deprecation warnings, it can be too annoying to some
regular users who don't write code and just run some Ruby apps.

There's a lot of stuff I'd remove from Ruby to reduce footprint
and startup times first :)

Note that not removing String#crypt also has risks. Mainly it
risks unsuspecting users using it without understanding that
doesn't really provide security. I think we are doing a
disservice to those users by basically promoting an insecure
approach. Most code that would be broken by the removal of
String#crypt is insecure and should probably be updated.

I don't disagree, but we need to educate developers, first;
instead of just annoying users who don't write or maintain code.

Updated by shyouhei (Shyouhei Urabe) 6 months ago

I can list up 10+ more reasons why using String#crypt is a bad idea today, but it seems we all agree on that point. The problem is how to let programmers avoid uisng it.

In addition to deprecate the method, how about removing DES from String#crypt ? DES is completely usafe today and shall not be used anyway. Using DES with String#crypt requires a programmer's ignorance and/or casual mistakes. By prohibiting the algorithm, I think we can educate programmers to abandon the method to something better, like bcrypt or argon2.

mame (Yusuke Endoh) wrote:

Incidentally, I found some usages of String#crypt in some gems.

https://github.com/eventmachine/eventmachine/blob/537127fa22a058e3d7b248a214df210e7c77dc95/lib/em/protocols/postgres3.rb#L187
https://github.com/activeldap/activeldap/blob/a63e89c384cf7f5bb95451a3749e6dc4aa799969/lib/active_ldap/user_password.rb#L38

Interesting. So for instance PostgreSQL requires DES password hash to be generated? That does not work on OpenBSD anymore, because they dropped DES support years ago.

Updated by jeremyevans0 (Jeremy Evans) 6 months ago

mame (Yusuke Endoh) wrote:

I agree with deprecation and removal of String#crypt. If it were not included in the core, and if it were proposed today, the proposal would be definitely rejected as "it should be implemented as a gem".

This moves the String#crypt implementation to the string/crypt
extension, but leaves the String#crypt core method. The core
method prints a deprecation warning, then loads the string/crypt
extension.

It looks complex a bit to me, and I don't see its advantage. How about:

2.6: core method deprecated, and a compatibility-layer gem released
3.0?: core method removed

The advantage of having the core method require the extension (either shipped with ruby or an external gem) is that you get the same behavior. If the extension has different behavior than the core method, because configure feature detection is different than extconf.rb feature detection, it's going to be hard to debug, especially with so few users of this code.

That being said, I'm fine with just deprecating the core method, shipping an external gem, and removing the core method in 3.0.

Unfortunately, the name "crypt" is already taken, though...

"string-crypt" is not taken, which is the reason I named the extension "string/crypt".

Incidentally, I found some usages of String#crypt in some gems.

https://github.com/eventmachine/eventmachine/blob/537127fa22a058e3d7b248a214df210e7c77dc95/lib/em/protocols/postgres3.rb#L187

The last PostgreSQL version that supported crypt passwords was 8.3, which stopped being supported over 5 years ago (PostgreSQL removed support for crypt passwords in 8.4, and no longer supports <9.3). You would have to be connecting to a pre-8.4 server for it to be possible to connect with crypt passwords, and even in old PostgreSQL versions they recommended against crypt passwords, stating "crypt to be used only if you must support pre-7.2 clients".

I think it's safe to say nobody is executing that code and would notice if it broke.

https://github.com/activeldap/activeldap/blob/a63e89c384cf7f5bb95451a3749e6dc4aa799969/lib/active_ldap/user_password.rb#L38

This gem depends on ~10 other gems if you count transitive dependencies, some heavy weight such as activemodel, and is actively maintained by a ruby committer. I'm guessing kou wouldn't have a problem depending on another gem for the crypt support, but I don't want to speak for him.

Updated by mame (Yusuke Endoh) 6 months ago

This ticket has been discussed at the developers' meeting today.

Some committers had a skeptic opinion against the removal, but agreed with it in some conditions.

We first need to remove the usage of String#crypt from WEBrick. normalperson (Eric Wong), the maintainer of WEBrick, what do you think? Is it possible and acceptable? If it is not, we can't deprecate String#crpyt as long as WEBrick is bundled.
If normalperson agrees with the removal, we then ask jeremyevans0 (Jeremy Evans) to release compatibility-layer gem by 2.6 release. If the release is succeeded, we can deprecate the core method, with a deprecation message like "String#crypt is deprecated; use `string-crypt' gem".
And finally, after normalperson (Eric Wong) removed the dependency from WEBrick in any way, we will remove the core method, maybe in 3.x.

Updated by normalperson (Eric Wong) 6 months ago

mame@ruby-lang.org wrote:

We first need to remove the usage of String#crypt from WEBrick. normalperson (Eric Wong), the maintainer of WEBrick, what do you think? Is it possible and acceptable? If it is not, we can't deprecate String#crpyt as long as WEBrick is bundled.
If normalperson agrees with the removal, we then ask jeremyevans0 (Jeremy Evans) to release compatibility-layer gem by 2.6 release. If the release is succeeded, we can deprecate the core method, with a deprecation message like "String#crypt is deprecated; use `string-crypt' gem".
And finally, after normalperson (Eric Wong) removed the dependency from WEBrick in any way, we will remove the core method, maybe in 3.x.

It's only the BasicAuth code and I have no idea how many people
use it; probably not many, but I don't want to break a single
user setup without adequate notice.

Even if BasicAuth is broken/removed/requires-extra-download,
most WEBrick can use the rest without difficulty.

WEBrick has several other authentication methods; which one do
we recommend users migrate to? I am not an expert in this area
(I don't like authentication at all).

Updated by shyouhei (Shyouhei Urabe) 6 months ago

normalperson (Eric Wong) wrote:

It's only the BasicAuth code and I have no idea how many people
use it; probably not many, but I don't want to break a single
user setup without adequate notice.

Understand your concern. The problem is, String#crypt is a thin
wrapper of crypt(3) whose behaviour is not under our control. It
might change from time to time.

Looking at the current implementation of
webrick/httpauth/basicuath.rb, The usage of String#crypt silently
expects that crypt(3) acts in traditional (== DES) mode. This
mode has been dropped in OpenBSD for years. It now expects some
fixed-format string as its argument. See also:
https://man.openbsd.org/crypt.3

So in short, the status quo is already broken. I recommend you
to add "adequate notice" ASAP.

Even if BasicAuth is broken/removed/requires-extra-download,
most WEBrick can use the rest without difficulty.

WEBrick has several other authentication methods; which one do
we recommend users migrate to? I am not an expert in this area
(I don't like authentication at all).

There are two separate issues I think.

  • Basic authentication itself does not require DES. It is an
    implementation detail. You can safely migrate to another
    hash function at will. Ruby's openssl library has for
    instance OpenSSL::KDF.scrypt().

  • On the other hand, WEBrick now supports .htpasswd file, which
    contain DES hash strings. This does not interface with other
    hash functions.

There are three possible solutions for this situation I think:

  1. Keep supporting DES-formatted .htpasswd file. In order to do
    so you arguably need your own DES implementation, given
    OpenBSD lacks one.

  2. Give up DES, but contiue supporting BasicAuth in different
    hasing function (maybe using OpenSSL library). This requires
    user-side migration of existing .htpasswd files into some new
    format, but nothing more.

  3. Give up BasicAuth at all to migrate to DigestAuth. Requires
    both developer-side and user-side to work on it.

On Wed, Jul 18, 2018 at 5:42 PM, Eric Wong normalperson@yhbt.net wrote:

mame@ruby-lang.org wrote:

We first need to remove the usage of String#crypt from WEBrick. normalperson (Eric Wong), the maintainer of WEBrick, what do you think? Is it possible and acceptable? If it is not, we can't deprecate String#crpyt as long as WEBrick is bundled.
If normalperson agrees with the removal, we then ask jeremyevans0 (Jeremy Evans) to release compatibility-layer gem by 2.6 release. If the release is succeeded, we can deprecate the core method, with a deprecation message like "String#crypt is deprecated; use `string-crypt' gem".
And finally, after normalperson (Eric Wong) removed the dependency from WEBrick in any way, we will remove the core method, maybe in 3.x.

It's only the BasicAuth code and I have no idea how many people
use it; probably not many, but I don't want to break a single
user setup without adequate notice.

Even if BasicAuth is broken/removed/requires-extra-download,
most WEBrick can use the rest without difficulty.

WEBrick has several other authentication methods; which one do
we recommend users migrate to? I am not an expert in this area
(I don't like authentication at all).

Updated by jeremyevans0 (Jeremy Evans) 6 months ago

shyouhei (Shyouhei Urabe) wrote:

  • Basic authentication itself does not require DES. It is an implementation detail. You can safely migrate to another hash function at will. Ruby's openssl library has for instance OpenSSL::KDF.scrypt().

Note that OpenSSL::KDF.scrypt is only defined if the OpenSSL library defines EVP_PBE_scrypt, so it isn't always available. I believe this was added in OpenSSL 1.1.0, and it isn't currently available in LibreSSL. OpenSSL::KDF.pbkdf2_hmac is always defined, so that could be used as long as the openssl extension can be required.

Personally, I think it's best to punt and require the user provide implementations for creating a password hash and checking the password hash via two callable options (e.g. :create_password_hash and :check_password_hash). That way the user can determine which password hash format they want to use, and it doesn't tie Webrick to one particular hash implementation. If that is considered acceptable I can work on a patch for Webrick that keeps the current default behavior with a deprecation warning if the callable options are not provided.

Updated by normalperson (Eric Wong) 6 months ago

"Urabe, Shyouhei" shyouhei@ruby-lang.org wrote:

normalperson (Eric Wong) wrote:

It's only the BasicAuth code and I have no idea how many people
use it; probably not many, but I don't want to break a single
user setup without adequate notice.

Understand your concern. The problem is, String#crypt is a thin
wrapper of crypt(3) whose behaviour is not under our control. It
might change from time to time.

OK, then we can refer users to their operating system
documentation when it breaks. In other words, we can shift
responsibility for breakage over to whoever provides crypt(3)
for their system.

What we should do now is try to educate users to avoid crypt(3)
before they start using it.

However, if users already depend on it, lets not break their use
case before their operating system breaks it for them.

Again, if system (libc) breaks something, there's nothing we can
do. However, lets not prematurely break things; but try to warn
and educate users to avoid future breakage.

Btw, glibc is considering such a shift to using 3rd-party libcrypt:

https://public-inbox.org/libc-alpha/20180625134403.2B6DC43994575@oldenburg.str.redhat.com/t/#u

Looking at the current implementation of
webrick/httpauth/basicuath.rb, The usage of String#crypt silently
expects that crypt(3) acts in traditional (== DES) mode. This
mode has been dropped in OpenBSD for years. It now expects some
fixed-format string as its argument. See also:
https://man.openbsd.org/crypt.3
https://man.openbsd.org/crypt.3

So in short, the status quo is already broken. I recommend you
to add "adequate notice" ASAP.

See proposed patch below.

Even if BasicAuth is broken/removed/requires-extra-download,
most WEBrick can use the rest without difficulty.

WEBrick has several other authentication methods; which one do
we recommend users migrate to? I am not an expert in this area
(I don't like authentication at all).

There are two separate issues I think.

  • Basic authentication itself does not require DES. It is an
    implementation detail. You can safely migrate to another
    hash function at will. Ruby's openssl library has for
    instance OpenSSL::KDF.scrypt().

  • On the other hand, WEBrick now supports .htpasswd file, which
    contain DES hash strings. This does not interface with other
    hash functions.

There are three possible solutions for this situation I think:

  1. Keep supporting DES-formatted .htpasswd file. In order to do so you arguably need your own DES implementation, given OpenBSD lacks one.

Nope, too much work to support broken behavior.

  1. Give up DES, but contiue supporting BasicAuth in different hasing function (maybe using OpenSSL library). This requires user-side migration of existing .htpasswd files into some new format, but nothing more.

Maybe, as Jeremy proposed in [ruby-core:88002]. But does it
make sense to add more options instead of telling people to use
existing options (perhaps DigestAuth)?

Both require work on the user-side to reconfigure, so I'd rather
we tell them to use an existing functionality rather than new
option (easier for people on older Ruby or webrick versions).

  1. Give up BasicAuth at all to migrate to DigestAuth. Requires both developer-side and user-side to work on it.

Why is there need for developer-side work on it? (aside from
documentation change)

Anyways, 3. seems ideal, we can slowly let users deal with it.
Anybody who complains when crypt(3) breaks, we can tell them
"we told you so" once we make documentation updates:

 diff --git a/lib/webrick/httpauth/basicauth.rb b/lib/webrick/httpauth/basicauth.rb
 index e23420fdfa..c774035ea1 100644
 --- a/lib/webrick/httpauth/basicauth.rb
 +++ b/lib/webrick/httpauth/basicauth.rb
 @@ -18,6 +18,10 @@ module HTTPAuth
  ##
  # Basic Authentication for WEBrick
  #
 +    # Do not use this for new deployments, use DigestAuth and
 +    # Htdigest instead.  Htpasswd and BasicAuth are insecure
 +    # and will be break incompatibly, soon.
 +    #
  # Use this class to add basic authentication to a WEBrick servlet.
  #
  # Here is an example of how to set up a BasicAuth:
 diff --git a/lib/webrick/httpauth/htpasswd.rb b/lib/webrick/httpauth/htpasswd.rb
 index 976eeeb13e..783394220d 100644
 --- a/lib/webrick/httpauth/htpasswd.rb
 +++ b/lib/webrick/httpauth/htpasswd.rb
 @@ -16,6 +16,10 @@ module WEBrick
 module HTTPAuth

  ##
 +    # Do not use this for new deployments, use Htdigest instead.
 +    # Htpasswd and BasicAuth are insecure and will break incompatibly,
 +    # soon.
 +    #
  # Htpasswd accesses apache-compatible password files.  Passwords are
  # matched to a realm where they are valid.  For security, the path for a
  # password database should be stored outside of the paths available to the

Updated by normalperson (Eric Wong) 6 months ago

merch-redmine@jeremyevans.net wrote:

Personally, I think it's best to punt and require the user
provide implementations for creating a password hash and
checking the password hash via two callable options (e.g.
:create_password_hash and :check_password_hash). That way the
user can determine which password hash format they want to
use, and it doesn't tie Webrick to one particular hash
implementation. If that is considered acceptable I can work
on a patch for Webrick that keeps the current default behavior
with a deprecation warning if the callable options are not
provided.

Is the worth the effort for you or anybody to work on
such an option (which still requires user education) vs
telling them to use Htdigest, which already exists?

Updated by jeremyevans0 (Jeremy Evans) 6 months ago

normalperson (Eric Wong) wrote:

merch-redmine@jeremyevans.net wrote:

Personally, I think it's best to punt and require the user
provide implementations for creating a password hash and
checking the password hash via two callable options (e.g.
:create_password_hash and :check_password_hash). That way the
user can determine which password hash format they want to
use, and it doesn't tie Webrick to one particular hash
implementation. If that is considered acceptable I can work
on a patch for Webrick that keeps the current default behavior
with a deprecation warning if the callable options are not
provided.

Is the worth the effort for you or anybody to work on
such an option (which still requires user education) vs
telling them to use Htdigest, which already exists?

I think that a single iteration of MD5 is not a significant security improvement on DES:

    class DigestAuth
      def self.make_passwd(realm, user, pass)
        pass ||= ""
        Digest::MD5::hexdigest([user, realm, pass].join(":"))
      end
    end

When using HTTP, digest auth is better than basic auth if your threat model includes connection sniffing (passive or active MITM). When using HTTPS with trusted certificates (define trusted however you like), I'd much prefer basic auth with a strong hash function than digest auth.

Updated by shyouhei (Shyouhei Urabe) 6 months ago

On Fri, Jul 20, 2018 at 5:51 PM, Eric Wong normalperson@yhbt.net wrote:

"Urabe, Shyouhei" shyouhei@ruby-lang.org wrote:

normalperson (Eric Wong) wrote:

It's only the BasicAuth code and I have no idea how many people
use it; probably not many, but I don't want to break a single
user setup without adequate notice.

Understand your concern. The problem is, String#crypt is a thin
wrapper of crypt(3) whose behaviour is not under our control. It
might change from time to time.

OK, then we can refer users to their operating system
documentation when it breaks. In other words, we can shift
responsibility for breakage over to whoever provides crypt(3)
for their system.

What we should do now is try to educate users to avoid crypt(3)
before they start using it.

However, if users already depend on it, lets not break their use
case before their operating system breaks it for them.

Again, if system (libc) breaks something, there's nothing we can
do. However, lets not prematurely break things; but try to warn
and educate users to avoid future breakage.

Makes sense to me.

Btw, glibc is considering such a shift to using 3rd-party libcrypt:

https://public-inbox.org/libc-alpha/20180625134403.2B6DC43994575@oldenburg.str.redhat.com/t/#u

Looking at the current implementation of
webrick/httpauth/basicuath.rb, The usage of String#crypt silently
expects that crypt(3) acts in traditional (== DES) mode. This
mode has been dropped in OpenBSD for years. It now expects some
fixed-format string as its argument. See also:
https://man.openbsd.org/crypt.3
https://man.openbsd.org/crypt.3

So in short, the status quo is already broken. I recommend you
to add "adequate notice" ASAP.

See proposed patch below.

OK, the patch seems fine.

Even if BasicAuth is broken/removed/requires-extra-download,
most WEBrick can use the rest without difficulty.

WEBrick has several other authentication methods; which one do
we recommend users migrate to? I am not an expert in this area
(I don't like authentication at all).

There are two separate issues I think.

  • Basic authentication itself does not require DES. It is an
    implementation detail. You can safely migrate to another
    hash function at will. Ruby's openssl library has for
    instance OpenSSL::KDF.scrypt().

  • On the other hand, WEBrick now supports .htpasswd file, which
    contain DES hash strings. This does not interface with other
    hash functions.

There are three possible solutions for this situation I think:

  1. Keep supporting DES-formatted .htpasswd file. In order to do so you arguably need your own DES implementation, given OpenBSD lacks one.

Nope, too much work to support broken behavior.

  1. Give up DES, but contiue supporting BasicAuth in different hasing function (maybe using OpenSSL library). This requires user-side migration of existing .htpasswd files into some new format, but nothing more.

Maybe, as Jeremy proposed in [ruby-core:88002]. But does it
make sense to add more options instead of telling people to use
existing options (perhaps DigestAuth)?

Both require work on the user-side to reconfigure, so I'd rather
we tell them to use an existing functionality rather than new
option (easier for people on older Ruby or webrick versions).

  1. Give up BasicAuth at all to migrate to DigestAuth. Requires both developer-side and user-side to work on it.

Why is there need for developer-side work on it? (aside from
documentation change)

Maybe this was my misunderstanding. Are BasicAuth and DigestAuth
made API-compatible? If that is the case what developers should
do might be just replacing BasicAuth to DigestAuth, and Htpasswd
to Htdigest.

Anyways, 3. seems ideal, we can slowly let users deal with it.
Anybody who complains when crypt(3) breaks, we can tell them
"we told you so" once we make documentation updates:

Updated by jeremyevans0 (Jeremy Evans) 6 months ago

shyouhei (Shyouhei Urabe) wrote:

Maybe this was my misunderstanding. Are BasicAuth and DigestAuth
made API-compatible? If that is the case what developers should
do might be just replacing BasicAuth to DigestAuth, and Htpasswd
to Htdigest.

No, basic auth and digest auth are very different, you can't just substitute one for the other. Digest auth requires MD5 for one and doesn't support a more secure format for password hash storage, while basic auth submits the password in plain text and can use any hash algorithm for storing passwords. Some HTTP clients support basic auth and not digest auth.

Digest auth really doesn't make sense when you are using HTTPS, it was designed to protect against password disclosure when using HTTP with a passive MITM (an attacker who can observe but not modify traffic). These days, there are very few cases where digest auth would be a good idea.

Updated by normalperson (Eric Wong) 6 months ago

merch-redmine@jeremyevans.net wrote:

shyouhei (Shyouhei Urabe) wrote:

  • Basic authentication itself does not require DES. It is an implementation detail. You can safely migrate to another hash function at will. Ruby's openssl library has for instance OpenSSL::KDF.scrypt().

Note that OpenSSL::KDF.scrypt is only defined if the OpenSSL
library defines EVP_PBE_scrypt, so it isn't always available.
I believe this was added in OpenSSL 1.1.0, and it isn't
currently available in LibreSSL. OpenSSL::KDF.pbkdf2_hmac is
always defined, so that could be used as long as the openssl
extension can be required.

Personally, I think it's best to punt and require the user
provide implementations for creating a password hash and
checking the password hash via two callable options (e.g.
:create_password_hash and :check_password_hash).

Can you provide pre-defined, accepted-as-secure implementations
of these which we can recommend for common use cases which would
be compatible with other webservers? I'd rather not
introduce/recommend hashes which are incompatible with other
webservers, but two-way compatibility with existing servers
is a good thing.

It could optionally recommend/use 3rd-party gem if available
(e.g. 'bcrypt'), or the bundled 'openssl' ext.

That way the user can determine which password hash format
they want to use, and it doesn't tie Webrick to one particular
hash implementation. If that is considered acceptable I can
work on a patch for Webrick that keeps the current default
behavior with a deprecation warning if the callable options
are not provided.

That seems reasonable, but I want to avoid situations where
users cargo-cult blocks of code into the config they don't
understand. They could get stuck with an option which is
eventually found insecure and we'd have no way of warning them.

Thanks.

Updated by jeremyevans0 (Jeremy Evans) 6 months ago

normalperson (Eric Wong) wrote:

Can you provide pre-defined, accepted-as-secure implementations
of these which we can recommend for common use cases which would
be compatible with other webservers? I'd rather not
introduce/recommend hashes which are incompatible with other
webservers, but two-way compatibility with existing servers
is a good thing.

It could optionally recommend/use 3rd-party gem if available
(e.g. 'bcrypt'), or the bundled 'openssl' ext.

.htpasswd is an Apache-specific format, and Apache currently supports only 5 password formats: bcrypt, MD5, SHA1, crypt, plain text (https://httpd.apache.org/docs/2.4/misc/password_encryptions.html). Only bcrypt and MD5 are considered secure. The MD5 format uses a Apache-specific algorithm with 1000 iterations of MD5, and I'm not aware of a ruby implementation of it. I believe the bcrypt format is a standard bcrypt except that it uses 2y instead of 2a or 2b as the version, but I have not tested this yet. I will test it and assuming the bcrypt format is compatible, that is probably the format we should recommend.

That seems reasonable, but I want to avoid situations where
users cargo-cult blocks of code into the config they don't
understand. They could get stuck with an option which is
eventually found insecure and we'd have no way of warning them.

That's would be nice, but I'm not sure how attainable that is in terms of users understanding the options. A more attainable goal is a single example that is considered secure and compatible today, which I think leaves only bcrypt.

Updated by jeremyevans0 (Jeremy Evans) 6 months ago

jeremyevans0 (Jeremy Evans) wrote:

.htpasswd is an Apache-specific format, and Apache currently supports only 5 password formats: bcrypt, MD5, SHA1, crypt, plain text (https://httpd.apache.org/docs/2.4/misc/password_encryptions.html). Only bcrypt and MD5 are considered secure. The MD5 format uses a Apache-specific algorithm with 1000 iterations of MD5, and I'm not aware of a ruby implementation of it. I believe the bcrypt format is a standard bcrypt except that it uses 2y instead of 2a or 2b as the version, but I have not tested this yet. I will test it and assuming the bcrypt format is compatible, that is probably the format we should recommend.

Apache htpasswd generates passwords using $2y$, but the hashes are the same as $2a$ format, and Apache handles $2a$ format just fine. The bcrypt gem does not consider $2y$ to match, but will work correctly if $2y$ is changed to $2a$. According to Wikipedia (https://en.wikipedia.org/wiki/Bcrypt#Versioning_history), $2y$ instead of $2a$ was just to note that the hash was not generated by a bad version of the crypt_blowfish PHP library, otherwise the formats are identical.

So to check the passwords in ruby, you would just need to do: BCrypt::Password.new(password_hash.sub(/\A\$2y$/, '$2a$')) == submitted_password

Tested using Apache 2.4.33 with the following .htaccess file:

AuthType Basic
AuthName "Restricted Files"
AuthUserFile "/var/www/passwd"
Require user foo bar

Where /var/www/passwd was populated:

cd /var/www
touch passwd
htpasswd -bB passwd foo foofoofoo
ruby -r bcrypt -e 'puts BCrypt::Password.create("barbarbar")' > passwd

Testing that both foo and bar can login with the passwords, and that the ruby bcrypt library can correctly check password hashes generated by htpasswd if you do the $2y$ to $2a$ conversion as shown above.

Should I prepare a patch to Webrick to accept :create_password_hash and :check_password_hash options, and add documentation with an example using bcrypt?

Updated by normalperson (Eric Wong) 6 months ago

Thanks for the research on this matter

merch-redmine@jeremyevans.net wrote:

Should I prepare a patch to Webrick to accept
:create_password_hash and :check_password_hash options, and
add documentation with an example using bcrypt?

Can you do it as a single option which doesn't involve having
users copy+paste code? Something like:

:password_hash => (:bcrypt|:crypt|:blahblah)

It can raise and tell users to install 'bcrypt' if 'bcrypt'
is missing.

Updated by jeremyevans0 (Jeremy Evans) 6 months ago

normalperson (Eric Wong) wrote:

Can you do it as a single option which doesn't involve having
users copy+paste code? Something like:

:password_hash => (:bcrypt|:crypt|:blahblah)

It can raise and tell users to install 'bcrypt' if 'bcrypt'
is missing.

One issue with that is it doesn't offer a way to specify the password hash cost. For example, the bcrypt gem defaults to cost 10, and htpasswd defaults to cost 5 (cost 10 is 2**(10-5) times as many iterations as cost 5). It also doesn't offer a way to use custom authentication (e.g. using basic authentication to authenticate against LDAP). It's simple, but inflexible.

However, if that is still the API you want, I'll work on that and submit it as a separate feature.

Updated by shyouhei (Shyouhei Urabe) 6 months ago

Off topic but,

jeremyevans0 (Jeremy Evans) wrote:

Apache htpasswd generates passwords using $2y$, but the hashes are the same as $2a$ format, and Apache handles $2a$ format just fine. The bcrypt gem does not consider $2y$ to match, but will work correctly if $2y$ is changed to $2a$. According to Wikipedia (https://en.wikipedia.org/wiki/Bcrypt#Versioning_history), $2y$ instead of $2a$ was just to note that the hash was not generated by a bad version of the crypt_blowfish PHP library, otherwise the formats are identical.

Is $2a$ okay? The wikipedia entry you linked says that OpenBSD people abandoned $2a$ to move to $2b$. Sounds very much like it has flaw(s). Also, the author of $2y$ variant now releases crypt_blowfish version 1.3 which states:

Version 1.3 adds support for the $2b$ prefix introduced in OpenBSD 5.5+, which behaves exactly the same as crypt_blowfish's $2y$.

see also: http://www.openwall.com/crypt/

I hope this is not an issue. But at least sub(/\A\$2y$/, '$2a$') seems something we should not.

Updated by normalperson (Eric Wong) 6 months ago

merch-redmine@jeremyevans.net wrote:

normalperson (Eric Wong) wrote:

Can you do it as a single option which doesn't involve having
users copy+paste code? Something like:

:password_hash => (:bcrypt|:crypt|:blahblah)

It can raise and tell users to install 'bcrypt' if 'bcrypt'
is missing.

One issue with that is it doesn't offer a way to specify the
password hash cost. For example, the bcrypt gem defaults to
cost 10, and htpasswd defaults to cost 5 (cost 10 is 2**(10-5)
times as many iterations as cost 5). It also doesn't offer a
way to use custom authentication (e.g. using basic
authentication to authenticate against LDAP). It's simple,
but inflexible.

Allowing options which other servers do not support is not
something I want, as it could be a way to lock people into
WEBrick. Being compatible with htpasswd to allow users
of other servers to easily migrate in any direction is more
important.

I haven't looked into LDAP authentication; but maybe that can
use a URI to the LDAP server instead of path. I don't know how
Apache or other servers do it, even; but we should try to steal
configuration/setup ideas from others servers to minimize
migration costs in either direction and not introduce things
which make it difficult to migrate away from.

Updated by jeremyevans0 (Jeremy Evans) 6 months ago

shyouhei (Shyouhei Urabe) wrote:

Off topic but,

jeremyevans0 (Jeremy Evans) wrote:

Apache htpasswd generates passwords using $2y$, but the hashes are the same as $2a$ format, and Apache handles $2a$ format just fine. The bcrypt gem does not consider $2y$ to match, but will work correctly if $2y$ is changed to $2a$. According to Wikipedia (https://en.wikipedia.org/wiki/Bcrypt#Versioning_history), $2y$ instead of $2a$ was just to note that the hash was not generated by a bad version of the crypt_blowfish PHP library, otherwise the formats are identical.

Is $2a$ okay? The wikipedia entry you linked says that OpenBSD people abandoned $2a$ to move to $2b$. Sounds very much like it has flaw(s).

There is only a difference between $2a$ and $2b$ on OpenBSD if the password length is 256 characters or longer. Such passwords are very rare. The bcrypt gem currently only supports and generates $2a hashes, but much like $2y$, if you change $2b$ to $2a$, the hash will match.

I hope this is not an issue. But at least sub(/\A\$2y\$/, '$2a$') seems something we should not.

sub(/\A\$2y\$/, '$2b$') is only fine after the bcrypt gem is modified to recognize $2b$. I tested the bcrypt gem, and the algorithm it uses is compatible with OpenBSD's $2b$ format:

pass = 'a'*500
s = `encrypt #{pass}`.chomp
# => "$2b$08$2iobD98d6MzMpS6/ViH8euWUmKGBgp4D8txRHAmCeIcfAS7ZtKPxW"
BCrypt::Password.new(s) == pass
# => false
BCrypt::Password.new(s.sub(/\A\$2b\$/, '$2a$')) == pass
# => true

So there are no security issues in the sub(/\A\$2y\$/, '$2a$'). I would say that until the bcrypt gem is modified to recognize $2b$ and $2y$, that we use sub(/\A\$2[yb]\$/, '$2a$') for maximum compatibility.

Updated by jeremyevans0 (Jeremy Evans) 6 months ago

normalperson (Eric Wong) wrote:

Allowing options which other servers do not support is not
something I want, as it could be a way to lock people into
WEBrick. Being compatible with htpasswd to allow users
of other servers to easily migrate in any direction is more
important.

Fair enough, I'll work on adding support for :password_hash=>:bcrypt|:crypt (currently defaulting to :crypt if not given for backwords compatibility).

I haven't looked into LDAP authentication; but maybe that can
use a URI to the LDAP server instead of path. I don't know how
Apache or other servers do it, even; but we should try to steal
configuration/setup ideas from others servers to minimize
migration costs in either direction and not introduce things
which make it difficult to migrate away from.

I don't think it makes sense to add specific support for other authentication options. If we aren't going to offer generic support, then we should limit the feature addition to the ability to use bcrypt. That allows people the ability to use an .htpasswd file with a strong password hash, and allows String#crypt to be deprecated and then moved to a gem.

Updated by shyouhei (Shyouhei Urabe) 6 months ago

jeremyevans0 (Jeremy Evans) wrote:

So there are no security issues in the sub(/\A\$2y\$/, '$2a$').

Great. Thank you for confirmation.

Updated by jeremyevans0 (Jeremy Evans) 6 months ago

I've released a string-crypt gem based on the previous patch: https://github.com/jeremyevans/ruby-string-crypt

The gem has some minor differences in extconf.rb, because it needs to avoid overlap with macros defined in ruby/config.h. It also supports an environment variable during compilation to force use of the internal DES-crypt implementation, useful for testing support (especially on OpenBSD where crypt(3) doesn't support DES-crypt). I would appreciate users of other operating systems testing it and letting me know if anything breaks.

Attached is an updated patch that just deprecates String#crypt without making any changes, and updates the tests to handle the deprecation warnings.

Updated by jeremyevans0 (Jeremy Evans) 3 months ago

mame (Yusuke Endoh) wrote:

This ticket has been discussed at the developers' meeting today.

Some committers had a skeptic opinion against the removal, but agreed with it in some conditions.

We first need to remove the usage of String#crypt from WEBrick. normalperson (Eric Wong), the maintainer of WEBrick, what do you think? Is it possible and acceptable? If it is not, we can't deprecate String#crpyt as long as WEBrick is bundled.
If normalperson agrees with the removal, we then ask jeremyevans0 (Jeremy Evans) to release compatibility-layer gem by 2.6 release. If the release is succeeded, we can deprecate the core method, with a deprecation message like "String#crypt is deprecated; use `string-crypt' gem".

The Webrick issue has been dealt with, and the string-crypt gem was released back in July. Can we apply the most recent patch in this ticket to deprecate String#crypt?

Updated by jeremyevans0 (Jeremy Evans) about 2 months ago

jeremyevans0 (Jeremy Evans) wrote:

mame (Yusuke Endoh) wrote:

This ticket has been discussed at the developers' meeting today.

Some committers had a skeptic opinion against the removal, but agreed with it in some conditions.

We first need to remove the usage of String#crypt from WEBrick. normalperson (Eric Wong), the maintainer of WEBrick, what do you think? Is it possible and acceptable? If it is not, we can't deprecate String#crpyt as long as WEBrick is bundled.
If normalperson agrees with the removal, we then ask jeremyevans0 (Jeremy Evans) to release compatibility-layer gem by 2.6 release. If the release is succeeded, we can deprecate the core method, with a deprecation message like "String#crypt is deprecated; use `string-crypt' gem".

The Webrick issue has been dealt with, and the string-crypt gem was released back in July. Can we apply the most recent patch in this ticket to deprecate String#crypt?

I've updated the patch to apply against trunk. I would like this change to make 2.6 unless there are objections. Can a committer perform a final review and commit if acceptable?

Updated by normalperson (Eric Wong) about 2 months ago

merch-redmine@jeremyevans.net wrote:

File 0001-Deprecate-String-crypt.patch added

I am against the rb_warn call. Documentation change is enough.

Typical users (non-programmers) may not know how to quiet
the warning or fix the problem. They will just be annoyed,
and turned off of using Ruby.

Updated by mame (Yusuke Endoh) about 2 months ago

IMO, documentation is not enough. A warning may annoy a user, but sudden removal (in future) is much worse.

As a first step, how about rb_warning instead of rb_warn? It prints a warning only when $VERBOSE is enabled. A well-mannered developer can be aware of the deprecation, will fix the issue, and may even write how to fix in stackoverflow. A non-programmer user won't be annoyed.
And then, we can replace it with rb_warn in the next version. Of course all programs won't be ready, but thing would be better. Anyway, a fix is needed as long as we will remove String#crypt in future.

Updated by jeremyevans0 (Jeremy Evans) about 2 months ago

mame (Yusuke Endoh) wrote:

IMO, documentation is not enough. A warning may annoy a user, but sudden removal (in future) is much worse.

As a first step, how about rb_warning instead of rb_warn? It prints a warning only when $VERBOSE is enabled. A well-mannered developer can be aware of the deprecation, will fix the issue, and may even write how to fix in stackoverflow. A non-programmer user won't be annoyed.
And then, we can replace it with rb_warn in the next version. Of course all programs won't be ready, but thing would be better. Anyway, a fix is needed as long as we will remove String#crypt in future.

Here's an updated patch that switches to using rb_warning, with appropriate updates to the tests. This also fixes the the Webrick test changes to use spaces instead of tabs, consistent with the rest of the file.

Updated by normalperson (Eric Wong) about 2 months ago

mame@ruby-lang.org wrote:

IMO, documentation is not enough. A warning may annoy a user,
but sudden removal (in future) is much worse.

There should never be a sudden removal.

Honestly, I don't think we should make the first move on this
issue until more-widely used projects (e.g. glibc) start
warning or deprecating on it.

As a first step, how about rb_warning instead of rb_warn? It prints a warning only when $VERBOSE is enabled. A well-mannered developer can be aware of the deprecation, will fix the issue, and may even write how to fix in stackoverflow. A non-programmer user won't be annoyed.

It's better, I suppose. I don't know if we can guarantee $VERBOSE
isn't widely-used by some programs?

Updated by shyouhei (Shyouhei Urabe) about 2 months ago

Well, it seems we do not agree whether we should add a warning. But is anybody against on documenting the deprecation? I think we can:

  • Consider String#crypt deprecated as of 2.6.
  • Add NEWS about it.
  • Add rdoc about it.

Can I do the above? Or do we need to wait glibc for them?

Updated by normalperson (Eric Wong) about 2 months ago

shyouhei@ruby-lang.org wrote:

Well, it seems we do not agree whether we should add a
warning. But is anybody against on documenting the
deprecation? I think we can:

  • Consider String#crypt deprecated as of 2.6.
  • Add NEWS about it.
  • Add rdoc about it.

Can I do the above? Or do we need to wait glibc for them?

Sure, please do that. We can get Ruby programmers to stop using
it, at least.

I am against the warning because users of Ruby programs aren't
all programmers and some users aren't even bug reporters.

#37

Updated by shyouhei (Shyouhei Urabe) about 2 months ago

  • Status changed from Open to Closed

Applied in changeset trunk|r66154.


string.c: [DOC] deprecate String#crypt [ci skip] [Feature #14915]

Updated by shyouhei (Shyouhei Urabe) about 2 months ago

  • Status changed from Closed to Open

I did not intend to close this ticket. What remains to be discussed is:

  • Should we need to add warnings?
  • If that is the case, when and how?

Updated by mame (Yusuke Endoh) about 2 months ago

I think that the warning is needed. People (at least, I) don't read documents. Even if I noticed, I cannot remember where my code uses String#crypt.

I understand that the warning will annoy normal non-programmer users. However, some of the users may report the warning to the author, which leads to the fix before the removal. If no warning is printed, no one will notice the deprecation, and the future removal will break the program suddenly. This is much worse than annoying warnings.

Updated by normalperson (Eric Wong) about 2 months ago

mame@ruby-lang.org wrote:

I understand that the warning will annoy normal non-programmer
users. However, some of the users may report the warning to
the author, which leads to the fix before the removal.

Some users will also replace the Ruby program with something
else. I've seen it happen too many times with other warnings.

Instead of stderr which every user sees, how about we start
warning to syslog, which only admins will look at?

If no warning is printed, no one will notice the deprecation,
and the future removal will break the program suddenly. This
is much worse than annoying warnings.

Of course we never remove anything suddenly; and I trust
C library maintainers won't, either.

Updated by Eregon (Benoit Daloze) 7 days ago

If we want to remove a feature, we deprecate and warn first, that has always been the case in MRI.

shyouhei (Shyouhei Urabe) Please add the warning. Otherwise, no users will consider it deprecated.

normalperson (Eric Wong) If you disagree, please start a general discussion about warnings in another ticket.

Also available in: Atom PDF