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

Incorrect code generation when result of NODE_NEGATE is not used #3348

Closed
clayton-shopify opened this Issue Dec 16, 2016 · 0 comments

Comments

Projects
None yet
1 participant
@clayton-shopify
Contributor

clayton-shopify commented Dec 16, 2016

The following report was submitted by https://hackerone.com/dkasak:

Introduction

Not using the result of NODE_NEGATE leads to incorrect code generation which could possibly result in arbitrary bytecode generation. Currently it is possible to produce a crash through a SIGABRT via an assert failure.

Proof of concept

assert_failure.rb

-0E00;0
  1. Run either:

    a. mruby assert_failure.rb
    b. sandbox assert_failure.rb

  2. Both cause a crash via an assert failure.

Discussion

The flaw was introduced in commit d56a19c.

The problem is that codegen is called recursively on the argument of NODE_NEGATE without checking whether it is a valid node. This is problematic because codegen assumes that its tree argument is a valid node (i.e. that the node's type is stored under the car member, further nodes or data under the cdr member and that filename_index and lineno are set properly). This creates an opportunity for the attacker to control further code generation, though it might not be possible to place a valid node type in car with the current codebase. Nevertheless, filename_index can be controlled and this is what allows the POC to make the assertion on line 135 of debug.c fail:

ruby: ext/mruby_engine/mruby/src/debug.c:135: mrb_debug_info_append_file: Assertion `irep->filename' failed.
zsh: abort      mruby-engine/bin/sandbox assert_failure.rb

Running mruby with the following input in gdb:

-0.5555555555555555555;0

And breaking on parse.y:2885, we can see that filename_index is set to 13621 (which is two '5' characters interpreted as an integer):

(gdb) x/16hd yyvsp[0].nd->cdr
0x79162c:       11824   13621   13621   13621   13621   13621   13621   13621
0x79163c:       13621   13621   53      0       52      0       0       0
(gdb) p yyvsp[0].nd->cdr->filename_index 
$21 = 13621

Then, breaking codegen.c:1230, we see that tree->filename_index is indeed set to 13621:

(gdb) p tree->filename_index
$22 = 13621

This eventually leads to assert to fail.

Solution

To fix the problem, we should ensure tree->car is a valid node before calling codegen on tree in the case of NODE_NEGATE.

negate_node.patch

diff --git a/mrbgems/mruby-compiler/core/codegen.c b/mrbgems/mruby-compiler/core/codegen.c
index 90bafb3..9d47cca 100644
--- a/mrbgems/mruby-compiler/core/codegen.c
+++ b/mrbgems/mruby-compiler/core/codegen.c
@@ -2232,7 +2232,9 @@ codegen(codegen_scope *s, node *tree, int val)
    nt = (intptr_t)tree->car;
    tree = tree->cdr;
    if (!val) {
-        codegen(s, tree, NOVAL);
+        if (tree && (intptr_t)tree->car < NODE_LAST) {
+          codegen(s, tree, NOVAL);
+        }
        break;
    }
    switch (nt) {

--
Denis Kasak

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