From 11ccede58e5ed7dc65764a7cd245cad9863eb899 Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Sat, 14 Jul 2018 06:17:11 -0700 Subject: [PATCH] Deprecate String#crypt, move implementation to string/crypt extension 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. --- common.mk | 4 - configure.ac | 56 ---------- ext/string/crypt/crypt.c | 103 ++++++++++++++++++ ext/string/crypt/extconf.rb | 52 +++++++++ {missing => ext/string/crypt/missing}/crypt.c | 79 -------------- {missing => ext/string/crypt/missing}/crypt.h | 6 - .../string/crypt/missing}/des_tables.c | 0 string.c | 88 ++------------- test/ruby/test_m17n_comb.rb | 10 +- test/ruby/test_string.rb | 4 +- test/webrick/test_httpauth.rb | 8 +- 11 files changed, 180 insertions(+), 230 deletions(-) create mode 100644 ext/string/crypt/crypt.c create mode 100644 ext/string/crypt/extconf.rb rename {missing => ext/string/crypt/missing}/crypt.c (94%) rename {missing => ext/string/crypt/missing}/crypt.h (97%) rename {missing => ext/string/crypt/missing}/des_tables.c (100%) diff --git a/common.mk b/common.mk index 287241e2ed..10a573b36e 100644 --- a/common.mk +++ b/common.mk @@ -869,7 +869,6 @@ RUBY_H_INCLUDES = {$(VPATH)}ruby.h {$(VPATH)}config.h {$(VPATH)}defines.h \ acosh.$(OBJEXT): {$(VPATH)}acosh.c alloca.$(OBJEXT): {$(VPATH)}alloca.c {$(VPATH)}config.h -crypt.$(OBJEXT): {$(VPATH)}crypt.c {$(VPATH)}crypt.h {$(VPATH)}missing/des_tables.c dup2.$(OBJEXT): {$(VPATH)}dup2.c erf.$(OBJEXT): {$(VPATH)}erf.c explicit_bzero.$(OBJEXT): {$(VPATH)}explicit_bzero.c @@ -944,8 +943,6 @@ $(srcs_vpath)mjit_compile.inc: $(srcdir)/tool/ruby_vm/views/mjit_compile.inc.erb common-srcs: $(srcs_vpath)parse.c $(srcs_vpath)lex.c $(srcs_vpath)enc/trans/newline.c $(srcs_vpath)id.c \ srcs-lib srcs-ext incs -missing-srcs: $(srcdir)/missing/des_tables.c - srcs: common-srcs missing-srcs srcs-enc EXT_SRCS = $(srcdir)/ext/ripper/ripper.c \ @@ -2755,7 +2752,6 @@ strftime.$(OBJEXT): {$(VPATH)}timev.h string.$(OBJEXT): $(hdrdir)/ruby/ruby.h string.$(OBJEXT): $(top_srcdir)/include/ruby.h string.$(OBJEXT): {$(VPATH)}config.h -string.$(OBJEXT): {$(VPATH)}crypt.h string.$(OBJEXT): {$(VPATH)}debug_counter.h string.$(OBJEXT): {$(VPATH)}defines.h string.$(OBJEXT): {$(VPATH)}encindex.h diff --git a/configure.ac b/configure.ac index 074a3fd394..c2fc434f3b 100644 --- a/configure.ac +++ b/configure.ac @@ -760,58 +760,12 @@ AS_CASE(["$target_os"], test -d "$d" && RUBY_APPEND_OPTIONS(LDFLAGS, "-L$d") done ac_cv_type_getgroups=gid_t # getgroups() on Rosetta fills garbage - ac_cv_lib_crypt_crypt=no ac_cv_func_fdatasync=no # Mac OS X wrongly reports it has fdatasync() ac_cv_func_vfork=no AS_IF([test $gcc_major -lt 4 -o \( $gcc_major -eq 4 -a $gcc_minor -lt 3 \)], [ ac_cv_func___builtin_setjmp=no ]) with_setjmp_type=sigsetjmp # to hijack SIGCHLD handler - AC_CACHE_CHECK(for broken crypt with 8bit chars, rb_cv_broken_crypt, - [AC_TRY_RUN([ -#include -#include -#include - -void -broken_crypt(const char *salt, const char *buf1, const char *buf2) -{ -#if 0 - printf("%.2x%.2x: %s -> %s\n", (unsigned char)salt[0], (unsigned char)salt[1], - buf1+2, buf2+2); -#endif -} - -int -main() -{ - int i; - char salt[2], buf[256], *s; - for (i = 0; i < 128*128; i++) { - salt[0] = 0x80 | (i & 0x7f); - salt[1] = 0x80 | (i >> 7); - strcpy(buf, crypt("", salt)); - if (strcmp(buf, s = crypt("", salt))) { - broken_crypt(salt, buf, s); - return 1; - } - } - salt[0] = salt[1] = ' '; - strcpy(buf, crypt("", salt)); - salt[0] = salt[1] = 0x80 | ' '; - if (strcmp(buf, s = crypt("", salt))) { - broken_crypt(salt, buf, s); - return 1; - } - return 0; -} -], - rb_cv_broken_crypt=no, - rb_cv_broken_crypt=yes, - rb_cv_broken_crypt=yes)]) - AS_IF([test "$rb_cv_broken_crypt" = yes], [ - AC_DEFINE(BROKEN_CRYPT, 1) - ]) POSTLINK="" AC_CHECK_PROGS(codesign, codesign) AC_CHECK_PROGS(dsymutil, dsymutil) @@ -899,7 +853,6 @@ main() ac_cv_func_link=yes ac_cv_func_readlink=yes ac_cv_func_symlink=yes - ac_cv_lib_crypt_crypt=no ac_cv_func_getpgrp_void=no ac_cv_func_memcmp_working=yes ac_cv_lib_dl_dlopen=no @@ -952,7 +905,6 @@ main() [ LIBS="-lm $LIBS"]) : ${ORIG_LIBS=$LIBS} -AC_CHECK_LIB(crypt, crypt) # glibc (GNU/Linux, GNU/Hurd, GNU/kFreeBSD) AC_CHECK_LIB(dl, dlopen) # Dynamic linking for SunOS/Solaris and SYSV AC_CHECK_LIB(dld, shl_load) # Dynamic linking for HP-UX AC_CHECK_LIB(socket, shutdown) # SunOS/Solaris @@ -1664,7 +1616,6 @@ AS_CASE(["$target_os"],[freebsd*],[ AC_REPLACE_FUNCS(acosh) AC_REPLACE_FUNCS(cbrt) -AC_REPLACE_FUNCS(crypt) AC_REPLACE_FUNCS(dup2) AC_REPLACE_FUNCS(erf) AC_REPLACE_FUNCS(explicit_bzero) @@ -1745,7 +1696,6 @@ AC_CHECK_FUNCS(chroot) AC_CHECK_FUNCS(chsize) AC_CHECK_FUNCS(clock_gettime) AC_CHECK_FUNCS(cosh) -AC_CHECK_FUNCS(crypt_r) AC_CHECK_FUNCS(daemon) AC_CHECK_FUNCS(dirfd) AC_CHECK_FUNCS(dl_iterate_phdr) @@ -1895,12 +1845,6 @@ main(int argc, char **argv) AS_IF([test "$rb_cv_getcwd_malloc" = no], [AC_DEFINE(NO_GETCWD_MALLOC, 1)]) ]) -AS_IF([test "$ac_cv_func_crypt_r" = yes], - [AC_CHECK_HEADERS(crypt.h)]) -AS_IF([test "$ac_cv_func_crypt_r:$ac_cv_header_crypt_h" = yes:yes], - [AC_CHECK_MEMBERS([struct crypt_data.initialized], [], [], - [AC_INCLUDES_DEFAULT([@%:@include ])])]) - RUBY_CHECK_BUILTIN_FUNC(__builtin_alloca_with_align, [__builtin_alloca_with_align(1, 4096)]) RUBY_CHECK_BUILTIN_FUNC(__builtin_assume_aligned, [__builtin_assume_aligned((void*)32, 32)]) RUBY_CHECK_BUILTIN_FUNC(__builtin_bswap16, [__builtin_bswap16(0)]) diff --git a/ext/string/crypt/crypt.c b/ext/string/crypt/crypt.c new file mode 100644 index 0000000000..2527e7c0b4 --- /dev/null +++ b/ext/string/crypt/crypt.c @@ -0,0 +1,103 @@ +#include + +#include "ruby.h" +#include "ruby/encoding.h" + +#if defined HAVE_CRYPT_R +# if defined HAVE_CRYPT_H +# include +# endif +#elif !defined HAVE_CRYPT +# include "missing/crypt.h" +# include "missing/crypt.c" +# define HAVE_CRYPT_R 1 +#endif + +static void +mustnot_wchar(VALUE str) +{ + rb_encoding *enc = rb_enc_get(str); + if (rb_enc_mbminlen(enc) > 1) { + rb_raise(rb_eArgError, "wide char encoding: %s", rb_enc_name(enc)); + } +} + +/* + * call-seq: + * str.crypt(salt_str) -> new_str + * + * Applies a one-way cryptographic hash to str by invoking the + * standard library function crypt(3) with the given + * salt string. While the format and the result are system and + * implementation dependent, using a salt matching the regular + * expression \A[a-zA-Z0-9./]{2} should work on most + * most platforms, but on those platforms it uses insecure DES + * encryption, and only the first two characters of the salt string + * are significant. + * + * This method is for use in system specific scripts, so if you want + * a cross-platform hash function consider using Digest or OpenSSL + * instead. + */ + +static VALUE +rb_string_crypt(VALUE str, VALUE salt) +{ +#ifdef HAVE_CRYPT_R + VALUE databuf; + struct crypt_data *data; +# define CRYPT_END() ALLOCV_END(databuf) +#else + extern char *crypt(const char *, const char *); +# define CRYPT_END() (void)0 +#endif + VALUE result; + const char *s, *saltp; + char *res; +#ifdef BROKEN_CRYPT + char salt_8bit_clean[3]; +#endif + + StringValue(salt); + mustnot_wchar(str); + mustnot_wchar(salt); + if (RSTRING_LEN(salt) < 2) { + short_salt: + rb_raise(rb_eArgError, "salt too short (need >=2 bytes)"); + } + + s = StringValueCStr(str); + saltp = RSTRING_PTR(salt); + if (!saltp[0] || !saltp[1]) goto short_salt; +#ifdef BROKEN_CRYPT + if (!ISASCII((unsigned char)saltp[0]) || !ISASCII((unsigned char)saltp[1])) { + salt_8bit_clean[0] = saltp[0] & 0x7f; + salt_8bit_clean[1] = saltp[1] & 0x7f; + salt_8bit_clean[2] = '\0'; + saltp = salt_8bit_clean; + } +#endif +#ifdef HAVE_CRYPT_R + data = ALLOCV(databuf, sizeof(struct crypt_data)); +# ifdef HAVE_STRUCT_CRYPT_DATA_INITIALIZED + data->initialized = 0; +# endif + res = crypt_r(s, saltp, data); +#else + res = crypt(s, saltp); +#endif + if (!res) { + int err = errno; + CRYPT_END(); + rb_syserr_fail(err, "crypt"); + } + result = rb_str_new_cstr(res); + CRYPT_END(); + FL_SET_RAW(result, OBJ_TAINTED_RAW(str) | OBJ_TAINTED_RAW(salt)); + return result; +} + +void Init_crypt(void) { + rb_eval_string("class String; remove_method(:crypt) if instance_methods(false).include?(:crypt); end"); + rb_define_method(rb_cString, "crypt", rb_string_crypt, 1); +} diff --git a/ext/string/crypt/extconf.rb b/ext/string/crypt/extconf.rb new file mode 100644 index 0000000000..2e9b9a2ae4 --- /dev/null +++ b/ext/string/crypt/extconf.rb @@ -0,0 +1,52 @@ +require 'mkmf' + +if /darwin/ =~ RUBY_PLATFORM && try_run(< +#include +#include + +void +broken_crypt(const char *salt, const char *buf1, const char *buf2) +{ +#if 0 + printf("%.2x%.2x: %s -> %s\n", (unsigned char)salt[0], (unsigned char)salt[1], + buf1+2, buf2+2); +#endif +} + +int +main() +{ + int i; + char salt[2], buf[256], *s; + for (i = 0; i < 128*128; i++) { + salt[0] = 0x80 | (i & 0x7f); + salt[1] = 0x80 | (i >> 7); + strcpy(buf, crypt("", salt)); + if (strcmp(buf, s = crypt("", salt))) { + broken_crypt(salt, buf, s); + return 1; + } + } + salt[0] = salt[1] = ' '; + strcpy(buf, crypt("", salt)); + salt[0] = salt[1] = 0x80 | ' '; + if (strcmp(buf, s = crypt("", salt))) { + broken_crypt(salt, buf, s); + return 1; + } + return 0; +} +END + $defs << "-DBROKEN_CRYPT" +end + +if have_header('crypt.h') + have_struct_member("struct crypt_data", "initialized", "crypt.h") +end + +have_library('crypt', 'crypt') +have_func('crypt_r') +have_func('crypt') + +create_makefile 'string/crypt' diff --git a/missing/crypt.c b/ext/string/crypt/missing/crypt.c similarity index 94% rename from missing/crypt.c rename to ext/string/crypt/missing/crypt.c index f523aa51e6..3a46e4acf6 100644 --- a/missing/crypt.c +++ b/ext/string/crypt/missing/crypt.c @@ -372,22 +372,6 @@ static const C_block constdatablock = {{0}}; /* encryption constant */ static void des_setkey_r(const unsigned char *key, struct crypt_data *data); static void des_cipher_r(const unsigned char *in, unsigned char *out, long salt, int num_iter, struct crypt_data *data); -#ifdef USE_NONREENTRANT_CRYPT -static struct crypt_data default_crypt_data; -#endif - -#ifdef USE_NONREENTRANT_CRYPT -/* - * Return a pointer to static data consisting of the "setting" - * followed by an encryption produced by the "key" and "setting". - */ -char * -crypt(const char *key, const char *setting) -{ - return crypt_r(key, setting, &default_crypt_data); -} -#endif - /* * Return a pointer to data consisting of the "setting" followed by an * encryption produced by the "key" and "setting". @@ -792,69 +776,6 @@ init_perm(C_block perm[64/CHUNKBITS][1<= 0; i--) { - k = cblock.b[i]; - for (j = 7; j >= 0; j--) { - *--block = k&01; - k >>= 1; - } - } -} - #ifdef DEBUG STATIC void prtab(const char *s, const unsigned char *t, int num_rows) diff --git a/missing/crypt.h b/ext/string/crypt/missing/crypt.h similarity index 97% rename from missing/crypt.h rename to ext/string/crypt/missing/crypt.h index 7c2642f593..32191e5a87 100644 --- a/missing/crypt.h +++ b/ext/string/crypt/missing/crypt.h @@ -237,12 +237,6 @@ struct crypt_data { char cryptresult[1+4+4+11+1]; /* encrypted result */ }; -char *crypt(const char *key, const char *setting); -void setkey(const char *key); -void encrypt(char *block, int flag); - char *crypt_r(const char *key, const char *setting, struct crypt_data *data); -void setkey_r(const char *key, struct crypt_data *data); -void encrypt_r(char *block, int flag, struct crypt_data *data); #endif /* CRYPT_H */ diff --git a/missing/des_tables.c b/ext/string/crypt/missing/des_tables.c similarity index 100% rename from missing/des_tables.c rename to ext/string/crypt/missing/des_tables.c diff --git a/string.c b/string.c index 0ce8c40ecb..4819c502c2 100644 --- a/string.c +++ b/string.c @@ -25,7 +25,6 @@ #define BEG(no) (regs->beg[(no)]) #define END(no) (regs->end[(no)]) -#include #include #include @@ -33,15 +32,6 @@ #include #endif -#if defined HAVE_CRYPT_R -# if defined HAVE_CRYPT_H -# include -# endif -#elif !defined HAVE_CRYPT -# include "missing/crypt.h" -# define HAVE_CRYPT_R 1 -#endif - #define STRING_ENUMERATORS_WANTARRAY 0 /* next major */ #undef rb_str_new @@ -242,15 +232,6 @@ mustnot_broken(VALUE str) } } -static void -mustnot_wchar(VALUE str) -{ - rb_encoding *enc = STR_ENC_GET(str); - if (rb_enc_mbminlen(enc) > 1) { - rb_raise(rb_eArgError, "wide char encoding: %s", rb_enc_name(enc)); - } -} - static int fstring_cmp(VALUE a, VALUE b); static VALUE register_fstring(VALUE str); @@ -9202,72 +9183,19 @@ rb_str_oct(VALUE str) * * Applies a one-way cryptographic hash to str by invoking the * standard library function crypt(3) with the given - * salt string. While the format and the result are system and - * implementation dependent, using a salt matching the regular - * expression \A[a-zA-Z0-9./]{2} should be valid and - * safe on any platform, in which only the first two characters are - * significant. - * - * This method is for use in system specific scripts, so if you want - * a cross-platform hash function consider using Digest or OpenSSL - * instead. + * salt string. + * + * This core method is deprecated, require "string/crypt" to continue + * using it. */ static VALUE rb_str_crypt(VALUE str, VALUE salt) { -#ifdef HAVE_CRYPT_R - VALUE databuf; - struct crypt_data *data; -# define CRYPT_END() ALLOCV_END(databuf) -#else - extern char *crypt(const char *, const char *); -# define CRYPT_END() (void)0 -#endif - VALUE result; - const char *s, *saltp; - char *res; -#ifdef BROKEN_CRYPT - char salt_8bit_clean[3]; -#endif - - StringValue(salt); - mustnot_wchar(str); - mustnot_wchar(salt); - if (RSTRING_LEN(salt) < 2) { - short_salt: - rb_raise(rb_eArgError, "salt too short (need >=2 bytes)"); - } - - s = StringValueCStr(str); - saltp = RSTRING_PTR(salt); - if (!saltp[0] || !saltp[1]) goto short_salt; -#ifdef BROKEN_CRYPT - if (!ISASCII((unsigned char)saltp[0]) || !ISASCII((unsigned char)saltp[1])) { - salt_8bit_clean[0] = saltp[0] & 0x7f; - salt_8bit_clean[1] = saltp[1] & 0x7f; - salt_8bit_clean[2] = '\0'; - saltp = salt_8bit_clean; - } -#endif -#ifdef HAVE_CRYPT_R - data = ALLOCV(databuf, sizeof(struct crypt_data)); -# ifdef HAVE_STRUCT_CRYPT_DATA_INITIALIZED - data->initialized = 0; -# endif - res = crypt_r(s, saltp, data); -#else - res = crypt(s, saltp); -#endif - if (!res) { - int err = errno; - CRYPT_END(); - rb_syserr_fail(err, "crypt"); - } - result = rb_str_new_cstr(res); - CRYPT_END(); - FL_SET_RAW(result, OBJ_TAINTED_RAW(str) | OBJ_TAINTED_RAW(salt)); - return result; + rb_warn("The String#crypt core method is deprecated. " \ + "require \"string/crypt\" to continue using this method."); + rb_require("string/crypt"); + return rb_funcall(str, rb_intern("crypt"), 1, salt); } diff --git a/test/ruby/test_m17n_comb.rb b/test/ruby/test_m17n_comb.rb index 99c162a92f..6a315506d9 100644 --- a/test/ruby/test_m17n_comb.rb +++ b/test/ruby/test_m17n_comb.rb @@ -771,12 +771,18 @@ def test_str_crypt_nonstrict end end + private def crypt_result(str, salt) + assert_warn(/\A\z|The String#crypt core method is deprecated/) do + str.crypt(salt) + end + end + private def confirm_crypt_result(str, salt) if b(salt).length < 2 - assert_raise(ArgumentError) { str.crypt(salt) } + assert_raise(ArgumentError) { crypt_result(str, salt) } return end - t = str.crypt(salt) + t = crypt_result(str, salt) assert_equal(b(str).crypt(b(salt)), t, "#{encdump(str)}.crypt(#{encdump(salt)})") assert_encoding('ASCII-8BIT', t.encoding) end diff --git a/test/ruby/test_string.rb b/test/ruby/test_string.rb index 878ecb3083..0c7ea62da0 100644 --- a/test/ruby/test_string.rb +++ b/test/ruby/test_string.rb @@ -651,7 +651,9 @@ def test_crypt skip "This sometimes fails with -DMJIT_FORCE_ENABLE. This seems important to be fixed..." end - assert_equal(S('aaGUC/JkO9/Sc'), S("mypassword").crypt(S("aa"))) + assert_warn(/\A\z|The String#crypt core method is deprecated/) do + assert_equal(S('aaGUC/JkO9/Sc'), S("mypassword").crypt(S("aa"))) + end assert_not_equal(S('aaGUC/JkO9/Sc'), S("mypassword").crypt(S("ab"))) assert_raise(ArgumentError) {S("mypassword").crypt(S(""))} assert_raise(ArgumentError) {S("mypassword").crypt(S("\0a"))} diff --git a/test/webrick/test_httpauth.rb b/test/webrick/test_httpauth.rb index 8439be2025..593c1a6ae4 100644 --- a/test/webrick/test_httpauth.rb +++ b/test/webrick/test_httpauth.rb @@ -57,7 +57,9 @@ def test_basic_auth2 Tempfile.create("test_webrick_auth") {|tmpfile| tmpfile.close tmp_pass = WEBrick::HTTPAuth::Htpasswd.new(tmpfile.path) - tmp_pass.set_passwd(realm, "webrick", "supersecretpassword") + assert_warn(/\A\z|The String#crypt core method is deprecated/) do + tmp_pass.set_passwd(realm, "webrick", "supersecretpassword") + end tmp_pass.set_passwd(realm, "foo", "supersecretpassword") tmp_pass.flush @@ -117,7 +119,9 @@ def test_bad_username_with_control_characters Tempfile.create("test_webrick_auth") {|tmpfile| tmpfile.close tmp_pass = WEBrick::HTTPAuth::Htpasswd.new(tmpfile.path) - tmp_pass.set_passwd(realm, "webrick", "supersecretpassword") + assert_warn(/\A\z|The String#crypt core method is deprecated/) do + tmp_pass.set_passwd(realm, "webrick", "supersecretpassword") + end tmp_pass.set_passwd(realm, "foo", "supersecretpassword") tmp_pass.flush -- 2.17.1