From a6ff93c66fa406f97f1f3faf02da0252a8246b21 Mon Sep 17 00:00:00 2001 From: Jeremy Evans Date: Thu, 21 Jul 2011 15:47:00 -0700 Subject: [PATCH 2/2] Avoid inadvertent symbol creation in reflection methods Inadvertent symbol creation when passing strings to reflection methods can lead to denial of service by memory exhaustion. The basic principle in this commit is that if a string is passed to any of these reflection methods: respond_to? method_defined? public_method_defined? private_method_defined? protected_method_defined? const_defined? instance_variable_defined? class_variable_defined? Then we attempt to lookup the string in the symbol table. If the string is not in the symbol table, then we can return false immediately, because then such a method/const/instance variable/class variable cannot exist. Currently this breaks backwards compatibility for the following code: const_defined?("foo") instance_variable_defined?("foo") class_variable_defined?("foo") These used to raise NameErrors, now they return false. It's probably not difficult to fix this issue, but it would require adding equivalent functions that take strings instead of IDs for the following methods: rb_is_const_id rb_is_instance_id rb_is_class_id I didn't want to do that work until I know whether this patch has a chance for acceptance. The original idea for this came from Michael Koziarski. --- include/ruby/ruby.h | 1 + object.c | 11 ++++++++--- parse.y | 24 ++++++++++++++++++++++++ test/ruby/test_parse.rb | 12 ++++++++++++ vm_method.c | 23 ++++++++++++++++++----- 5 files changed, 63 insertions(+), 8 deletions(-) diff --git a/include/ruby/ruby.h b/include/ruby/ruby.h index 340bea1..7f5633d 100644 --- a/include/ruby/ruby.h +++ b/include/ruby/ruby.h @@ -1098,6 +1098,7 @@ ID rb_intern(const char*); ID rb_intern2(const char*, long); ID rb_intern_str(VALUE str); const char *rb_id2name(ID); +int rb_check_id(VALUE, ID*); ID rb_to_id(VALUE); VALUE rb_id2str(ID); diff --git a/object.c b/object.c index 57b349a..bfc5ac3 100644 --- a/object.c +++ b/object.c @@ -1833,7 +1833,8 @@ rb_mod_const_defined(int argc, VALUE *argv, VALUE mod) else { rb_scan_args(argc, argv, "11", &name, &recur); } - id = rb_to_id(name); + if(!rb_check_id(name, &id)) + return Qfalse; if (!rb_is_const_id(id)) { rb_name_error(id, "wrong constant name %s", rb_id2name(id)); } @@ -1923,7 +1924,9 @@ rb_obj_ivar_set(VALUE obj, VALUE iv, VALUE val) static VALUE rb_obj_ivar_defined(VALUE obj, VALUE iv) { - ID id = rb_to_id(iv); + ID id; + if(!rb_check_id(iv, &id)) + return Qfalse; if (!rb_is_instance_id(id)) { rb_name_error(id, "`%s' is not allowed as an instance variable name", rb_id2name(id)); @@ -2002,7 +2005,9 @@ rb_mod_cvar_set(VALUE obj, VALUE iv, VALUE val) static VALUE rb_mod_cvar_defined(VALUE obj, VALUE iv) { - ID id = rb_to_id(iv); + ID id; + if(!rb_check_id(iv, &id)) + return Qfalse; if (!rb_is_class_id(id)) { rb_name_error(id, "`%s' is not allowed as a class variable name", rb_id2name(id)); diff --git a/parse.y b/parse.y index db8791b..f550265 100644 --- a/parse.y +++ b/parse.y @@ -10073,6 +10073,30 @@ rb_is_junk_id(ID id) return is_junk_id(id); } +int +rb_check_id(VALUE name, ID *id) +{ + VALUE tmp; + + switch (TYPE(name)) { + default: + tmp = rb_check_string_type(name); + if (NIL_P(tmp)) { + tmp = rb_inspect(name); + rb_raise(rb_eTypeError, "%s is not a symbol", + RSTRING_PTR(tmp)); + } + name = tmp; + /* fall through */ + case T_STRING: + return st_lookup(global_symbols.sym_id, (st_data_t)name, id); + case T_SYMBOL: + *id = SYM2ID(name); + return 1; + } + return 0; /* not reached */ +} + #endif /* !RIPPER */ static void diff --git a/test/ruby/test_parse.rb b/test/ruby/test_parse.rb index 99a6edf..900c796 100644 --- a/test/ruby/test_parse.rb +++ b/test/ruby/test_parse.rb @@ -813,6 +813,18 @@ x = __ENCODING__ assert_equal(':"foo=="', "foo==".intern.inspect) end + def test_no_inadvertent_symbol_creation + s = "gadzooks" + {:respond_to? =>"#{s}1", :method_defined? =>"#{s}2", + :public_method_defined? =>"#{s}3", :private_method_defined? =>"#{s}4", + :protected_method_defined? =>"#{s}5", :const_defined? =>"A#{s}", + :instance_variable_defined? =>"@#{s}", :class_variable_defined? =>"@@#{s}" + }.each do |meth, str| + Object.send(meth, str) + assert !Symbol.all_symbols.any?{|sym| sym.to_s == str} + end + end + def test_all_symbols x = Symbol.all_symbols assert_kind_of(Array, x) diff --git a/vm_method.c b/vm_method.c index 86ff2ee..b8991cb 100644 --- a/vm_method.c +++ b/vm_method.c @@ -713,7 +713,10 @@ rb_mod_undef_method(int argc, VALUE *argv, VALUE mod) static VALUE rb_mod_method_defined(VALUE mod, VALUE mid) { - if (!rb_method_boundp(mod, rb_to_id(mid), 1)) { + ID id; + if(!rb_check_id(mid, &id)) + return Qfalse; + if (!rb_method_boundp(mod, id, 1)) { return Qfalse; } return Qtrue; @@ -763,7 +766,10 @@ check_definition(VALUE mod, ID mid, rb_method_flag_t noex) static VALUE rb_mod_public_method_defined(VALUE mod, VALUE mid) { - return check_definition(mod, rb_to_id(mid), NOEX_PUBLIC); + ID id; + if(!rb_check_id(mid, &id)) + return Qfalse; + return check_definition(mod, id, NOEX_PUBLIC); } /* @@ -795,7 +801,10 @@ rb_mod_public_method_defined(VALUE mod, VALUE mid) static VALUE rb_mod_private_method_defined(VALUE mod, VALUE mid) { - return check_definition(mod, rb_to_id(mid), NOEX_PRIVATE); + ID id; + if(!rb_check_id(mid, &id)) + return Qfalse; + return check_definition(mod, mid, NOEX_PRIVATE); } /* @@ -827,7 +836,10 @@ rb_mod_private_method_defined(VALUE mod, VALUE mid) static VALUE rb_mod_protected_method_defined(VALUE mod, VALUE mid) { - return check_definition(mod, rb_to_id(mid), NOEX_PROTECTED); + ID id; + if(!rb_check_id(mid, &id)) + return Qfalse; + return check_definition(mod, id, NOEX_PROTECTED); } int @@ -1238,7 +1250,8 @@ obj_respond_to(int argc, VALUE *argv, VALUE obj) ID id; rb_scan_args(argc, argv, "11", &mid, &priv); - id = rb_to_id(mid); + if(!rb_check_id(mid, &id)) + return Qfalse; if (basic_obj_respond_to(obj, id, !RTEST(priv))) return Qtrue; return Qfalse; -- 1.7.5