fix Bitmap's object to string conversion #62

Merged

cremno merged 1 commits from mri-bitmap-fix-obj2str-conversion into master

2014-08-31 12:06:54 +00:00

cremno commented

2014-08-31 08:06:45 +00:00

(Migrated from github.com)

Calling #to_s might not return a string (it should though).

The RGSS also calls rb_obj_as_string:

class A
  B = 'Test'
  def to_s; B; end
end
s = Sprite.new
s.bitmap = Bitmap.new(Graphics.width, Graphics.height)
p A::B.tainted?  # => false
a = A.new
a.taint  # a is tainted, not A::B!
s.bitmap.draw_text(s.bitmap.rect, a)   # rb_obj_as_string() taints the return value of A#to_s (A::B)
p A::B.tainted?  # => true
rgss_stop

Don't worry about tainting, I just wanted to prove it (for some reason). More importantly is that RGSS classes don't check if the string contains '\0'. For example, drawing "Te\0st" doesn't raise an error like it should (see “poison null byte”) and only draws Te. Should mkxp?

Calling #to_s might not return a string (it should though). The RGSS also calls `rb_obj_as_string`: ``` ruby class A B = 'Test' def to_s; B; end end s = Sprite.new s.bitmap = Bitmap.new(Graphics.width, Graphics.height) p A::B.tainted? # => false a = A.new a.taint # a is tainted, not A::B! s.bitmap.draw_text(s.bitmap.rect, a) # rb_obj_as_string() taints the return value of A#to_s (A::B) p A::B.tainted? # => true rgss_stop ``` Don't worry about tainting, I just wanted to prove it (for some reason). More importantly is that RGSS classes don't check if the string contains `'\0'`. For example, drawing `"Te\0st"` doesn't raise an error like it should (see “poison null byte”) and only draws `Te`. Should mkxp?

Ancurio commented

2014-08-31 09:23:53 +00:00

(Migrated from github.com)

More importantly is that RGSS classes don't check if the string contains '\0'. For example, drawing "Te\0st" doesn't raise an error like it should (see “poison null byte”) and only draws Te. Should mkxp?

If RGSS behaves this way (ie. treating it as a null terminated C string regardless of the actual length), then I don't see a problem with mkxp too continuing to behave like this. Are there any corner cases you're aware of where this might end up hurting?

> More importantly is that RGSS classes don't check if the string contains '\0'. For example, drawing "Te\0st" doesn't raise an error like it should (see “poison null byte”) and only draws Te. Should mkxp? If RGSS behaves this way (ie. treating it as a null terminated C string regardless of the actual length), then I don't see a problem with mkxp too continuing to behave like this. Are there any corner cases you're aware of where this might end up hurting?

cremno commented

2014-08-31 09:56:03 +00:00

(Migrated from github.com)

I don't think so. But using StringValueCStr (which makes sure that a string doesn't contain NUL) instead of RSTRING_PTR would probably ease your concerns about CRuby's GC.

I didn't mention that Bitmap.new with a filename is affected, too. I doubt fixing this bug would break a game. Of course, the RGSS isn't as important as a libc or CRuby itself.

I don't think so. But using `StringValueCStr` (which makes sure that a string doesn't contain NUL) instead of `RSTRING_PTR` would probably ease your concerns about CRuby's GC. I didn't mention that `Bitmap.new` with a filename is affected, too. I doubt fixing this bug would break a game. Of course, the RGSS isn't as important as a [libc](http://googleprojectzero.blogspot.de/2014/08/the-poisoned-nul-byte-2014-edition.html) or [CRuby](https://www.ruby-lang.org/en/news/2012/10/12/poisoned-NUL-byte-vulnerability/) itself.

Ancurio commented

2014-08-31 10:20:29 +00:00

(Migrated from github.com)

I don't think so. But using StringValueCStr (which makes sure that a string doesn't contain NUL) instead of RSTRING_PTR would probably ease your concerns about CRuby's GC.

I don't understand how they are related in this case. A rogue null inside a string just means that we potentially read less than there is to read. How would this affect the GC / libc?

> I don't think so. But using StringValueCStr (which makes sure that a string doesn't contain NUL) instead of RSTRING_PTR would probably ease your concerns about CRuby's GC. I don't understand how they are related in this case. A rogue null inside a string just means that we potentially read less than there is to read. How would this affect the GC / libc?

cremno commented

2014-08-31 10:43:43 +00:00

(Migrated from github.com)

Using StringValueCStr would solve both issues, wouldn't it? str is on the stack and the bug is fixed.

Okay, as I've said it's not that important as someone exploiting Bitmap.new is unlikely, but I just don't see a major downside in making sure a string, which will be passed to a function that expects null-terminated strings, doesn't contain NUL

Using [`StringValueCStr`](http://rxr.whitequark.org/mri/source/include/ruby/ruby.h?v=2.1.2#537) would solve both issues, wouldn't it? `str` is on the stack and the bug is fixed. Okay, as I've said it's not that important as someone exploiting `Bitmap.new` is unlikely, but I just don't see a major downside in making sure a string, which will be passed to a function that expects null-terminated strings, doesn't contain NUL

Ancurio commented

2014-08-31 11:11:27 +00:00

(Migrated from github.com)

Using StringValueCStr would solve both issues, wouldn't it? str is on the stack and the bug is fixed.

Oh, so you mean using this function instead of your objAsStringPtr then? Anyway, that's okay, if it makes the code a bit simpler I don't really mind the difference in behavior between RMVX/A and mkxp as it's unlikely to come up. I just don't want to have more code for error checking which RGSS never does in the first place.

Okay, as I've said it's not that important as someone exploiting Bitmap.new is unlikely, but I just don't see a major downside in making sure a string, which will be passed to a function that expects null-terminated strings, doesn't contain NUL

This actually sounds interesting. How would someone exploit Bitmap.new? The only exploit I've ever heard of was eg. ruby looking at a string with a rogue null, deciding it is "safe" and pass it to a C function, whereas with the null interpreted as the string end, it would no longer be safe. In RGSS, ruby never does anything or looks at the string arguments, they're passed straight to the C++ core.

> Using StringValueCStr would solve both issues, wouldn't it? str is on the stack and the bug is fixed. Oh, so you mean using this function instead of your `objAsStringPtr` then? Anyway, that's okay, if it makes the code a bit simpler I don't really mind the difference in behavior between RMVX/A and mkxp as it's unlikely to come up. I just don't want to have more code for error checking which RGSS never does in the first place. > Okay, as I've said it's not that important as someone exploiting Bitmap.new is unlikely, but I just don't see a major downside in making sure a string, which will be passed to a function that expects null-terminated strings, doesn't contain NUL This actually sounds interesting. How would someone exploit Bitmap.new? The only exploit I've ever heard of was eg. ruby looking at a string with a rogue null, deciding it is "safe" and pass it to a C function, whereas with the null interpreted as the string end, it would no longer be safe. In RGSS, ruby never does anything or looks at the string arguments, they're passed straight to the C++ core.

Ancurio commented

2014-08-31 12:06:41 +00:00

(Migrated from github.com)

Anyway, I'll just pull this first as the patch itself is fine. Thanks for your work!