{"action":"created","issue":{"url":"https://api.github.com/repos/antirez/redis/issues/2855","labels_url":"https://api.github.com/repos/antirez/redis/issues/2855/labels{/name}","comments_url":"https://api.github.com/repos/antirez/redis/issues/2855/comments","events_url":"https://api.github.com/repos/antirez/redis/issues/2855/events","html_url":"https://github.com/antirez/redis/issues/2855","id":115403423,"number":2855,"title":"Integer overflow (leading to stack-based buffer overflow) in embedded lua_struct.c","user":{"login":"lucab","id":98086,"avatar_url":"https://avatars.githubusercontent.com/u/98086?v=3","gravatar_id":"","url":"https://api.github.com/users/lucab","html_url":"https://github.com/lucab","followers_url":"https://api.github.com/users/lucab/followers","following_url":"https://api.github.com/users/lucab/following{/other_user}","gists_url":"https://api.github.com/users/lucab/gists{/gist_id}","starred_url":"https://api.github.com/users/lucab/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lucab/subscriptions","organizations_url":"https://api.github.com/users/lucab/orgs","repos_url":"https://api.github.com/users/lucab/repos","events_url":"https://api.github.com/users/lucab/events{/privacy}","received_events_url":"https://api.github.com/users/lucab/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/antirez/redis/labels/scripting","name":"scripting","color":"aaaaaa"}],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":11,"created_at":"2015-11-05T23:56:32Z","updated_at":"2015-12-14T15:37:11Z","closed_at":null,"body":"[re-posting via github after private reporting, as agreed with antirez]\r\n\r\nEmbedded copy of lua_struct.c suffers of an integer overflow in the `getnum()` parser that can be used to trigger (at least) a stack-based buffer overflow.\r\nThis affects all released versions of redis in both 2.8 and 3.0 branches.\r\n\r\nThe following code is part of `lua_struct.c`\r\n```C\r\nstatic int getnum (const char **fmt, int df) {\r\n  if (!isdigit(**fmt))  /* no number? */\r\n    return df;  /* return default value */\r\n  else {\r\n    int a = 0;\r\n    do {\r\n      a = a*10 + *((*fmt)++) - '0';\r\n    } while (isdigit(**fmt));\r\n    return a;\r\n  }\r\n}\r\n\r\nstatic size_t optsize (lua_State *L, char opt, const char **fmt) {\r\n  switch (opt) {\r\n[...]\r\n    case 'c': return getnum(fmt, 1);\r\n    case 'i': case 'I': {\r\n      int sz = getnum(fmt, sizeof(int));\r\n      if (sz > MAXINTSIZE)\r\n        luaL_error(L, \"integral size %d is larger than limit of %d\",\r\n                       sz, MAXINTSIZE);\r\n      return sz;\r\n    }\r\n    default: return 0;  /* other cases do not need alignment */\r\n  }\r\n}\r\n```\r\n\r\n`getnum()` can be tricked into an integer wraparound with a large size number as input, thus returning a negative value.\r\n`optsize()` has no lower bound/negative check; moreover, there is an implicit `int` -> `size_t` promotion, yielding a very large (unsigned) size value.\r\n\r\nThis, plus further `int`/`size_t` confusion in the whole module, results in stack-based buffer overflows in other places,\r\neg. `putinteger()` reachable in LUA via `struct.pack()`.\r\n\r\nSimple PoC as follow:\r\n```\r\n  EVAL \"struct.pack('>I2147483648', '10')\" 0\r\n```\r\n\r\nWhere:\r\n * '2147483648' is a user-controlled index, larger than `MAX_INT32`, fooling `optsize()`\r\n * '>I' is there to reach a buffer overflow in `putinteger()`\r\n * '10' is a user-controlled input stored into `value`\r\n\r\nThis will result in memory corruption due a user-controlled write outside of a (stack-based) array. Running the PoC above, this can be observed from gdb:\r\n```\r\n171 char buff[32];\r\n...\r\n185         for (i = size - 1; i >= 0; i--) {\r\n186           buff[i] = (value & 0xff);\r\n(gdb) print i\r\n$3 = 2147483647\r\n```"},"comment":{"url":"https://api.github.com/repos/antirez/redis/issues/comments/164469600","html_url":"https://github.com/antirez/redis/issues/2855#issuecomment-164469600","issue_url":"https://api.github.com/repos/antirez/redis/issues/2855","id":164469600,"user":{"login":"sunheehnus","id":2889804,"avatar_url":"https://avatars.githubusercontent.com/u/2889804?v=3","gravatar_id":"","url":"https://api.github.com/users/sunheehnus","html_url":"https://github.com/sunheehnus","followers_url":"https://api.github.com/users/sunheehnus/followers","following_url":"https://api.github.com/users/sunheehnus/following{/other_user}","gists_url":"https://api.github.com/users/sunheehnus/gists{/gist_id}","starred_url":"https://api.github.com/users/sunheehnus/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sunheehnus/subscriptions","organizations_url":"https://api.github.com/users/sunheehnus/orgs","repos_url":"https://api.github.com/users/sunheehnus/repos","events_url":"https://api.github.com/users/sunheehnus/events{/privacy}","received_events_url":"https://api.github.com/users/sunheehnus/received_events","type":"User","site_admin":false},"created_at":"2015-12-14T15:37:11Z","updated_at":"2015-12-14T15:37:11Z","body":"Hello @lucab I came to the same conclusion with you. :-)\r\nBut your fix imports some side effects on \"c\" format string(limit the chars within 29)\r\n\r\n***\r\n#### Here is the test case:\r\nthe original version of redis-lua\r\n```c\r\n127.0.0.1:1111> EVAL \"return struct.pack('>c48',\r\n'111111111111111111111111111111111111111111111111111111')\" 0\r\n\"111111111111111111111111111111111111111111111111\"\r\n```\r\n\r\nthe applied-your-patch version\r\n```c\r\n127.0.0.1:1111> EVAL \"return struct.pack('>c48',\r\n'111111111111111111111111111111111111111111111111111111')\" 0\r\n(error) ERR Error running script (call to\r\nf_e9c2d0f007a691b067c1f2f320be6b157de3e79c): @user_script:1:\r\nuser_script:1: bad argument #1 to 'pack' (invalid format option '8')\r\n```\r\n\r\nAnd it is already fixed in https://github.com/antirez/redis/pull/2943 :-)"}}