Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix out-of-bounds read in egs_view by making region arrays the same size #756

Merged
merged 1 commit into from
Jun 30, 2022
Merged

Fix out-of-bounds read in egs_view by making region arrays the same size #756

merged 1 commit into from
Jun 30, 2022

Conversation

mxxo
Copy link
Contributor

@mxxo mxxo commented Jul 27, 2021 •
edited
Loading

After compiling egs_view with asan support, there was a buffer overflow
error encountered opening tutor7pp/test1.egsinp.

It was traced back to the memcpy call in ImageWindow::paintEvent:

==26122==ERROR: AddressSanitizer: dynamic-stack-buffer-overflow on address 0x7ffe71889e40 at pc 0x7f543595ece0 bp 0x7ffe71889da0 sp 0x7ffe71889548
READ of size 400 at 0x7ffe71889e40 thread T0
    #0 0x7f543595ecdf  (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x99cdf)
    #1 0x55dc3acdbb15 in memcpy /usr/include/x86_64-linux-gnu/bits/string_fortified.h:34
    #2 0x55dc3acdbb15 in ImageWindow::paintEvent(QPaintEvent*) /home/max/projects/EGSnrc/HEN_HOUSE/egs++/view/image_window.cpp:348
    #3 0x7f5434f86047 in QWidget::event(QEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x193047)

The issue is that memcpy will always copy sizeof(lastRegions) bytes from regions,
into lastRegions, but before this change regions could be potentially
shorter than lastRegions, making part of the read out of bounds.

int maxreg=min(int((h-145)/15),N_REG_MAX); 
int regions[maxreg];

After this change, maxreg is always set to N_REG_MAX, the length of lastRegions.

EGSnrc/HEN_HOUSE/egs++/view/image_window.cpp

Lines 338 to 347 in a6fc389

int maxreg=min(int((h-145)/15),N_REG_MAX);
int regions[maxreg];
EGS_Vector colors[N_REG_MAX];
EGS_Vector hitCoord(0,0,0);
EGS_Float hitScore = 0;
vis->getRegions(xp, lastRequestGeo, regions, colors, maxreg, hitCoord, q.score, hitScore);
if (!wasRerenderRequested && memcmp(regions, lastRegions, sizeof(lastRegions)) == 0) {
return;
}
memcpy(lastRegions, regions, sizeof(lastRegions));

This effectively reverts part of 246ee26, where the size of regions was changed to scale the region list when the window was resized (perhaps there is a better resolution that keeps this feature, I thought I would try the simplest resolution for now).

Reproduction

Compile egs_view with asan support by uncommenting the following lines:

EGSnrc/HEN_HOUSE/egs++/view/view.pro

Lines 91 to 94 in a6fc389

#DEFINES += VIEW_DEBUG
#QMAKE_CXXFLAGS+="-fsanitize=address -fno-omit-frame-pointer"
#QMAKE_CXXFLAGS+="-ggdb3"
#QMAKE_LFLAGS+="-fsanitize=address" 

$ cd $EGS_HOME
$ egs_view tutor7pp/test1.egsinp
Full debug log and asan output 
$ egs_view tutor7pp/test1.egsinp 
In init()
QObject::connect: No such signal QLineEdit::lostFocus()
QObject::connect:  (sender name:   'fileName')
QObject::connect:  (receiver name: 'save image')
In loadInput(), reloading is 0
GeometryViewControl::loadInput: Clearing previous geometries...
GeometryViewControl::loadInput: Clearing previous ausgab objects...
GeometryViewControl::loadInput: Reading input file...
GeometryViewControl::loadInput: Creating the geometry...
Got 0 user defined colors
In setGeometry(), geometry name is slab
 center: (0,0,0.05)
 xmin: (-5,-5,0)
 xmax: (5,5,0.1)
 nx=512 ny=512 nnx=512 nny=512 nxr=1 nyr=1
GeometryViewControl::loadInput: Processing ausgab objects...
EGS_ObjectFactory::createObjects(): the input is not of type ausgab object definition and also does not have items of this type
 nx=512 ny=512 nnx=128 nny=128 nxr=4 nyr=4
In resizeEvent(): size is 512 512 old size is: -1 -1 shown: 0
 nx=512 ny=512 nnx=128 nny=256 nxr=4 nyr=2
 nx=512 ny=512 nnx=512 nny=512 nxr=1 nyr=1
