Skip to content

Commit

Permalink
dt_iop_load_modules_so(): Prevent stack-buffer-overflow if IOP name i…
Browse files Browse the repository at this point in the history
…s longer than 19 symbols

We use  char op[20];  to store iop names, and if somehow we end up with iop
with long-enough name, we end up overflowing stack:

==23602==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fff00816974 at pc 0x7fdcbfbce3f2 bp 0x7fff00816810 sp 0x7fff00816808
WRITE of size 1 at 0x7fff00816974 thread T0
    #0 0x7fdcbfbce3f1 in dt_iop_load_modules_so /home/lebedevri/darktable/src/develop/imageop.c:1214
    #1 0x7fdcbfb00229 in dt_init /home/lebedevri/darktable/src/common/darktable.c:873
    #2 0x400b9f in main /home/lebedevri/darktable/src/main.c:24
    #3 0x7fdcb80b1b44 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b44)
    #4 0x400c22 (/usr/local/bin/darktable+0x400c22)

Address 0x7fff00816974 is located in stack of thread T0 at offset 180 in frame
    #0 0x7fdcbfbcc92f in dt_iop_load_modules_so /home/lebedevri/darktable/src/develop/imageop.c:1196

  This frame has 5 object(s):
    [32, 40) 'stmt'
    [96, 104) 'stmt'
    [160, 180) 'op' <== Memory access at offset 180 overflows this variable
    [224, 1248) 'path'
    [1280, 5376) 'plugindir'

SUMMARY: AddressSanitizer: stack-buffer-overflow /home/lebedevri/darktable/src/develop/imageop.c:1214 dt_iop_load_modules_so
...
==23602==ABORTING

Now, if we encounter such an iop (e.g. libexposure123456789012.so), we will simply fail to load it:
[iop_load_module] failed to open operation `exposure12345678901': /usr/local/lib/darktable/plugins/libexposure12345678901.so: cannot open shared object file: No such file or directory

While there, also add check to the CMake add_iop() macro to prevent adding such iops.
  • Loading branch information
LebedevRI committed Feb 26, 2015
1 parent 9aeee06 commit 8bfcc8c
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 2 deletions.
3 changes: 1 addition & 2 deletions src/develop/imageop.c
Original file line number Diff line number Diff line change
Expand Up @@ -1210,8 +1210,7 @@ void dt_iop_load_modules_so()
// get lib*.so
if(!g_str_has_prefix(d_name, SHARED_MODULE_PREFIX)) continue;
if(!g_str_has_suffix(d_name, SHARED_MODULE_SUFFIX)) continue;
strncpy(op, d_name + name_offset, strlen(d_name) - name_end);
op[strlen(d_name) - name_end] = '\0';
g_strlcpy(op, d_name + name_offset, MIN(sizeof(op), strlen(d_name) - name_end + 1));
module = (dt_iop_module_so_t *)calloc(1, sizeof(dt_iop_module_so_t));
gchar *libname = g_module_build_path(plugindir, (const gchar *)op);
if(dt_iop_load_module_so(module, libname, op))
Expand Down
5 changes: 5 additions & 0 deletions src/iop/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@ cmake_minimum_required(VERSION 2.6)
include_directories("${CMAKE_CURRENT_BINARY_DIR}/../" "${CMAKE_CURRENT_SOURCE_DIR}")

macro (add_iop _lib _src)
string(LENGTH ${_lib} _lib_namelength)
if(${_lib_namelength} GREATER 19)
message(FATAL_ERROR "IOP name \"${_lib}\" is too long (${_lib_namelength} symbols), it should not be greater than 19 symbols.")
endif(${_lib_namelength} GREATER 19)

set(_input ${CMAKE_CURRENT_SOURCE_DIR}/${_src})
set(_output ${CMAKE_CURRENT_BINARY_DIR}/introspection_${_src}) # keep ${_src} in the end to keep the file extension (c vs. cc)
add_custom_command(
Expand Down

0 comments on commit 8bfcc8c

Please sign in to comment.