Merge pull request #410 from Watson1978/fix/buffer-append-string-uaf

Fix use-after-free when packing a string into an IO

Author: byroot

ext/msgpack/buffer.h

@@ -237,14 +237,27 @@ void _msgpack_buffer_append_long_string(msgpack_buffer_t* b, VALUE string);
 
 static inline size_t msgpack_buffer_append_string(msgpack_buffer_t* b, VALUE string)
 {
-    size_t length;
-    char *ptr;
-    RSTRING_GETMEM(string, ptr, length);
+    size_t length = RSTRING_LEN(string);
 
     if(length > b->write_reference_threshold) {
         _msgpack_buffer_append_long_string(b, string);
-    } else {
-        msgpack_buffer_append(b, ptr, length);
+    } else if(length > 0) {
+        /* Making room can run io.write or trigger GC, either of which may free
+         * or shrink the string, so reserve the space first and only then read
+         * its pointer. */
+        msgpack_buffer_ensure_writable(b, length);
+        char *ptr;
+        size_t current;
+        RSTRING_GETMEM(string, ptr, current);
+        if(current > length) {
+            current = length;
+        }
+        memcpy(b->tail.last, ptr, current);
+        if(current < length) {
+            memset(b->tail.last + current, 0, length - current);
+        }
+        b->tail.last += length;
+        RB_GC_GUARD(string);
     }
 
     return length;

spec/packer_spec.rb

@@ -616,4 +616,32 @@ def to_msgpack_ext
     packer.write(top_hash)
     packer.full_pack
   end
+
+  it 'does not copy freed memory when a string is freed during io.write' do
+    skip if IS_JRUBY
+
+    size = 100_000 # larger than the io buffer, smaller than the write reference threshold
+    src = "A".b * size
+    captured = "".b
+    flipped = false
+    io = Object.new
+    io.define_singleton_method(:write) do |data|
+      unless flipped
+        flipped = true
+        src.replace("".b)                 # free the buffer the packer still points at
+        GC.start
+        Array.new(20) { "Z".b * size }    # reuse the freed region
+      end
+      captured << data.b
+      data.bytesize
+    end
+
+    packer = MessagePack::Packer.new(io)
+    packer.write(src)
+    packer.full_pack
+
+    body = MessagePack.unpack(captured)
+    expect(body.bytesize).to eq(size)
+    expect(body.b.count("Z".b)).to eq(0)  # must not surface the reused heap region
+  end
 end