=================================================================
==26122==ERROR: AddressSanitizer: dynamic-stack-buffer-overflow on address 0x7ffe71889e40 at pc 0x7f543595ece0 bp 0x7ffe71889da0 sp 0x7ffe71889548
READ of size 400 at 0x7ffe71889e40 thread T0
    #0 0x7f543595ecdf  (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x99cdf)
    #1 0x55dc3acdbb15 in memcpy /usr/include/x86_64-linux-gnu/bits/string_fortified.h:34
    #2 0x55dc3acdbb15 in ImageWindow::paintEvent(QPaintEvent*) /home/max/projects/EGSnrc/HEN_HOUSE/egs++/view/image_window.cpp:348
    #3 0x7f5434f86047 in QWidget::event(QEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x193047)
    #4 0x7f5434f4783b in QApplicationPrivate::notify_helper(QObject*, QEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x15483b)
    #5 0x7f5434f4f103 in QApplication::notify(QObject*, QEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x15c103)
    #6 0x7f54341c98d7 in QCoreApplication::notifyInternal2(QObject*, QEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x28a8d7)
    #7 0x7f5434f7f199 in QWidgetPrivate::sendPaintEvent(QRegion const&) (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x18c199)
    #8 0x7f5434f7f759 in QWidgetPrivate::drawWidget(QPaintDevice*, QRegion const&, QPoint const&, int, QPainter*, QWidgetBackingStore*) (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x18c759)
    #9 0x7f5434f56dfd  (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x163dfd)
    #10 0x7f5434f570a4  (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x1640a4)
    #11 0x7f5434f6e67e in QWidgetPrivate::syncBackingStore() (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x17b67e)
    #12 0x7f5434f861b7 in QWidget::event(QEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x1931b7)
    #13 0x7f5434f4783b in QApplicationPrivate::notify_helper(QObject*, QEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x15483b)
    #14 0x7f5434f4f103 in QApplication::notify(QObject*, QEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x15c103)
    #15 0x7f54341c98d7 in QCoreApplication::notifyInternal2(QObject*, QEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x28a8d7)
    #16 0x7f5434f57ec7  (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x164ec7)
    #17 0x7f5434f58b86  (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x165b86)
    #18 0x7f5434f703c6 in QWidget::repaint(QRect const&) (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x17d3c6)
    #19 0x7f5434f7042b in QWidget::repaint() (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x17d42b)
    #20 0x55dc3ace3d76 in ImageWindow::drawResults(RenderResults, RenderParameters) /home/max/projects/EGSnrc/HEN_HOUSE/egs++/view/image_window.cpp:648
    #21 0x55dc3acef5ef in ImageWindow::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) .moc/linux/moc_image_window.cpp:203
    #22 0x7f54341f90c1 in QObject::event(QEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x2ba0c1)
    #23 0x7f5434f8675a in QWidget::event(QEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x19375a)
    #24 0x7f5434f4783b in QApplicationPrivate::notify_helper(QObject*, QEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x15483b)
    #25 0x7f5434f4f103 in QApplication::notify(QObject*, QEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5+0x15c103)
    #26 0x7f54341c98d7 in QCoreApplication::notifyInternal2(QObject*, QEvent*) (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x28a8d7)
    #27 0x7f54341cc04c in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x28d04c)
    #28 0x7f5434223262  (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x2e4262)
    #29 0x7f54314eb536 in g_main_context_dispatch (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4c536)
    #30 0x7f54314eb76f  (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4c76f)
    #31 0x7f54314eb7fb in g_main_context_iteration (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4c7fb)
    #32 0x7f543422288e in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x2e388e)
    #33 0x7f54341c7909 in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x288909)
    #34 0x7f54341d09b3 in QCoreApplication::exec() (/usr/lib/x86_64-linux-gnu/libQt5Core.so.5+0x2919b3)
    #35 0x55dc3ac1ed33 in main /home/max/projects/EGSnrc/HEN_HOUSE/egs++/view/main.cpp:137
    #36 0x7f54331e5b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)
    #37 0x55dc3ac1f639 in _start (/home/max/projects/EGSnrc/HEN_HOUSE/bin/linux/egs_view+0x1f639)

Address 0x7ffe71889e40 is located in stack of thread T0
SUMMARY: AddressSanitizer: dynamic-stack-buffer-overflow (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x99cdf) 
Shadow bytes around the buggy address:
  0x10004e309370: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10004e309380: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10004e309390: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10004e3093a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10004e3093b0: 00 00 00 00 00 00 00 00 ca ca ca ca 00 00 00 00
=>0x10004e3093c0: 00 00 00 00 00 00 00 00[cb]cb cb cb 00 00 00 00
  0x10004e3093d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10004e3093e0: 00 00 00 00 00 00 f1 f1 f1 f1 f8 f2 f2 f2 f8 f2
  0x10004e3093f0: f2 f2 00 f2 f2 f2 00 f2 f2 f2 00 f2 f2 f2 00 f2
  0x10004e309400: f2 f2 00 f2 f2 f2 00 f2 f2 f2 00 f2 f2 f2 00 f2
  0x10004e309410: f2 f2 00 f2 f2 f2 00 f2 f2 f2 00 f2 f2 f2 00 f2
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
==26122==ABORTING

@mxxo mxxo requested a review from a team as a code owner July 27, 2021 23:49
@mxxo mxxo requested review from ftessier, mainegra and blakewalters and removed request for a team July 27, 2021 23:49
@mxxo mxxo changed the base branch from master to develop July 27, 2021 23:50
@rtownson rtownson self-assigned this May 11, 2022
@rtownson rtownson added the bug label May 11, 2022
@ftessier ftessier force-pushed the view-oob-read branch 2 times, most recently from b861676 to b35e844 Compare June 27, 2022 20:06
@mxxo @ftessier
Fix regions array buffer overflow in egs++ viewer
c71de66 
After compiling egs_view with asan support, there was a buffer overflow
error traced back to the memcpy call in ImageWindow::paintEvent.

ERROR: AddressSanitizer: dynamic-stack-buffer-overflow ...
READ of size 400 at 0x7ffe71889e40 thread T0
    #0 0x7f543595ecdf  (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x99cdf)
    nrc-cnrc#1 0x55dc3acdbb15 in memcpy
    nrc-cnrc#2 0x55dc3acdbb15 in ImageWindow::paintEvent(QPaintEvent*)
    nrc-cnrc#3 0x7f5434f86047 in QWidget::event(QEvent*)

The issue is that memcpy will always copy sizeof(lastRegions) bytes into
the array regions. But before this change, regions could be shorter than
lastRegions, leading to a buffer overflow. After this change, maxreg is
always set to N_REG_MAX, the length of lastRegions.
@ftessier ftessier added the small label Jun 27, 2022
@ftessier ftessier merged commit 825038e into nrc-cnrc:develop Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blakewalters blakewalters blakewalters approved these changes

@mainegra mainegra mainegra approved these changes

@ftessier ftessier ftessier approved these changes

Assignees

@rtownson rtownson

Labels
bug small
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mxxo @ftessier @mainegra @blakewalters @rtownson