search

Perl / perl5

🐪 The Perl programming language
https://dev.perl.org/perl5/
Other
1.92k stars 550 forks source link

(probably exploitable) crash bug in Perl5 (v5.21.9-73-gd98e5cd) #14565

Closed p5pRT closed 9 years ago

p5pRT commented 9 years ago

Migrated from rt.perl.org#123996 (status was 'resolved')

Searchable as RT123996$

p5pRT commented 9 years ago

From @geeknik

I believe I have found an exploitable crash bug in Perl v5.21.10 (v5.21.9-73-gd98e5cd). I built it with the following command line​: ./Configure -des -Dusedevel -DDEBUGGING -Dcc=afl-gcc -Doptimize=-O2\ -g && AFL_HARDEN=1 make -j12 test-prep

This bug was found with help from AFL (http​://lcamtuf.coredump.cx/afl). This crash doesn't seem to affect the current version installed on Debian 7 (v5.21.7 (v5.21.6-602-ge9d2bd8)) as I just get an "Out of memory during list extend at test06-min line 1." Same with Perl5 v5.14.2 on Debian and v5.18.4 on FreeBSD.

This is output from the original test case (test06\, attached)​: gdb-peda$ file ~/perl/perl gdb-peda$ set args test06 gdb-peda$ r [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1". Smartmatch is experimental at test06 line 1. Use of literal control characters in variable names is deprecated at test06 line 1.

Program received signal SIGSEGV\, Segmentation fault. [----------------------------------registers-----------------------------------] RAX​: 0x11f0b78 --> 0x0 RBX​: 0x8 RCX​: 0x1fff8 RDX​: 0x1fef8 RSI​: 0x11f0b70 --> 0x11f1688 --> 0x11cef80 --> 0x11cef70 --> 0x11cefa0 --> 0x11cefb0 --> 0x11cef40 --> 0x11cef60 --> 0x11cefd0 --> 0x11cefe0 --> 0x11ceff0 --> 0x11cf000 --> 0x11cf010 --> 0x11cf020 --> 0x11cf030 --> 0x11cf040 --> 0x11cf050 --> 0x11cf060 --> 0x11cf070 --> 0x11cf080 --> 0x11cf090 --> 0x11cf0a0 --> 0x11cf0b0 --> 0x11cf0c0 --> 0x11cf0d0 --> 0x11cf0e0 --> 0x11cf0f0 --> 0x11cf100 --> 0x11cf110 --> 0x11cf120 --> 0x11cf130 --> 0x11cf140 --> 0x11cf150 --> 0x11cf160 --> 0x11cf170 --> 0x11cf180 --> 0x11cf190 --> 0x11cf1a0 --> 0x11cf1b0 --> 0x11cf1c0 --> 0x11cf1d0 --> 0x11cf1e0 --> 0x11cf1f0 --> 0x11cf200 --> 0x11cf210 --> 0x11cf220 --> 0x11cf230 --> 0x11cf240 --> 0x11cf250 --> 0x11cf260 --> 0x11cf270 --> 0x11cf280 --> 0x11cf290 --> 0x11cf2a0 --> 0x11cf2b0 --> 0x11cf2c0 --> 0x11cf2d0 --> 0x11cf2e0 --> 0x11cf2f0 --> 0x11cf300 --> 0x11cf310 --> 0x11cf320 --> 0x11cf330 --> 0x11cf340 --> 0x11cf350 --> 0x11cf360 --> 0x11cf370 --> 0x11cf380 --> 0x11cf390 --> 0x11cf3a0 --> 0x11cf3b0 --> 0x11cf3c0 --> 0x11cf3d0 --> 0x11cf3e0 --> 0x11cf3f0 --> 0x11cf400 --> 0x11cf410 --> 0x11cf420 --> 0x11cf430 --> 0x11cf440 --> 0x11cf450 --> 0x11cf460 --> 0x11cf470 --> 0x11cf480 --> 0x11cf490 --> 0x11cf4a0 --> 0x11cf4b0 --> 0x11cf4c0 --> 0x11cf4d0 --> 0x11cf4e0 --> 0x11cf4f0 --> 0x11cf500 --> 0x11cf510 --> 0x11cf520 --> 0x11cf530 --> 0x11cf540 --> 0x11cf550 --> 0x11cf560 --> 0x11cf570 --> 0x11cf580 --> 0x11cf590 --> 0x11cf5a0 --> 0x11cf5b0 --> 0x11cf5c0 --> 0x11cf5d0 --> 0x11cf5e0 --> 0x11cf5f0 --> 0x11cf600 --> 0x11cf610 --> 0x11cf620 --> 0x11cf630 --> 0x11cf640 --> 0x11cf650 --> 0x11cf660 --> 0x11cf670 --> 0x11cf680 --> 0x11cf690 --> 0x11cf6a0 --> 0x11cf6b0 --> 0x11cf6c0 --> 0x11cf6d0 --> 0x11cf6e0 --> 0x11cf6f0 --> 0x11cf700 --> 0x11cf710 --> 0x11cf720 --> 0x11cf730 --> 0x11cf740 --> 0x11cf750 --> 0x11cf760 --> 0x11cf770 --> 0x11cf780 --> 0x11cf790 --> 0x11cf7a0 --> 0x11cf7b0 --> 0x11cf7c0 --> 0x11cf7d0 --> 0x11cf7e0 --> 0x11cf7f0 --> 0x11cf800 --> 0x11cf810 --> 0x11cf820 --> 0x11cf830 --> 0x11cf840 --> 0x11cf850 --> 0x11cf860 --> 0x11cf870 --> 0x11cf880 --> 0x11cf890 --> 0x11cf8a0 --> 0x11cf8b0 --> 0x11cf8c0 --> 0x11cf8d0 --> 0x11cf8e0 --> 0x11cf8f0 --> 0x11cf900 --> 0x11cf910 --> 0x11cf920 --> 0x11cf930 --> 0x11cf940 --> 0x11cf950 --> 0x11cf960 --> 0x11cf970 --> 0x11cf980 --> 0x11cf990 --> 0x11cf9a0 --> 0x11cf9b0 --> 0x11cf9c0 --> 0x11cf9d0 --> 0x11cf9e0 --> 0x11cf9f0 --> 0x11cfa00 --> 0x11cfa10 --> 0x11cfa20 --> 0x11cfa30 --> 0x11cfa40 --> 0x11cfa50 --> 0x11cfa60 --> 0x11cfa70 --> 0x11cfa80 --> 0x11cfa90 --> 0x11cfaa0 --> 0x11cfab0 --> 0x11cfac0 --> 0x11cfad0 --> 0x0 RDI​: 0x1210b70 RBP​: 0x1 RSP​: 0x7fffffffe058 --> 0x7ef768 (\<Perl_repeatcpy+5016>​: mov rcx\,rax) RIP​: 0x7ffff6e88498 (\<__memcpy_ssse3_back+7016>​: movdqa XMMWORD PTR [rdi-0x10]\,xmm1) R8 : 0x1210b68 R9 : 0x8 R10​: 0x4 R11​: 0x7ffff6eb3290 --> 0xfffd6150fffd5ace R12​: 0x3fffffffffffffff R13​: 0x20000 R14​: 0x8000 R15​: 0x11d0b78 --> 0x11f1688 --> 0x11cef80 --> 0x11cef70 --> 0x11cefa0 --> 0x11cefb0 --> 0x11cef40 --> 0x11cef60 --> 0x11cefd0 --> 0x11cefe0 --> 0x11ceff0 --> 0x11cf000 --> 0x11cf010 --> 0x11cf020 --> 0x11cf030 --> 0x11cf040 --> 0x11cf050 --> 0x11cf060 --> 0x11cf070 --> 0x11cf080 --> 0x11cf090 --> 0x11cf0a0 --> 0x11cf0b0 --> 0x11cf0c0 --> 0x11cf0d0 --> 0x11cf0e0 --> 0x11cf0f0 --> 0x11cf100 --> 0x11cf110 --> 0x11cf120 --> 0x11cf130 --> 0x11cf140 --> 0x11cf150 --> 0x11cf160 --> 0x11cf170 --> 0x11cf180 --> 0x11cf190 --> 0x11cf1a0 --> 0x11cf1b0 --> 0x11cf1c0 --> 0x11cf1d0 --> 0x11cf1e0 --> 0x11cf1f0 --> 0x11cf200 --> 0x11cf210 --> 0x11cf220 --> 0x11cf230 --> 0x11cf240 --> 0x11cf250 --> 0x11cf260 --> 0x11cf270 --> 0x11cf280 --> 0x11cf290 --> 0x11cf2a0 --> 0x11cf2b0 --> 0x11cf2c0 --> 0x11cf2d0 --> 0x11cf2e0 --> 0x11cf2f0 --> 0x11cf300 --> 0x11cf310 --> 0x11cf320 --> 0x11cf330 --> 0x11cf340 --> 0x11cf350 --> 0x11cf360 --> 0x11cf370 --> 0x11cf380 --> 0x11cf390 --> 0x11cf3a0 --> 0x11cf3b0 --> 0x11cf3c0 --> 0x11cf3d0 --> 0x11cf3e0 --> 0x11cf3f0 --> 0x11cf400 --> 0x11cf410 --> 0x11cf420 --> 0x11cf430 --> 0x11cf440 --> 0x11cf450 --> 0x11cf460 --> 0x11cf470 --> 0x11cf480 --> 0x11cf490 --> 0x11cf4a0 --> 0x11cf4b0 --> 0x11cf4c0 --> 0x11cf4d0 --> 0x11cf4e0 --> 0x11cf4f0 --> 0x11cf500 --> 0x11cf510 --> 0x11cf520 --> 0x11cf530 --> 0x11cf540 --> 0x11cf550 --> 0x11cf560 --> 0x11cf570 --> 0x11cf580 --> 0x11cf590 --> 0x11cf5a0 --> 0x11cf5b0 --> 0x11cf5c0 --> 0x11cf5d0 --> 0x11cf5e0 --> 0x11cf5f0 --> 0x11cf600 --> 0x11cf610 --> 0x11cf620 --> 0x11cf630 --> 0x11cf640 --> 0x11cf650 --> 0x11cf660 --> 0x11cf670 --> 0x11cf680 --> 0x11cf690 --> 0x11cf6a0 --> 0x11cf6b0 --> 0x11cf6c0 --> 0x11cf6d0 --> 0x11cf6e0 --> 0x11cf6f0 --> 0x11cf700 --> 0x11cf710 --> 0x11cf720 --> 0x11cf730 --> 0x11cf740 --> 0x11cf750 --> 0x11cf760 --> 0x11cf770 --> 0x11cf780 --> 0x11cf790 --> 0x11cf7a0 --> 0x11cf7b0 --> 0x11cf7c0 --> 0x11cf7d0 --> 0x11cf7e0 --> 0x11cf7f0 --> 0x11cf800 --> 0x11cf810 --> 0x11cf820 --> 0x11cf830 --> 0x11cf840 --> 0x11cf850 --> 0x11cf860 --> 0x11cf870 --> 0x11cf880 --> 0x11cf890 --> 0x11cf8a0 --> 0x11cf8b0 --> 0x11cf8c0 --> 0x11cf8d0 --> 0x11cf8e0 --> 0x11cf8f0 --> 0x11cf900 --> 0x11cf910 --> 0x11cf920 --> 0x11cf930 --> 0x11cf940 --> 0x11cf950 --> 0x11cf960 --> 0x11cf970 --> 0x11cf980 --> 0x11cf990 --> 0x11cf9a0 --> 0x11cf9b0 --> 0x11cf9c0 --> 0x11cf9d0 --> 0x11cf9e0 --> 0x11cf9f0 --> 0x11cfa00 --> 0x11cfa10 --> 0x11cfa20 --> 0x11cfa30 --> 0x11cfa40 --> 0x11cfa50 --> 0x11cfa60 --> 0x11cfa70 --> 0x11cfa80 --> 0x11cfa90 --> 0x11cfaa0 --> 0x11cfab0 --> 0x11cfac0 --> 0x11cfad0 --> 0x0 EFLAGS​: 0x10202 (carry parity adjust zero sign trap INTERRUPT direction overflow) [-------------------------------------code-------------------------------------]   0x7ffff6e88488 \<__memcpy_ssse3_back+7000>​: movdqu xmm6\,XMMWORD PTR [rsi-0x60]   0x7ffff6e8848d \<__memcpy_ssse3_back+7005>​: movdqu xmm7\,XMMWORD PTR [rsi-0x70]   0x7ffff6e88492 \<__memcpy_ssse3_back+7010>​: movdqu xmm8\,XMMWORD PTR [rsi-0x80] => 0x7ffff6e88498 \<__memcpy_ssse3_back+7016>​: movdqa XMMWORD PTR [rdi-0x10]\,xmm1   0x7ffff6e8849d \<__memcpy_ssse3_back+7021>​: movdqa XMMWORD PTR [rdi-0x20]\,xmm2   0x7ffff6e884a2 \<__memcpy_ssse3_back+7026>​: movdqa XMMWORD PTR [rdi-0x30]\,xmm3   0x7ffff6e884a7 \<__memcpy_ssse3_back+7031>​: movdqa XMMWORD PTR [rdi-0x40]\,xmm4   0x7ffff6e884ac \<__memcpy_ssse3_back+7036>​: movdqa XMMWORD PTR [rdi-0x50]\,xmm5 [------------------------------------stack-------------------------------------] 0000| 0x7fffffffe058 --> 0x7ef768 (\<Perl_repeatcpy+5016>​: mov rcx\,rax) 0008| 0x7fffffffe060 --> 0x4 0016| 0x7fffffffe068 --> 0x11d0b80 --> 0x11f1688 --> 0x11cef80 --> 0x11cef70 --> 0x11cefa0 --> 0x11cefb0 --> 0x11cef40 --> 0x11cef60 --> 0x11cefd0 --> 0x11cefe0 --> 0x11ceff0 --> 0x11cf000 --> 0x11cf010 --> 0x11cf020 --> 0x11cf030 --> 0x11cf040 --> 0x11cf050 --> 0x11cf060 --> 0x11cf070 --> 0x11cf080 --> 0x11cf090 --> 0x11cf0a0 --> 0x11cf0b0 --> 0x11cf0c0 --> 0x11cf0d0 --> 0x11cf0e0 --> 0x11cf0f0 --> 0x11cf100 --> 0x11cf110 --> 0x11cf120 --> 0x11cf130 --> 0x11cf140 --> 0x11cf150 --> 0x11cf160 --> 0x11cf170 --> 0x11cf180 --> 0x11cf190 --> 0x11cf1a0 --> 0x11cf1b0 --> 0x11cf1c0 --> 0x11cf1d0 --> 0x11cf1e0 --> 0x11cf1f0 --> 0x11cf200 --> 0x11cf210 --> 0x11cf220 --> 0x11cf230 --> 0x11cf240 --> 0x11cf250 --> 0x11cf260 --> 0x11cf270 --> 0x11cf280 --> 0x11cf290 --> 0x11cf2a0 --> 0x11cf2b0 --> 0x11cf2c0 --> 0x11cf2d0 --> 0x11cf2e0 --> 0x11cf2f0 --> 0x11cf300 --> 0x11cf310 --> 0x11cf320 --> 0x11cf330 --> 0x11cf340 --> 0x11cf350 --> 0x11cf360 --> 0x11cf370 --> 0x11cf380 --> 0x11cf390 --> 0x11cf3a0 --> 0x11cf3b0 --> 0x11cf3c0 --> 0x11cf3d0 --> 0x11cf3e0 --> 0x11cf3f0 --> 0x11cf400 --> 0x11cf410 --> 0x11cf420 --> 0x11cf430 --> 0x11cf440 --> 0x11cf450 --> 0x11cf460 --> 0x11cf470 --> 0x11cf480 --> 0x11cf490 --> 0x11cf4a0 --> 0x11cf4b0 --> 0x11cf4c0 --> 0x11cf4d0 --> 0x11cf4e0 --> 0x11cf4f0 --> 0x11cf500 --> 0x11cf510 --> 0x11cf520 --> 0x11cf530 --> 0x11cf540 --> 0x11cf550 --> 0x11cf560 --> 0x11cf570 --> 0x11cf580 --> 0x11cf590 --> 0x11cf5a0 --> 0x11cf5b0 --> 0x11cf5c0 --> 0x11cf5d0 --> 0x11cf5e0 --> 0x11cf5f0 --> 0x11cf600 --> 0x11cf610 --> 0x11cf620 --> 0x11cf630 --> 0x11cf640 --> 0x11cf650 --> 0x11cf660 --> 0x11cf670 --> 0x11cf680 --> 0x11cf690 --> 0x11cf6a0 --> 0x11cf6b0 --> 0x11cf6c0 --> 0x11cf6d0 --> 0x11cf6e0 --> 0x11cf6f0 --> 0x11cf700 --> 0x11cf710 --> 0x11cf720 --> 0x11cf730 --> 0x11cf740 --> 0x11cf750 --> 0x11cf760 --> 0x11cf770 --> 0x11cf780 --> 0x11cf790 --> 0x11cf7a0 --> 0x11cf7b0 --> 0x11cf7c0 --> 0x11cf7d0 --> 0x11cf7e0 --> 0x11cf7f0 --> 0x11cf800 --> 0x11cf810 --> 0x11cf820 --> 0x11cf830 --> 0x11cf840 --> 0x11cf850 --> 0x11cf860 --> 0x11cf870 --> 0x11cf880 --> 0x11cf890 --> 0x11cf8a0 --> 0x11cf8b0 --> 0x11cf8c0 --> 0x11cf8d0 --> 0x11cf8e0 --> 0x11cf8f0 --> 0x11cf900 --> 0x11cf910 --> 0x11cf920 --> 0x11cf930 --> 0x11cf940 --> 0x11cf950 --> 0x11cf960 --> 0x11cf970 --> 0x11cf980 --> 0x11cf990 --> 0x11cf9a0 --> 0x11cf9b0 --> 0x11cf9c0 --> 0x11cf9d0 --> 0x11cf9e0 --> 0x11cf9f0 --> 0x11cfa00 --> 0x11cfa10 --> 0x11cfa20 --> 0x11cfa30 --> 0x11cfa40 --> 0x11cfa50 --> 0x11cfa60 --> 0x11cfa70 --> 0x11cfa80 --> 0x11cfa90 --> 0x11cfaa0 --> 0x11cfab0 --> 0x11cfac0 --> 0x11cfad0 --> 0x0 0024| 0x7fffffffe070 --> 0x7ffffffffffffffe 0032| 0x7fffffffe078 --> 0x1000000000000000 0040| 0x7fffffffe080 --> 0x7 0048| 0x7fffffffe088 --> 0x7 0056| 0x7fffffffe090 --> 0x8 [------------------------------------------------------------------------------] Legend​: code\, data\, rodata\, value Stopped reason​: SIGSEGV __memcpy_ssse3_back () at ../sysdeps/x86_64/multiarch/memcpy-ssse3-back.S​:1664 1664 ../sysdeps/x86_64/multiarch/memcpy-ssse3-back.S​: No such file or directory. gdb-peda$ exploit Description​: Access violation on destination operand Short description​: DestAv (8/22) Hash​: 925c76a199d52d3ee63d7a002c765bfa.925c76a199d52d3ee63d7a002c765bfa Exploitability Classification​: EXPLOITABLE Explanation​: The target crashed on an access violation at an address matching the destination operand of the instruction. This likely indicates a write access violation\, which means the attacker may control the write address and/or value. Other tags​: AccessViolation (21/22)

Valgrind​: Smartmatch is experimental at test06 line 1. Use of literal control characters in variable names is deprecated at test06 line 1. ==44050== Invalid write of size 8 ==44050== at 0x4C2A7ED​: memcpy (mc_replace_strmem.c​:838) ==44050== by 0x7EF767​: Perl_repeatcpy (string3.h​:52) ==44050== by 0xA540FC​: Perl_pp_repeat (pp.c​:1741) ==44050== by 0x7CB49E​: Perl_runops_debug (dump.c​:2237) ==44050== by 0x53B4C8​: perl_run (perl.c​:2427) ==44050== by 0x42B167​: main (perlmain.c​:116) ==44050== Address 0x5eb4130 is not stack'd\, malloc'd or (recently) free'd ==44050== ==44050== Invalid read of size 8 ==44050== at 0x4C2A7E8​: memcpy (mc_replace_strmem.c​:838) ==44050== by 0x7EF767​: Perl_repeatcpy (string3.h​:52) ==44050== by 0xA540FC​: Perl_pp_repeat (pp.c​:1741) ==44050== by 0x7CB49E​: Perl_runops_debug (dump.c​:2237) ==44050== by 0x53B4C8​: perl_run (perl.c​:2427) ==44050== by 0x42B167​: main (perlmain.c​:116) ==44050== Address 0x5eb4130 is not stack'd\, malloc'd or (recently) free'd ==44050== ==44050== Invalid read of size 8 ==44050== at 0x4C2A7FA​: memcpy (mc_replace_strmem.c​:838) ==44050== by 0x7EF767​: Perl_repeatcpy (string3.h​:52) ==44050== by 0xA540FC​: Perl_pp_repeat (pp.c​:1741) ==44050== by 0x7CB49E​: Perl_runops_debug (dump.c​:2237) ==44050== by 0x53B4C8​: perl_run (perl.c​:2427) ==44050== by 0x42B167​: main (perlmain.c​:116) ==44050== Address 0x5eb4120 is not stack'd\, malloc'd or (recently) free'd ==44050== ==44050== ==44050== Process terminating with default action of signal 11 (SIGSEGV) ==44050== Access not within mapped region at address 0x62B3D30 ==44050== at 0x4C2A7ED​: memcpy (mc_replace_strmem.c​:838) ==44050== by 0x7EF767​: Perl_repeatcpy (string3.h​:52) ==44050== by 0xA540FC​: Perl_pp_repeat (pp.c​:1741) ==44050== by 0x7CB49E​: Perl_runops_debug (dump.c​:2237) ==44050== by 0x53B4C8​: perl_run (perl.c​:2427) ==44050== by 0x42B167​: main (perlmain.c​:116) ==44050== If you believe this happened as a result of a stack ==44050== overflow in your program's main thread (unlikely but ==44050== possible)\, you can try to increase the size of the ==44050== main thread stack using the --main-stacksize= flag. ==44050== The main thread stack size used in this run was 8388608. ==44050== Invalid free() / delete / delete[] / realloc() ==44050== at 0x4C27D4E​: free (vg_replace_malloc.c​:427) ==44050== by 0x5C5AD6B​: _nl_archive_subfreeres (in /lib/x86_64-linux-gnu/ libc-2.13.so) ==44050== by 0x5C5AA38​: free_mem (in /lib/x86_64-linux-gnu/libc-2.13.so) ==44050== by 0x5C5B1C1​: __libc_freeres (in /lib/x86_64-linux-gnu/ libc-2.13.so) ==44050== by 0x4A226EC​: _vgnU_freeres (vg_preloaded.c​:61) ==44050== by 0x4​: ??? ==44050== by 0x7EF767​: Perl_repeatcpy (string3.h​:52) ==44050== by 0xA540FC​: Perl_pp_repeat (pp.c​:1741) ==44050== by 0x7CB49E​: Perl_runops_debug (dump.c​:2237) ==44050== by 0x53B4C8​: perl_run (perl.c​:2427) ==44050== by 0x42B167​: main (perlmain.c​:116) ==44050== Address 0x5edfaa8 is 168 bytes inside a block of size 4\,080 alloc'd ==44050== at 0x4C28BED​: malloc (vg_replace_malloc.c​:263) ==44050== by 0x7DB80C​: Perl_safesysmalloc (util.c​:149) ==44050== by 0x976C39​: Perl_newSV (sv.c​:309) ==44050== by 0x8A7BB6​: Perl_av_fetch (av.c​:275) ==44050== by 0x67629C​: Perl_pad_alloc (pad.c​:751) ==44050== by 0x4718D4​: Perl_newUNOP (op.c​:4189) ==44050== by 0x65D526​: Perl_yyparse (perly.y​:1000) ==44050== by 0x531D30​: S_parse_body (perl.c​:2277) ==44050== by 0x539532​: perl_parse (perl.c​:1611) ==44050== by 0x42AED7​: main (perlmain.c​:114) ==44050== ==44050== ==44050== Process terminating with default action of signal 11 (SIGSEGV) ==44050== General Protection Fault ==44050== at 0x5EDFAAA​: ??? ==44050== by 0x5C5AD91​: _nl_archive_subfreeres (in /lib/x86_64-linux-gnu/ libc-2.13.so) ==44050== by 0x5C5AA38​: free_mem (in /lib/x86_64-linux-gnu/libc-2.13.so) ==44050== by 0x5C5B1C1​: __libc_freeres (in /lib/x86_64-linux-gnu/ libc-2.13.so) ==44050== by 0x4A226EC​: _vgnU_freeres (vg_preloaded.c​:61) ==44050== by 0x4​: ??? ==44050== by 0x7EF767​: Perl_repeatcpy (string3.h​:52) ==44050== by 0xA540FC​: Perl_pp_repeat (pp.c​:1741) ==44050== by 0x7CB49E​: Perl_runops_debug (dump.c​:2237) ==44050== by 0x53B4C8​: perl_run (perl.c​:2427) ==44050== by 0x42B167​: main (perlmain.c​:116) Segmentation fault

With the minimized test case(test06-min\, attached)\, the output is a bit different​:

gdb-peda$ file ~/perl/perl gdb-peda$ set args test06-min gdb-peda$ r [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1". Number found where operator expected at test06-min line 1\, near "$~0" (Missing operator before 0?) Smartmatch is experimental at test06-min line 1.

Program received signal SIGSEGV\, Segmentation fault. [----------------------------------registers-----------------------------------] RAX​: 0x11f0b70 --> 0x0 RBX​: 0x8 RCX​: 0x20000 RDX​: 0x1ff00 RSI​: 0x11f0b70 --> 0x0 RDI​: 0x1210b70 RBP​: 0x1 RSP​: 0x7fffffffe058 --> 0x7ef768 (\<Perl_repeatcpy+5016>​: mov rcx\,rax) RIP​: 0x7ffff6e88498 (\<__memcpy_ssse3_back+7016>​: movdqa XMMWORD PTR [rdi-0x10]\,xmm1) R8 : 0x1210b60 R9 : 0x0 R10​: 0x4 R11​: 0x7ffff6eb3290 --> 0xfffd6150fffd5ace R12​: 0x3fffffffffffffff R13​: 0x20000 R14​: 0x8000 R15​: 0x11d0b70 --> 0x11e3030 (0x00000000011e3030) EFLAGS​: 0x10206 (carry PARITY adjust zero sign trap INTERRUPT direction overflow) [-------------------------------------code-------------------------------------]   0x7ffff6e88488 \<__memcpy_ssse3_back+7000>​: movdqu xmm6\,XMMWORD PTR [rsi-0x60]   0x7ffff6e8848d \<__memcpy_ssse3_back+7005>​: movdqu xmm7\,XMMWORD PTR [rsi-0x70]   0x7ffff6e88492 \<__memcpy_ssse3_back+7010>​: movdqu xmm8\,XMMWORD PTR [rsi-0x80] => 0x7ffff6e88498 \<__memcpy_ssse3_back+7016>​: movdqa XMMWORD PTR [rdi-0x10]\,xmm1   0x7ffff6e8849d \<__memcpy_ssse3_back+7021>​: movdqa XMMWORD PTR [rdi-0x20]\,xmm2   0x7ffff6e884a2 \<__memcpy_ssse3_back+7026>​: movdqa XMMWORD PTR [rdi-0x30]\,xmm3   0x7ffff6e884a7 \<__memcpy_ssse3_back+7031>​: movdqa XMMWORD PTR [rdi-0x40]\,xmm4   0x7ffff6e884ac \<__memcpy_ssse3_back+7036>​: movdqa XMMWORD PTR [rdi-0x50]\,xmm5 [------------------------------------stack-------------------------------------] 0000| 0x7fffffffe058 --> 0x7ef768 (\<Perl_repeatcpy+5016>​: mov rcx\,rax) 0008| 0x7fffffffe060 --> 0x4 0016| 0x7fffffffe068 --> 0x11d0b78 --> 0x11e3030 (0x00000000011e3030) 0024| 0x7fffffffe070 --> 0x7ffffffffffffffe 0032| 0x7fffffffe078 --> 0x1000000000000000 0040| 0x7fffffffe080 --> 0xffffffffffffffff 0048| 0x7fffffffe088 --> 0x7 0056| 0x7fffffffe090 --> 0x0 [------------------------------------------------------------------------------] Legend​: code\, data\, rodata\, value Stopped reason​: SIGSEGV __memcpy_ssse3_back () at ../sysdeps/x86_64/multiarch/memcpy-ssse3-back.S​:1664 1664 ../sysdeps/x86_64/multiarch/memcpy-ssse3-back.S​: No such file or directory. gdb-peda$ exploit Description​: Access violation on destination operand Short description​: DestAv (8/22) Hash​: 925c76a199d52d3ee63d7a002c765bfa.925c76a199d52d3ee63d7a002c765bfa Exploitability Classification​: EXPLOITABLE Explanation​: The target crashed on an access violation at an address matching the destination operand of the instruction. This likely indicates a write access violation\, which means the attacker may control the write address and/or value. Other tags​: AccessViolation (21/22)

Number found where operator expected at test06-min line 1\, near "$~0" (Missing operator before 0?) Smartmatch is experimental at test06-min line 1. ==37339== Invalid write of size 8 ==37339== at 0x4C2A7ED​: memcpy (mc_replace_strmem.c​:838) ==37339== by 0x7EF767​: Perl_repeatcpy (string3.h​:52) ==37339== by 0xA540FC​: Perl_pp_repeat (pp.c​:1741) ==37339== by 0x7CB49E​: Perl_runops_debug (dump.c​:2237) ==37339== by 0x53B4C8​: perl_run (perl.c​:2427) ==37339== by 0x42B167​: main (perlmain.c​:116) ==37339== Address 0x5eb4128 is not stack'd\, malloc'd or (recently) free'd ==37339== ==37339== Invalid read of size 8 ==37339== at 0x4C2A7E8​: memcpy (mc_replace_strmem.c​:838) ==37339== by 0x7EF767​: Perl_repeatcpy (string3.h​:52) ==37339== by 0xA540FC​: Perl_pp_repeat (pp.c​:1741) ==37339== by 0x7CB49E​: Perl_runops_debug (dump.c​:2237) ==37339== by 0x53B4C8​: perl_run (perl.c​:2427) ==37339== by 0x42B167​: main (perlmain.c​:116) ==37339== Address 0x5eb4128 is not stack'd\, malloc'd or (recently) free'd ==37339== ==37339== Invalid read of size 8 ==37339== at 0x4C2A7FA​: memcpy (mc_replace_strmem.c​:838) ==37339== by 0x7EF767​: Perl_repeatcpy (string3.h​:52) ==37339== by 0xA540FC​: Perl_pp_repeat (pp.c​:1741) ==37339== by 0x7CB49E​: Perl_runops_debug (dump.c​:2237) ==37339== by 0x53B4C8​: perl_run (perl.c​:2427) ==37339== by 0x42B167​: main (perlmain.c​:116) ==37339== Address 0x5eb4118 is 8 bytes after a block of size 1\,024 alloc'd ==37339== at 0x4C28BED​: malloc (vg_replace_malloc.c​:263) ==37339== by 0x7DB80C​: Perl_safesysmalloc (util.c​:149) ==37339== by 0x8A602C​: Perl_av_extend_guts (av.c​:182) ==37339== by 0xAE6BEF​: Perl_new_stackinfo (scope.c​:56) ==37339== by 0x51897A​: Perl_init_stacks (perl.c​:4017) ==37339== by 0x518EF9​: perl_construct (perl.c​:249) ==37339== by 0x42AE73​: main (perlmain.c​:110) ==37339== ==37339== ==37339== Process terminating with default action of signal 11 (SIGSEGV) ==37339== Access not within mapped region at address 0x62B3D28 ==37339== at 0x4C2A7ED​: memcpy (mc_replace_strmem.c​:838) ==37339== by 0x7EF767​: Perl_repeatcpy (string3.h​:52) ==37339== by 0xA540FC​: Perl_pp_repeat (pp.c​:1741) ==37339== by 0x7CB49E​: Perl_runops_debug (dump.c​:2237) ==37339== by 0x53B4C8​: perl_run (perl.c​:2427) ==37339== by 0x42B167​: main (perlmain.c​:116) ==37339== If you believe this happened as a result of a stack ==37339== overflow in your program's main thread (unlikely but ==37339== possible)\, you can try to increase the size of the ==37339== main thread stack using the --main-stacksize= flag. ==37339== The main thread stack size used in this run was 8388608. ==37339== Invalid free() / delete / delete[] / realloc() ==37339== at 0x4C27D4E​: free (vg_replace_malloc.c​:427) ==37339== by 0x5C5AD6B​: _nl_archive_subfreeres (in /lib/x86_64-linux-gnu/ libc-2.13.so) ==37339== by 0x5C5AA38​: free_mem (in /lib/x86_64-linux-gnu/libc-2.13.so) ==37339== by 0x5C5B1C1​: __libc_freeres (in /lib/x86_64-linux-gnu/ libc-2.13.so) ==37339== by 0x4A226EC​: _vgnU_freeres (vg_preloaded.c​:61) ==37339== by 0x4​: ??? ==37339== by 0x7EF767​: Perl_repeatcpy (string3.h​:52) ==37339== by 0xA540FC​: Perl_pp_repeat (pp.c​:1741) ==37339== by 0x7CB49E​: Perl_runops_debug (dump.c​:2237) ==37339== by 0x53B4C8​: perl_run (perl.c​:2427) ==37339== by 0x42B167​: main (perlmain.c​:116) ==37339== Address 0x5edf368 is 24 bytes inside a block of size 4\,080 alloc'd ==37339== at 0x4C28BED​: malloc (vg_replace_malloc.c​:263) ==37339== by 0x7DB80C​: Perl_safesysmalloc (util.c​:149) ==37339== by 0x995F91​: Perl_newSVuv (sv.c​:309) ==37339== by 0x5D1C1C​: Perl_scan_num (toke.c​:10291) ==37339== by 0x5E7A42​: Perl_yylex (toke.c​:6249) ==37339== by 0x65AC04​: Perl_yyparse (perly.c​:322) ==37339== by 0x531D30​: S_parse_body (perl.c​:2277) ==37339== by 0x539532​: perl_parse (perl.c​:1611) ==37339== by 0x42AED7​: main (perlmain.c​:114) ==37339== ==37339== ==37339== Process terminating with default action of signal 11 (SIGSEGV) ==37339== General Protection Fault ==37339== at 0x5EDF372​: ??? ==37339== by 0x5EDF2​: ??? ==37339== by 0x5C5AD91​: _nl_archive_subfreeres (in /lib/x86_64-linux-gnu/ libc-2.13.so) ==37339== by 0x5C5AA38​: free_mem (in /lib/x86_64-linux-gnu/libc-2.13.so) ==37339== by 0x5C5B1C1​: __libc_freeres (in /lib/x86_64-linux-gnu/ libc-2.13.so) ==37339== by 0x4A226EC​: _vgnU_freeres (vg_preloaded.c​:61) ==37339== by 0x4​: ??? ==37339== by 0x7EF767​: Perl_repeatcpy (string3.h​:52) ==37339== by 0xA540FC​: Perl_pp_repeat (pp.c​:1741) ==37339== by 0x7CB49E​: Perl_runops_debug (dump.c​:2237) ==37339== by 0x53B4C8​: perl_run (perl.c​:2427) ==37339== by 0x42B167​: main (perlmain.c​:116) Segmentation fault

test06 hexdump (53 bytes)​: 0000000 2470 797e 7e7e 7e7e 7e7e 7e7e 6e69 2474 0000010 781b 7836 2d28 5f24 352f 2c29 2428 255f 0000020 3f35 2224 5f3a 7829 7e7e 7e7e 357e 222c 0000030 227c 3726 000a 0000035

test06-min hexdump (25 bytes)​: 0000000 2470 307e 7e7e 6e69 2474 7830 2928 282c 0000010 3f30 3a30 2930 7e78 0030 0000019

System information​: Debian 7\, Kernel 3.2.65-1+deb7u1 x86_64\, GCC 4.9.2\, libc 2.13-38+deb7u7

p5pRT commented 9 years ago

From @geeknik

test06

p5pRT commented 9 years ago

From @geeknik

test06-min

p5pRT commented 9 years ago

From @geeknik

Here is another test case that produces similar GDB output\, but over 1000 errors in Valgrind (and even crashed Valgrind).

Hexdump of the -byte test case​: 0000000 2470 2830 2c29 3028 303f 303a 7829 3232 0000010 3030 3030 3030 3030 0000018

p5pRT commented 9 years ago

From @geeknik

test20-min

p5pRT commented 9 years ago

From @hvds

This is effectively​:

% ./miniperl -e '@​x = (1) x ~0' Segmentation fault (core dumped) %

The scalar case seems to be caught correctly​:

% ./miniperl -e '$x = 0 x ~0' panic​: realloc\, size=9223372036854775808 at -e line 1. %

I'm not sure why the MEM_WRAP_CHECK_1(max\, SV*\, oom_list_extend) in this pp_repeat case isn't catching it for us; but in any case\, it seems odd that items\, count and max are all signed - they should either be unsigned\, or we should be checking for and croaking on negative values.

We actually explicitly avoid checking that in the secondary test​:

  if (items > 0 && max > 0 && (max \< items || max \< count))   Perl_croak(aTHX_ "%s"\, oom_list_extend);

.. but I suspect that was assuming non-negative counts\, and intended just to avoid zeros.

Hugo

p5pRT commented 9 years ago

The RT System itself - Status changed from 'new' to 'open'

p5pRT commented 9 years ago

From @hvds

On Sat Mar 07 03​:32​:29 2015\, hv wrote​:

This is effectively​:

% ./miniperl -e '@​x = (1) x ~0' Segmentation fault (core dumped) % [...] I'm not sure why the MEM_WRAP_CHECK_1(max\, SV*\, oom_list_extend) in this pp_repeat case isn't catching it for us; but in any case\, it seems odd that items\, count and max are all signed - they should either be unsigned\, or we should be checking for and croaking on negative values.

I should clarify that in this case we have (IV) count capped at IV_MAX and (I32) items = 1\, giving (I32) max = -1 when sizeof(IV)==8.

if (items > 0 && max > 0 && (max \< items || max \< count)) Perl_croak(aTHX_ "%s"\, oom_list_extend);

Oh\, this also isn't how to check for overflow after multiplication - it's an idiom appropriate for checking overflow after addition.

Hugo

p5pRT commented 9 years ago

From @hvds

if (items > 0 && max > 0 && (max \< items || max \< count)) Perl_croak(aTHX_ "%s"\, oom_list_extend);

Oh\, this also isn't how to check for overflow after multiplication - it's an idiom appropriate for checking overflow after addition.

I think the simplest example of failure for that check is​:   ./miniperl -e '@​x = (1\,2\,3\,4\,5) x (2**30 + 1)' .. in which 5 * (2^30+1) will wrap to 2^30+5.

The suggested change below catches all the failure cases I can think of (but doesn't attempt to distinguish them with more specific error messages). I'm not sure if some compilers will complain about the triviality of 'count != (I32)count' when sizeof(IV)==sizeof(I32)\, we might need an additional guard there.

I did look at a second approach that further capped count to I32_MAX\, but that left '(0) x ~0' bravely trying to allocate all my memory\, which seems less useful - I'd be tempted even to remove the initial cap to IV_MAX\, making it another overflow error instead.

I've taken a quick look at the scalar repeat case\, I think it looks safe from all these problems\, but it'd be worth more pairs of eyes.

Hugo

--- a/pp.c +++ b/pp.c @​@​ -1724\,7 +1724\,8 @​@​ PP(pp_repeat)

  MEM_WRAP_CHECK_1(max\, SV*\, oom_list_extend);   /* Did the max computation overflow? */ - if (items > 0 && max > 0 && (max \< items || max \< count)) + if (items \< 0 || count != (I32)count + || (items != 0 && max / items != (I32)count))   Perl_croak(aTHX_ "%s"\, oom_list_extend);   MEXTEND(MARK\, max);   if (count > 1) {

p5pRT commented 9 years ago

From @jhi

On Saturday-201503-07 6​:32\, Hugo van der Sanden via RT wrote​:

but in any case\, it seems odd that items\, count and max are all signed - they should either be unsigned\, or we should be checking for and croaking on negative values.

As a general pondering\, I am starting to dislike unsigned for most uses\, especially where they indicate count or size or whatnot. There just seems to be too many chances of flipping over to the large end.

Unsigned types only seem to make sense for applications like bit vectors\, modular arithmetics in places like crypto\, and code units (like Unicode). (In other words\, I think size_t was not such a good idea\, ssize_t was better.)

Then again\, signed types are hateful because their behaviour on overflow is explicitly undefined\, and this makes them fair game for compiler writers to fly flocks of nasal daemons when optimizing.

p5pRT commented 9 years ago

From @jhi

On Saturday-201503-07 7​:30\, Hugo van der Sanden via RT wrote​:

left '(0) x ~0' bravely trying to allocate all my memory\, which seems less useful - I'd be tempted even to remove the initial cap to IV_MAX\, making it another overflow error instead.

Would make sense.

The IV_MAX as a limit in the array case makes not that much sense anyway\, it's not like we can/will allocate single bytes. Maybe more like IV_MAX / sizeof(SV*)\, asymptotically...

p5pRT commented 9 years ago

From @hvds

Under what conditions would this be exploitable? As I understand it we'll be writing repeats of a sequence of SV*s; to me\, the only obvious way to gain control of the values would be to allocate a bunch of SVs and then inspect their addresses picking the ones you want\, but I can't see that option being available to anyone that hasn't already got full control over the code.

I'll try to finalize a fix this weekend if noone else gets there first.

Hugo

p5pRT commented 9 years ago

From @iabyn

On Wed\, Mar 11\, 2015 at 01​:50​:05AM -0700\, Hugo van der Sanden via RT wrote​:

Under what conditions would this be exploitable? As I understand it we'll be writing repeats of a sequence of SV*s; to me\, the only obvious way to gain control of the values would be to allocate a bunch of SVs and then inspect their addresses picking the ones you want\, but I can't see that option being available to anyone that hasn't already got full control over the code.

I think it falls on the 'no CVE and no emergency maint releases' side of the fence. The previous issue we have with repeat ($char x $n) caused a wild overflown count arg to be passed to memset()\, which combined with a glibc memset() bug that didn't handle -ve lengths correctly\, meant that glibc could start using random areas of memory as a jump table. That was enough for me to decide that maybe it could be exploitable. But in this case we pass sensible values to mem*().

So we are in a situation where the use code is already broken - it allows the attacker to specify the RHS arg of 'x'\, and so can already DOS\, exhaust memory\, and probably crash the process; this is just giving them another route to crashing.

I'll try to finalize a fix this weekend if noone else gets there first.

I've had a look and worked up this more extensive fix (my recent work on MEM_WRAP_CHECK is still fresh in my head)​:

commit 95c7ae9aa2b88c1f37eafb1af837ea97123d558c Author​: David Mitchell \davem@&#8203;iabyn\.com AuthorDate​: Wed Mar 11 12​:25​:58 2015 +0000 Commit​: David Mitchell \davem@&#8203;iabyn\.com CommitDate​: Wed Mar 11 12​:47​:14 2015 +0000

  repeat op​: avoid integer overflows  
  For the list variant of the x operator\, the overflow detection code   doesn't always work\, resulting in the stack not being extended enough.  
  There are two places where an overflow could occur​: calculating how   many stack items to extend by (items * count)\, and passing to repeatcpy()   how many bytes to copy 'items' SV pointers (items * sizeof(const SV *)).  
  The overflow detection was generally a mess; checking for overflow   using a value (max) that may have already overflown; checking whether   'max' had overflown incorrectly\, and not checking (items * sizeof(const SV   *) at all (perhaps hoping that the other checks would be a superset of   this).  
  Also\, some of the vars were still I32s; promote to 64-bit capable types.  
  Finally\, the signature of Perl_repeatcpy() still has an I32 len;   that really should be changed to IV at some point; but for now I've just   ensured that the callers (list 'x' and scalar 'x') don't wrap.   I haven't put in a check for the only other core caller of repeatcpy()\,   S_study_chunk()\, since that would only become an issue compiling a pattern   with a fixed or floating substr within it of length > 2**31.

Affected files ...  
  M pp.c

Differences ...

Inline Patch ```diff diff --git a/pp.c b/pp.c index f795725..c357a65 100644 --- a/pp.c +++ b/pp.c @@ -1717,17 +1717,19 @@ PP(pp_repeat) if (GIMME_V == G_ARRAY && PL_op->op_private & OPpREPEAT_DOLIST) { dMARK; - static const char* const oom_list_extend = "Out of memory during list extend"; - const I32 items = SP - MARK; - const I32 max = items * count; + const Size_t items = SP - MARK; const U8 mod = PL_op->op_flags & OPf_MOD; - MEM_WRAP_CHECK_1(max, SV*, oom_list_extend); - /* Did the max computation overflow? */ - if (items > 0 && max > 0 && (max < items || max < count)) - Perl_croak(aTHX_ "%s", oom_list_extend); - MEXTEND(MARK, max); if (count > 1) { + Size_t max; + + if ( items > MEM_SIZE_MAX / count /* max would overflow */ + || items > I32_MAX / sizeof(SV *) /* repeatcpy would overflow */ + ) + Perl_croak(aTHX_ "%s","Out of memory during list extend"); + max = items * count; + MEXTEND(MARK, max); + while (SP > MARK) { if (*SP) { if (mod && SvPADTMP(*SP)) { @@ -1749,8 +1751,6 @@ PP(pp_repeat) SV * const tmpstr = POPs; STRLEN len; bool isutf; - static const char* const oom_string_extend = - "Out of memory during string extend"; if (TARG != tmpstr) sv_setsv_nomg(TARG, tmpstr); @@ -1760,11 +1760,16 @@ PP(pp_repeat) if (count < 1) SvCUR_set(TARG, 0); else { - const STRLEN max = (UV)count * len; - if (len > MEM_SIZE_MAX / count) - Perl_croak(aTHX_ "%s", oom_string_extend); - MEM_WRAP_CHECK_1(max, char, oom_string_extend); - SvGROW(TARG, max + 1); + STRLEN max; + + if ( len > (MEM_SIZE_MAX-1) / count /* max would overflow */ + || len > I32_MAX /* repeatcpy would overflow */ + ) + Perl_croak(aTHX_ "%s", + "Out of memory during string extend"); + max = (UV)count * len + 1; + SvGROW(TARG, max); + repeatcpy(SvPVX(TARG) + len, SvPVX(TARG), len, count - 1); SvCUR_set(TARG, SvCUR(TARG) * count); } -- ```

A power surge on the Bridge is rapidly and correctly diagnosed as a faulty capacitor by the highly-trained and competent engineering staff.   -- Things That Never Happen in "Star Trek" #9

p5pRT commented 9 years ago

From @hvds

On Wed Mar 11 06​:03​:46 2015\, davem wrote​:

I've had a look and worked up this more extensive fix (my recent work on MEM_WRAP_CHECK is still fresh in my head)

Thanks\, looks both cleaner and more comprehensive.

+ if ( items > MEM_SIZE_MAX / count /* max would overflow */ + || items > I32_MAX / sizeof(SV *) /* repeatcpy would overflow */

For the RHS of those comparisons\, I'm not sure as what type the division will be evaluated; it probably doesn't matter for the result\, but an additional cast on the dividend to clarify the intent (particularly on the second one) might make the next reader's life a little easier.

Hugo

p5pRT commented 9 years ago

From @iabyn

On Thu\, Mar 12\, 2015 at 01​:31​:57AM -0700\, Hugo van der Sanden via RT wrote​:

On Wed Mar 11 06​:03​:46 2015\, davem wrote​:

I've had a look and worked up this more extensive fix (my recent work on MEM_WRAP_CHECK is still fresh in my head)

Thanks\, looks both cleaner and more comprehensive.

+ if ( items > MEM_SIZE_MAX / count /* max would overflow */ + || items > I32_MAX / sizeof(SV *) /* repeatcpy would overflow */

For the RHS of those comparisons\, I'm not sure as what type the division will be evaluated; it probably doesn't matter for the result\, but an additional cast on the dividend to clarify the intent (particularly on the second one) might make the next reader's life a little easier.

I don't know. C's integer promotion rules are a bit of a haze to me\, but I assume that in the absence of casts\, C will promote each each integer type in the expression to the largest necessary\, and will vaguely Do the Right Thing as regards signedness. Whereas putting in explicit casts could inadvertently truncate or wrap values?

These two expressions

  items > MEM_SIZE_MAX / count   items > I32_MAX / sizeof(SV *)

in terms of types are​:

  Size_t > MEM_SIZE / IV   Size_t > int(*) / size_t (*) or long\, or whatever is 32-bit

which typically map to

  size_t > size_t / IV   size_t > int / size_t

which in terms of signedness\, are

  U > U / S   U > S / U

and in terms of 32/64-bitness on a 64-bit platform are likely to be​:

  64 > 64 / 64   64 > 32 / 64

Anyone have any advice on suitable casts?

-- The optimist believes that he lives in the best of all possible worlds. As does the pessimist.

p5pRT commented 9 years ago

From @iabyn

On Thu\, Mar 12\, 2015 at 10​:18​:19AM +0000\, Dave Mitchell wrote​:

On Thu\, Mar 12\, 2015 at 01​:31​:57AM -0700\, Hugo van der Sanden via RT wrote​:

On Wed Mar 11 06​:03​:46 2015\, davem wrote​:

I've had a look and worked up this more extensive fix (my recent work on MEM_WRAP_CHECK is still fresh in my head)

Thanks\, looks both cleaner and more comprehensive.

+ if ( items > MEM_SIZE_MAX / count /* max would overflow */ + || items > I32_MAX / sizeof(SV *) /* repeatcpy would overflow */

For the RHS of those comparisons\, I'm not sure as what type the division will be evaluated; it probably doesn't matter for the result\, but an additional cast on the dividend to clarify the intent (particularly on the second one) might make the next reader's life a little easier.

I don't know. C's integer promotion rules are a bit of a haze to me\, but I assume that in the absence of casts\, C will promote each each integer type in the expression to the largest necessary\, and will vaguely Do the Right Thing as regards signedness. Whereas putting in explicit casts could inadvertently truncate or wrap values?

These two expressions

items > MEM\_SIZE\_MAX / count
items > I32\_MAX / sizeof\(SV \*\)

in terms of types are​:

Size\_t > MEM\_SIZE / IV
Size\_t > int\(\*\) / size\_t       \(\*\) or long\, or whatever is 32\-bit

which typically map to

size\_t > size\_t / IV
size\_t > int / size\_t

which in terms of signedness\, are

U > U / S
U > S / U

and in terms of 32/64-bitness on a 64-bit platform are likely to be​:

64 > 64 / 64
64 > 32 / 64

Anyone have any advice on suitable casts?

Ok\, I've read around (again) on integral conversion\, and I *think* the rules for binary operations on integral values are​:

  lower-case represents a 'small' int type\, upper-case a larger int type;   s and u are signed and unsigned.

  same sign easy​:

  s S => S S   u U => U U

  mixed sign​: can be a problem​:

  S u => S S   s U \   S U / => U U undefined behaviour if s \< 0 ?

Does the above look right?

so in the examples above\, the U/S and S/U are potentially problematical if the S value is \< 0.

So the correct casts would be​:

  items > MEM_SIZE_MAX / (UV)count   items > (U32)I32_MAX / sizeof(SV *)

i.e. all arithmetic is done using unsigned args\, and the IV->UV conversion isn't undefined because we already know that count > 0.

Does that sound right?

-- "Emacs isn't a bad OS once you get used to it. It just lacks a decent editor."

p5pRT commented 9 years ago

From @jhi

    S u  => S S
    s U \\
    S U / => U U undefined behaviour if s \< 0 ?

I don't think it's undefined behaviour (as in Nasal Daemons sense)... the signed get promoted to unsigned.

Or at least that's how I think after reading​:

https://www.securecoding.cert.org/confluence/display/c/INT02-C.+Understand+integer+conversion+rules

(they mention a Flash vuln because of calloc\, interestingly...)

Does the above look right?

so in the examples above\, the U/S and S/U are potentially problematical if the S value is \< 0.

So the correct casts would be​:

 items > MEM\_SIZE\_MAX / \(UV\)count
 items > \(U32\)I32\_MAX / sizeof\(SV \*\)

That ... does look right. Though my head always spins quite badly\, again\, after reading about the rules.

Here's another article http​://embeddedgurus.com/stack-overflow/2009/08/a-tutorial-on-signed-and-unsigned-integers/ which suggests using intermediate variables makes mixing signed and unsigned easier to reason about\, and safer.

-- There is this special biologist word we use for 'stable'. It is 'dead'. -- Jack Cohen

p5pRT commented 9 years ago

From @iabyn

On Thu\, Mar 12\, 2015 at 09​:57​:36AM -0400\, Jarkko Hietaniemi wrote​:

    S u  => S S
    s U \\
    S U / => U U undefined behaviour if s \< 0 ?

I don't think it's undefined behaviour (as in Nasal Daemons sense)... the signed get promoted to unsigned.

Or at least that's how I think after reading​:

https://www.securecoding.cert.org/confluence/display/c/INT02-C.+Understand+integer+conversion+rules

Ok reading around a bit further\, I see that signed -> unsigned conversion has a specified behaviour\, where x' is the smallest positive value that satisfies x' ~ x % 2**N

So in​:

  I32 i32 = -8;   UV uv = 2**64-2   UV result = i32 + uv;

i32 gets converted to 2**64 - 8\, and the result of (2**64-2) + (2**64 - 8) is 2**64 + (2**64 - 10)\, which being unsigned has a defined overflow behaviour of wrapping\, and so becomes 2**64 - 10\, the correct result. And this will happen on all compliant platforms\, even non-2's-complement ones?

http​://embeddedgurus.com/stack-overflow/2009/08/a-tutorial-on-signed-and-unsigned-integers/

I like this bit​:

  This is the real crux of the problem with having signed and unsigned   data types. The C standard has an entire section on this topic that   only a compiler writer could love – and that the rest of us read and   wince at

-- A power surge on the Bridge is rapidly and correctly diagnosed as a faulty capacitor by the highly-trained and competent engineering staff.   -- Things That Never Happen in "Star Trek" #9

p5pRT commented 9 years ago

From @iabyn

On Thu\, Mar 12\, 2015 at 09​:57​:36AM -0400\, Jarkko Hietaniemi wrote​:

So the correct casts would be​:

 items > MEM\_SIZE\_MAX / \(UV\)count
 items > \(U32\)I32\_MAX / sizeof\(SV \*\)

That ... does look right. Though my head always spins quite badly\, again\, after reading about the rules.

I'll push the following to blead in a few days unless anyone objects​:

Inline Patch ```diff diff --git a/pp.c b/pp.c index f795725..ec0f9d3 100644 --- a/pp.c +++ b/pp.c @@ -1717,17 +1717,19 @@ PP(pp_repeat) if (GIMME_V == G_ARRAY && PL_op->op_private & OPpREPEAT_DOLIST) { dMARK; - static const char* const oom_list_extend = "Out of memory during list extend"; - const I32 items = SP - MARK; - const I32 max = items * count; + const Size_t items = SP - MARK; const U8 mod = PL_op->op_flags & OPf_MOD; - MEM_WRAP_CHECK_1(max, SV*, oom_list_extend); - /* Did the max computation overflow? */ - if (items > 0 && max > 0 && (max < items || max < count)) - Perl_croak(aTHX_ "%s", oom_list_extend); - MEXTEND(MARK, max); if (count > 1) { + Size_t max; + + if ( items > MEM_SIZE_MAX / (UV)count /* max would overflow */ + || items > (U32)I32_MAX / sizeof(SV *) /* repeatcpy would overflow */ + ) + Perl_croak(aTHX_ "%s","Out of memory during list extend"); + max = items * count; + MEXTEND(MARK, max); + while (SP > MARK) { if (*SP) { if (mod && SvPADTMP(*SP)) { @@ -1749,8 +1751,6 @@ PP(pp_repeat) SV * const tmpstr = POPs; STRLEN len; bool isutf; - static const char* const oom_string_extend = - "Out of memory during string extend"; if (TARG != tmpstr) sv_setsv_nomg(TARG, tmpstr); @@ -1760,11 +1760,16 @@ PP(pp_repeat) if (count < 1) SvCUR_set(TARG, 0); else { - const STRLEN max = (UV)count * len; - if (len > MEM_SIZE_MAX / count) - Perl_croak(aTHX_ "%s", oom_string_extend); - MEM_WRAP_CHECK_1(max, char, oom_string_extend); - SvGROW(TARG, max + 1); + STRLEN max; + + if ( len > (MEM_SIZE_MAX-1) / (UV)count /* max would overflow */ + || len > (U32)I32_MAX /* repeatcpy would overflow */ + ) + Perl_croak(aTHX_ "%s", + "Out of memory during string extend"); + max = (UV)count * len + 1; + SvGROW(TARG, max); + repeatcpy(SvPVX(TARG) + len, SvPVX(TARG), len, count - 1); SvCUR_set(TARG, SvCUR(TARG) * count); } -- ```

If life gives you lemons\, you'll probably develop a citric acid allergy.

p5pRT commented 9 years ago

From @hvds

Dave Mitchell \davem@&#8203;iabyn\.com wrote​: :On Thu\, Mar 12\, 2015 at 09​:57​:36AM -0400\, Jarkko Hietaniemi wrote​: :> > So the correct casts would be​: :> > :> > items > MEM_SIZE_MAX / (UV)count :> > items > (U32)I32_MAX / sizeof(SV *) :> :> That ... does look right. Though my head always spins quite badly\, :> again\, after reading about the rules.

Those look sane and credible to me.

:I'll push the following to blead in a few days unless anyone objects​:

Thanks.

Hugo

p5pRT commented 9 years ago

From @iabyn

On Thu\, Mar 12\, 2015 at 07​:41​:46PM +0000\, hv@​crypt.org wrote​:

Dave Mitchell \davem@&#8203;iabyn\.com wrote​: :On Thu\, Mar 12\, 2015 at 09​:57​:36AM -0400\, Jarkko Hietaniemi wrote​: :> > So the correct casts would be​: :> > :> > items > MEM_SIZE_MAX / (UV)count :> > items > (U32)I32_MAX / sizeof(SV *) :> :> That ... does look right. Though my head always spins quite badly\, :> again\, after reading about the rules.

Those look sane and credible to me.

:I'll push the following to blead in a few days unless anyone objects​:

Thanks.

Now pushed as b3b27d017bd3d61f6cbc3eaf8465b8d98d86a024.

-- Technology is dominated by two types of people​: those who understand what they do not manage\, and those who manage what they do not understand.

p5pRT commented 9 years ago

@iabyn - Status changed from 'open' to 'pending release'

p5pRT commented 9 years ago

From @hvds

On Thu Mar 05 18​:44​:35 2015\, brian.carpenter@​gmail.com wrote​:

Here is another test case that produces similar GDB output\, but over 1000 errors in Valgrind (and even crashed Valgrind).

Bother\, I missed this second test case\, which still fails​:

% ./miniperl -e 'p$0()\,(0?0​:0)x2200000000' Segmentation fault (core dumped) %

I think this is because of the (int) cast in MEXTEND​:   if (UNLIKELY(PL_stack_max - p \< (int)(n))) { ... } .. which wraps to negative in this case. Changing it to (SSize_t) appears to make it do the right thing (eventually); I'm not sure if markoff also needs to change\, but I'd guess so.

The patch below passes tests here\, and I'm reluctant to add a test that tries to allocate >2GB.

Thoughts?

Hugo

--- a/pp.h +++ b/pp.h @​@​ -278\,21 +278\,21 @​@​ Does not use C\. See also C\\, C\ and C   } STMT_END /* Same thing\, but update mark register too. */ # define MEXTEND(p\,n) STMT_START { \ - const int markoff = mark - PL_stack_base; \ + const SSize_t markoff = mark - PL_stack_base; \   sp = stack_grow(sp\,p\,(SSize_t) (n)); \   mark = PL_stack_base + markoff; \   PERL_UNUSED_VAR(sp); \   } STMT_END #else # define EXTEND(p\,n) STMT_START { \ - if (UNLIKELY(PL_stack_max - p \< (int)(n))) { \ + if (UNLIKELY(PL_stack_max - p \< (SSize_t)(n))) { \   sp = stack_grow(sp\,p\,(SSize_t) (n)); \   PERL_UNUSED_VAR(sp); \   } } STMT_END /* Same thing\, but update mark register too. */ # define MEXTEND(p\,n) STMT_START { \ - if (UNLIKELY(PL_stack_max - p \< (int)(n))) { \ - const int markoff = mark - PL_stack_base; \ + if (UNLIKELY(PL_stack_max - p \< (SSize_t)(n))) { \ + const SSize_t markoff = mark - PL_stack_base; \   sp = stack_grow(sp\,p\,(SSize_t) (n)); \   mark = PL_stack_base + markoff; \   PERL_UNUSED_VAR(sp); \

p5pRT commented 9 years ago

From @iabyn

On Sat\, Mar 21\, 2015 at 05​:33​:27AM -0700\, Hugo van der Sanden via RT wrote​:

On Thu Mar 05 18​:44​:35 2015\, brian.carpenter@​gmail.com wrote​:

Here is another test case that produces similar GDB output\, but over 1000 errors in Valgrind (and even crashed Valgrind).

Bother\, I missed this second test case\, which still fails​:

% ./miniperl -e 'p$0()\,(0?0​:0)x2200000000' Segmentation fault (core dumped) %

I think this is because of the (int) cast in MEXTEND​: if (UNLIKELY(PL_stack_max - p \< (int)(n))) { ... } .. which wraps to negative in this case. Changing it to (SSize_t) appears to make it do the right thing (eventually); I'm not sure if markoff also needs to change\, but I'd guess so.

The patch below passes tests here\, and I'm reluctant to add a test that tries to allocate >2GB.

Thoughts?

Looks good to me.

Note also that the whole markstack implementation needs an upgrade at some point; at the moment the markstack is an array of I32s\, where each entry is an index into the perl stack. If you have a machine big enough to push 2Gb SVs onto the stack\, bad things might happen. (This is is of course a separate issue from trying and failing to do this on a smaller machine\, and getting a SEGV or wild behaviour rather than an error message. Which is what the current ticket is really addressing.)

Nethertheless\, I think that post 5.22\, we should consider changing the types of PL_markstack\, PL_markstack_ptr\, PL_markstack_max from I32* to SSize_t*\, along with similar changes in PUSHMARK() and dORIGMARK.

-- The Enterprise successfully ferries an alien VIP from one place to another without serious incident.   -- Things That Never Happen in "Star Trek" #7

p5pRT commented 9 years ago

From @hvds

Ricardo\, I'd like to do a couple more test runs\, then merge this tonight with the expectation that it'll be fine\, but if it causes any black smoke we simply revert it. Are you happy with that?

Hugo

p5pRT commented 9 years ago

From @rjbs

* Hugo van der Sanden via RT \perl5\-security\-report@&#8203;perl\.org [2015-03-24T04​:06​:18]

Ricardo\, I'd like to do a couple more test runs\, then merge this tonight with the expectation that it'll be fine\, but if it causes any black smoke we simply revert it. Are you happy with that?

I replied to Hugo privately\, but should've replied here​: okay!

-- rjbs

p5pRT commented 9 years ago

From @hvds

Now pushed as commit ff36bb50a70​:

  fix signed/unsigned mismatch in (M)EXTEND  
  A large enough allocation request could wrap\, causing MEXTEND to decide   the stack was already big enough.

Hugo

p5pRT commented 9 years ago

From @geeknik

Can I get this bug marked public since the fix has already been pushed out? I'm not sure I agree with the no CVE assignment here and I'd like to have some of my infosec friends take a look.

On Tue\, Mar 24\, 2015 at 8​:04 PM\, Hugo van der Sanden via RT \< perl5-security-report@​perl.org> wrote​:

Now pushed as commit ff36bb50a70​:

fix signed/unsigned mismatch in \(M\)EXTEND

A large enough allocation request could wrap\, causing MEXTEND to decide
the stack was already big enough\.

Hugo

p5pRT commented 9 years ago

From @rjbs

* Brian Carpenter \brian\.carpenter@&#8203;gmail\.com [2015-04-08T00​:55​:18]

Can I get this bug marked public since the fix has already been pushed out? I'm not sure I agree with the no CVE assignment here and I'd like to have some of my infosec friends take a look.

In the event that we do not get a CVE or put specific attack details in a ticket\, my feeling is that we should be willing to make it public.

Security list members​: Please respond ASAP if you disagree\, even if only to say\, "I disagree\, more details later."

-- rjbs

p5pRT commented 9 years ago

From @iabyn

On Thu\, Apr 09\, 2015 at 02​:28​:03PM -0400\, Ricardo Signes wrote​:

* Brian Carpenter \brian\.carpenter@&#8203;gmail\.com [2015-04-08T00​:55​:18]

Can I get this bug marked public since the fix has already been pushed out? I'm not sure I agree with the no CVE assignment here and I'd like to have some of my infosec friends take a look.

In the event that we do not get a CVE or put specific attack details in a ticket\, my feeling is that we should be willing to make it public.

Security list members​: Please respond ASAP if you disagree\, even if only to say\, "I disagree\, more details later."

I have no objections to the ticket being moved to the public queue.

-- The Enterprise is captured by a vastly superior alien intelligence which does not put them on trial.   -- Things That Never Happen in "Star Trek" #10

p5pRT commented 9 years ago

From @khwilliamson

Thank you for submitting this ticket.

The issue should now be resolved with the release today of Perl v5.22\, which is available at http​://www.perl.org/get.html -- Karl Williamson for the Perl 5 team

p5pRT commented 9 years ago

@khwilliamson - Status changed from 'pending release' to 'resolved'