Overflow #7

Merged
merged 2 commits into from Jun 11, 2017

Conversation

Projects
None yet
2 participants
Contributor

msantos commented Apr 28, 2012

Hey Mazen, this is the same set of patches I sent you about a year ago.

I've split them so you can just pull in the fix for the ei_decode_string() usage.

The fix for the stack overflow still uses malloc/free. Alternatively, the patch could check the size of the string and return an error, if the string is too large (no idea what a good size for that would be).

msantos added some commits Apr 28, 2012

@msantos msantos Fix buffer overflow
Increase the buffer size for strings by one byte to accomodate the
trail NULL expected by ei_decode_string:

    The string is copied to p, and enough space must be allocated. The
    returned string is null terminated so you need to add an extra byte
    to the memory requirement.
8c6d81f
@msantos msantos Prevent stack overflow
Large strings can overflow the stack:

    crash() ->
        cecho:start(0,0),
        S = lists:duplicate(8192*1024*2, "x"),
        cecho:mvaddstr(0,0,S).

Fix the problem by putting the data on the heap.
a70554c
Contributor

msantos commented Apr 28, 2012

The code to crash the VM is hidden in the code commit:

-module(crash).
-export([crash/0]).

% erl -noinput -pa ../cecho/ebin/ -eval 'crash:crash()' +A 50
crash() ->
cecho:start(0,0),
S = lists:duplicate(8192_1024_2, "x"),
cecho:mvaddstr(0,0,S).

Owner

mazenharake commented Apr 28, 2012

Hi Michael,

Thanks for the fixes, I'll have a look as soon as I got some time and test it out a little bit. Out of curiosity, why would you want to print that large of a string? It won't even fit the screen?! From an ncurses perspective it wouldn't make sense... is there any other reason why this should be moved to heap?

Owner

mazenharake commented Apr 28, 2012

Oh and sorry if you sent this a year ago and I didn't do anything about it, it was a hectic period at that time as well :)

Contributor

msantos commented Apr 28, 2012

Oh an sorry if you sent this a year ago and I didn't do anything about
it, it was a hectic period at that time as well :)

No problem! Just going through my unfinished projects and figured I'd
put this here in case you wanted to look at it in the future.

Thanks for the fixes, I'll have a look as soon as I got some time and
test it out a little bit. Out of curiosity, why would you want to print
that large of a string? It won't even fit the screen?!

Purely from a perspective of safety and correctness. Erlang code
shouldn't be able to crash the VM.

From an ncurses
perspective it wouldn't make sense...

True, but ncurses behaves well when passed a string of that size.

is there any other reason why this
should be moved to heap?

I split the patch so you could fix it in another way if you wanted. The
use of ei_decode_string() is obviously incorrect.

For example, the code could arbitrarily decide on maximum string length,
avoiding the dynamic allocation on the stack:

diff --git a/c_src/cecho.c b/c_src/cecho.c
index 850d1a2..f93f879 100644
--- a/c_src/cecho.c
+++ b/c_src/cecho.c
@@ -26,6 +26,8 @@
 #include "ncurses.h"
 #include "assert.h"

+#define MAXBUFSZ    1024
+
 #if ERL_DRV_EXTENDED_MAJOR_VERSION < 2
 #define ErlDrvSizeT int
 #define ErlDrvSSizeT int
@@ -224,9 +226,13 @@ void do_addch(state *st) {
 void do_addstr(state *st) {
   int arity;
   long strlen;
+  char str[MAXBUFSZ];
   ei_decode_tuple_header(st->args, &(st->index), &arity);
   ei_decode_long(st->args, &(st->index), &strlen);
-  char str[strlen];
+  if (strlen >= sizeof(str)) {
+    encode_ok_reply(st, ENOMEM);
+    return;
+  }
   ei_decode_string(st->args, &(st->index), str);
   encode_ok_reply(st, addnstr(str, strlen));
 }
@@ -340,11 +346,15 @@ void do_mvaddch(state *st) {
 void do_mvaddstr(state *st) {
   int arity;
   long strlen, y, x;
+  char str[MAXBUFSZ];
   ei_decode_tuple_header(st->args, &(st->index), &arity);
   ei_decode_long(st->args, &(st->index), &y);
   ei_decode_long(st->args, &(st->index), &x);
   ei_decode_long(st->args, &(st->index), &strlen);
-  char str[strlen];
+  if (strlen >= MAXBUFSZ) {
+    encode_ok_reply(st, ENOMEM);
+    return;
+  }
   ei_decode_string(st->args, &(st->index), str);
   encode_ok_reply(st, mvaddnstr((int)y, (int)x, str, (int)strlen));
 }
@@ -393,10 +403,14 @@ void do_wmove(state *st) {
 void do_waddstr(state *st) {
   int arity;
   long slot, strlen;
+  char str[MAXBUFSZ];
   ei_decode_tuple_header(st->args, &(st->index), &arity);
   ei_decode_long(st->args, &(st->index), &slot);
   ei_decode_long(st->args, &(st->index), &strlen);
-  char str[strlen];
+  if (strlen >= sizeof(str)) {
+    encode_ok_reply(st, ENOMEM);
+    return;
+  }
   ei_decode_string(st->args, &(st->index), str);
   encode_ok_reply(st, waddnstr(st->win[slot], str, strlen));
 }
@@ -414,12 +428,16 @@ void do_waddch(state *st) {
 void do_mvwaddstr(state *st) {
   int arity;
   long slot, y, x, strlen;
+  char str[MAXBUFSZ];
   ei_decode_tuple_header(st->args, &(st->index), &arity);
   ei_decode_long(st->args, &(st->index), &slot);
   ei_decode_long(st->args, &(st->index), &y);
   ei_decode_long(st->args, &(st->index), &x);
   ei_decode_long(st->args, &(st->index), &strlen);
-  char str[strlen];
+  if (strlen > sizeof(str)) {
+    encode_ok_reply(st, ENOMEM);
+    return;
+  }
   ei_decode_string(st->args, &(st->index), str);
   encode_ok_reply(st, mvwaddnstr(st->win[slot], (int)y, (int)x, str, strlen));
 }

@mazenharake mazenharake merged commit a70554c into mazenharake:master Jun 11, 2017

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