New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing fixnum type checks #3476

Closed
clayton-shopify opened this Issue Feb 27, 2017 · 0 comments

Comments

Projects
None yet
1 participant
@clayton-shopify
Contributor

clayton-shopify commented Feb 27, 2017

https://hackerone.com/aerodudrizzt reported the following issue:


There are several different places in which arguments are treated as fixnums without a prior check for their type. Since mrb_value is a union that holds all value types, it can cause a mixup between an object pointer and an integer value:

typedef struct mrb_value {
  union {
    mrb_float f;
    void *p;
    mrb_int i;
    mrb_sym sym;
  } value;
  enum mrb_vtype tt;
} mrb_value;

PoC Script:

class Integer
    def <=>(arg1)
        return arg1
    end
end

s = "hello"
s.<=>(1)

And the output varies between runs (because of ASLR) and between architecture (32/64 bit) and seems like this: -69972254725992 meaning an address of: 0x3fa3af631768.

Vulnerable Code:

The mrb_str_cmp_m function (s.<=>()) in string.c uses the <=> function of the argument, if the argument is not a string. That function can be overridden (like was demonstrated in the PoC), and the returned value is not checked to be a fixnum, while it is treated as a fixnum:

mruby/src/string.c

Lines 963 to 966 in b563bcb

if (!mrb_fixnum(tmp)) {
return mrb_funcall(mrb, mrb_fixnum_value(0), "-", 1, tmp);
}
result = -mrb_fixnum(tmp);

This means that the PoC code gets tmp as the original string (since 1.<=>(str1) returns str1), and mrb_fixnum(tmp) will be the address of the string object. Since it is returned as -mrb_fixnum(tmp) our value was negative.

More minor examples:

  1. mrb_str_aref_m function in string.c does not check the fixnum's type. can cause only a very minor information-leak over the MSbit (pos < 0).
    return str_substr(mrb, str, mrb_fixnum(a1), mrb_fixnum(a2));
  2. mrb_str_index function in string.c does not check the 2nd arg, but has no security implications.
    pos = mrb_fixnum(argv[1]);
  3. mrb_str_rindex function in string.c does not check the 2nd arg, can again leak the MSbit of the address (again a vpos < 0 check).
    pos = mrb_fixnum(vpos);

Suggested Fix:

Before the argument / returned value is treated as a fixnum, it should be checked to match it in type using the mrb_fixnum_p macro, or any other chosen way.

@matz matz closed this in bdac7df Feb 28, 2017

matz added a commit that referenced this issue Feb 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment