Skip to content

Commit

Permalink
xcf-load: avoid use-after-free for layers with multiple PROP_ACTIVE_L…
Browse files Browse the repository at this point in the history 
…AYER

If a given layer's properties contain multiple PROP_ACTIVE_LAYER entries, we
could add multiple entries for the same layer in info->selected_layers.

However, xcf_load_layer_props and xcf_load_layer both contain logic that
overwrites the layer pointer - they do take care of updating
info->selected_layers and and info->floating_sel (both of which could contain
or point to the current layer). Crucially, they both assume that
info->selected_layers can only contain this layer once - hence they only
remove it from the list once. This logic also frees the old layer because it
thinks that it's no longer in use.

But if we've added the original layer to selected_layers multiple times,
and then use the logic that overwrites layer, selected_layers will still
contain an invalid pointer to the now-freed original layer. Any later
code that tries to read info->selected_layers will hit a user-after-free.

Therefore we need to check if the current layer is already in the
selected_layers list, and avoid adding it again if it's already there.
(It also seems illogical to say that a layer is selected twice, but I'm
 focusing on the code-correctness aspect in my analysis.)

Adjustment by Jacob Boerema: the correct way to test that a value does
not exist in a list is to check for the value being negative. Also add
a message to warn users.

ASAN output:

==21241==ERROR: AddressSanitizer: heap-use-after-free on address 0x615000012150 at pc 0x000000d54dbf bp 0x7ffcf813bb60 sp 0x7ffcf813bb58
READ of size 8 at 0x615000012150 thread T0
    #0 0xd54dbe in gimp_image_set_selected_layers /home/ahunt/git/gimp/app/core/gimpimage.c:4743:7
    #1 0xb146c6 in xcf_load_image /home/ahunt/git/gimp/app/xcf/xcf-load.c:690:7
    #2 0xb1199b in xcf_load_stream /home/ahunt/git/gimp/app/xcf/xcf.c:305:19
    #3 0x619cad in LLVMFuzzerTestOneInput /home/ahunt/git/gimp/app/fuzzers/xcf_fuzzer.c:35:17
    #4 0x51d2e4 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:599:15
    #5 0x506f62 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:323:6
    #6 0x50d2d0 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:856:9
    #7 0x537322 in main /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    #8 0x7f064a594349 in __libc_start_main (/lib64/libc.so.6+0x24349)
    #9 0x4e06f9 in _start /home/abuild/rpmbuild/BUILD/glibc-2.26/csu/../sysdeps/x86_64/start.S:120

0x615000012150 is located 336 bytes inside of 504-byte region [0x615000012000,0x6150000121f8)
freed by thread T0 here:
    #0 0x5e84e2 in free /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:127:3
    #1 0x7f064b3f81c8 in g_free (/usr/lib64/libglib-2.0.so.0+0x581c8)
    #2 0x7f064b41133f in g_slice_free1 (/usr/lib64/libglib-2.0.so.0+0x7133f)
    #3 0x7f064b7015c0 in g_type_free_instance /usr/src/debug/glib2-2.62.6-lp152.2.3.1.x86_64/build/../gobject/gtype.c:1946
    #4 0xb187d8 in xcf_load_layer /home/ahunt/git/gimp/app/xcf/xcf-load.c:2146:3
    #5 0xb13997 in xcf_load_image /home/ahunt/git/gimp/app/xcf/xcf-load.c:504:15
    #6 0xb1199b in xcf_load_stream /home/ahunt/git/gimp/app/xcf/xcf.c:305:19
    #7 0x619cad in LLVMFuzzerTestOneInput /home/ahunt/git/gimp/app/fuzzers/xcf_fuzzer.c:35:17
    #8 0x51d2e4 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:599:15
    #9 0x506f62 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:323:6
    #10 0x50d2d0 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:856:9
    #11 0x537322 in main /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    #12 0x7f064a594349 in __libc_start_main (/lib64/libc.so.6+0x24349)
    #13 0x4e06f9 in _start /home/abuild/rpmbuild/BUILD/glibc-2.26/csu/../sysdeps/x86_64/start.S:120

previously allocated by thread T0 here:
    #0 0x5e874d in malloc /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3
    #1 0x7f064b3f80c8 in g_malloc (/usr/lib64/libglib-2.0.so.0+0x580c8)
    #2 0x7f064b410da5 in g_slice_alloc (/usr/lib64/libglib-2.0.so.0+0x70da5)
    #3 0x7f064b411258 in g_slice_alloc0 (/usr/lib64/libglib-2.0.so.0+0x71258)
    #4 0x7f064b7011d5 in g_type_create_instance /usr/src/debug/glib2-2.62.6-lp152.2.3.1.x86_64/build/../gobject/gtype.c:1849
    #5 0x7f064b6e164f in g_object_new_internal /usr/src/debug/glib2-2.62.6-lp152.2.3.1.x86_64/build/../gobject/gobject.c:1827
    #6 0x7f064b6e354d in g_object_new_valist /usr/src/debug/glib2-2.62.6-lp152.2.3.1.x86_64/build/../gobject/gobject.c:2152
    #7 0x7f064b6e38c8 in g_object_new /usr/src/debug/glib2-2.62.6-lp152.2.3.1.x86_64/build/../gobject/gobject.c:1670
    #8 0xdb5b90 in gimp_item_new /home/ahunt/git/gimp/app/core/gimpitem.c:723:10
    #9 0xce0358 in gimp_drawable_new /home/ahunt/git/gimp/app/core/gimpdrawable.c:1067:14
    #10 0xddd8db in gimp_layer_new /home/ahunt/git/gimp/app/core/gimplayer-new.c:65:11
    #11 0xb182a6 in xcf_load_layer /home/ahunt/git/gimp/app/xcf/xcf-load.c:2027:11
    #12 0xb13997 in xcf_load_image /home/ahunt/git/gimp/app/xcf/xcf-load.c:504:15
    #13 0xb1199b in xcf_load_stream /home/ahunt/git/gimp/app/xcf/xcf.c:305:19
    #14 0x619cad in LLVMFuzzerTestOneInput /home/ahunt/git/gimp/app/fuzzers/xcf_fuzzer.c:35:17
    #15 0x51d2e4 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:599:15
    #16 0x506f62 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:323:6
    #17 0x50d2d0 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:856:9
    #18 0x537322 in main /home/abuild/rpmbuild/BUILD/llvm-12.0.0.src/build/../projects/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    #19 0x7f064a594349 in __libc_start_main (/lib64/libc.so.6+0x24349)
    #20 0x4e06f9 in _start /home/abuild/rpmbuild/BUILD/glibc-2.26/csu/../sysdeps/x86_64/start.S:120

SUMMARY: AddressSanitizer: heap-use-after-free /home/ahunt/git/gimp/app/core/gimpimage.c:4743:7 in gimp_image_set_selected_layers
Shadow bytes around the buggy address:
  0x0c2a7fffa3d0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2a7fffa3e0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2a7fffa3f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2a7fffa400: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2a7fffa410: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x0c2a7fffa420: fd fd fd fd fd fd fd fd fd fd[fd]fd fd fd fd fd
  0x0c2a7fffa430: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa
  0x0c2a7fffa440: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2a7fffa450: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2a7fffa460: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2a7fffa470: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==21241==ABORTING

( crash-f37f84bd7072e65348ca0e2c920d1e08165356fd )
  • Loading branch information
@ahunt @Wormnest
ahunt authored and Wormnest committed Jul 20, 2024
1 parent ff9f71e commit 7d94942
Showing 1 changed file with 8 additions and 1 deletion.
9 changes: 8 additions & 1 deletion app/xcf/xcf-load.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1639,7 +1639,14 @@ xcf_load_layer_props (XcfInfo *info,
return TRUE;

case PROP_ACTIVE_LAYER:
info->selected_layers = g_list_prepend (info->selected_layers, *layer);
{
if (g_list_index (info->selected_layers, *layer) < 0)
info->selected_layers = g_list_prepend (info->selected_layers, *layer);
else
gimp_message_literal (info->gimp, G_OBJECT (info->progress),
GIMP_MESSAGE_WARNING,
"Invalid duplicate selected layer detected");
}
break;

case PROP_FLOATING_SELECTION:
Expand Down

0 comments on commit 7d94942

Please sign in to comment.