Fix use-after-free by mrb_gc_unregistor()#6390
Merged
matz merged 1 commit intomruby:masterfrom Oct 23, 2024
Merged
Conversation
Calling `mrb_gc_unregistor()` from `mrb_data_type::dfree` caused a use-after-free deep inside `mrb_close()`. The impetus to investigate was <mruby#6342 (review)>. Currently, when `mrb_close()` is called, all objects are destroyed first. The process is done heap page by heap page, and when all objects belonging to a heap page are destroyed, the heap page is released. If the next heap page contains `RData` objects, the `mrb_gc_unregistor()` function may be called from the `mrb_data_type::dfree` function. At this time, the `mrb_gc_unregistor()` function gets an array object from a Ruby global variable. If the array object belongs to a freed heap page, use-after-free is established by referencing this array object. About the fixes. First of all, there is the fact that the `mrb_gv_get()` function returns `nil` if `mrb->globals` is `NULL`. Therefore, before destroying all objects, free `mrb->globals` and set `mrb->globals` to `NULL` at the same time. Now the `mrb_gv_get()` function will return `nil` to the calling `mrb_gc_unregistor()` function and `mrb_gc_unregistor()` will do nothing more. ref. mruby#4618
Contributor
Author
|
Reproduction code: #if 0
#!/bin/sh
set -e
$(build/host/bin/mruby-config --cc --cflags) "$0" $(build/host/bin/mruby-config --ldflags --libs)
valgrind -- ./a.out
#lldb19 -- ./a.out
exit 0
#endif
#include <mruby.h>
#include <mruby/array.h>
#include <mruby/data.h>
#ifndef MRB_HEAP_PAGE_SIZE
# define MRB_HEAP_PAGE_SIZE 1024
#endif
struct mydata
{
mrb_value ary;
};
static void
mydata_free(mrb_state *mrb, void *opaque)
{
if (opaque) {
struct mydata *p = (struct mydata *)opaque;
mrb_gc_unregister(mrb, p->ary);
mrb_free(mrb, p);
}
}
static const mrb_data_type mydata_type = { "mydata", mydata_free };
int
main(int argc, char *argv[])
{
mrb_state *mrb = mrb_open();
struct RData *mydata_obj = mrb_data_object_alloc(mrb, mrb->object_class, NULL, NULL);
mydata_obj->data = mrb_calloc(mrb, 1, sizeof(struct mydata));
mydata_obj->type = &mydata_type;
struct mydata *p = (struct mydata *)mydata_obj->data;
p->ary = mrb_ary_new(mrb);
int ai = mrb_gc_arena_save(mrb);
// RData オブジェクトと "_gc_root_" グローバル変数オブジェクトを別のヒープページに確保する
for (int i = 0; i < MRB_HEAP_PAGE_SIZE * 2; i++) {
mrb_ary_push(mrb, p->ary, mrb_ary_new(mrb));
mrb_gc_arena_restore(mrb, ai);
}
mrb_gc_register(mrb, p->ary);
mrb_close(mrb);
return 0;
}Run through % sh ./uaf-gc-unregister.c
==24341== Memcheck, a memory error detector
==24341== Copyright (C) 2002-2024, and GNU GPL'd, by Julian Seward et al.
==24341== Using Valgrind-3.23.0 and LibVEX; rerun with -h for copyright info
==24341== Command: ./a.out
==24341==
==24341== Invalid read of size 1
==24341== at 0x26F3F7: mrb_gc_unregister (gc.c:452)
==24341== by 0x25F25C: mydata_free (uaf-gc-unregister.c:32)
==24341== by 0x26FD96: obj_free (gc.c:850)
==24341== by 0x26F1B8: free_heap (gc.c:359)
==24341== by 0x26F1B8: mrb_gc_destroy (gc.c:368)
==24341== by 0x27D892: mrb_close (state.c:188)
==24341== by 0x25F234: main (uaf-gc-unregister.c:60)
==24341== Address 0x54df090 is 10,800 bytes inside a block of size 49,184 free'd
==24341== at 0x484F2EC: free (vg_replace_malloc.c:993)
==24341== by 0x27DD48: mrb_default_allocf (allocf.c:22)
==24341== by 0x26F16E: mrb_free (gc.c:259)
==24341== by 0x26F16E: free_heap (gc.c:361)
==24341== by 0x26F16E: mrb_gc_destroy (gc.c:368)
==24341== by 0x27D892: mrb_close (state.c:188)
==24341== by 0x25F234: main (uaf-gc-unregister.c:60)
==24341== Block was alloc'd at
==24341== at 0x4852321: realloc (vg_replace_malloc.c:1806)
==24341== by 0x26EFDD: mrb_realloc_simple (gc.c:197)
==24341== by 0x26EFDD: mrb_realloc (gc.c:211)
==24341== by 0x26EFDD: mrb_malloc (gc.c:227)
==24341== by 0x26EFDD: mrb_calloc (gc.c:245)
==24341== by 0x26EFDD: add_heap (gc.c:299)
==24341== by 0x26EDA5: mrb_obj_alloc (gc.c:506)
==24341== by 0x25F331: ary_new_capa (array.c:48)
==24341== by 0x25F331: mrb_ary_new_capa (array.c:65)
==24341== by 0x25F331: mrb_ary_new (array.c:72)
==24341== by 0x25F207: main (uaf-gc-unregister.c:54)
==24341==
==24341==
==24341== HEAP SUMMARY:
==24341== in use at exit: 0 bytes in 0 blocks
==24341== total heap usage: 729 allocs, 729 frees, 261,916 bytes allocated
==24341==
==24341== All heap blocks were freed -- no leaks are possible
==24341==
==24341== For lists of detected and suppressed errors, rerun with: -s
==24341== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Calling
mrb_gc_unregistor()frommrb_data_type::dfreecaused a use-after-free deep insidemrb_close().The impetus to investigate was #6342 (review).
Currently, when
mrb_close()is called, all objects are destroyed first.The process is done heap page by heap page, and when all objects belonging to a heap page are destroyed, the heap page is released.
If the next heap page contains
RDataobjects, themrb_gc_unregistor()function may be called from themrb_data_type::dfreefunction.At this time, the
mrb_gc_unregistor()function gets an array object from a Ruby global variable.If the array object belongs to a freed heap page, use-after-free is established by referencing this array object.
About the fixes.
First of all, there is the fact that the
mrb_gv_get()function returnsnilifmrb->globalsisNULL.Therefore, before destroying all objects, free
mrb->globalsand setmrb->globalstoNULLat the same time.Now the
mrb_gv_get()function will returnnilto the callingmrb_gc_unregistor()function andmrb_gc_unregistor()will do nothing more.ref. #4618