Last Comment Bug 783041 - out-of-bounds read when blurring
: out-of-bounds read when blurring
Status: RESOLVED FIXED
[asan][adv-track-main17+]
: crash, csectype-dos, reproducible, sec-other, testcase
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: All All
: -- critical (vote)
: mozilla17
Assigned To: Mats Palmgren (:mats)
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks: 771842 774207
  Show dependency treegraph
 
Reported: 2012-08-15 11:44 PDT by miaubiz
Modified: 2014-07-14 13:04 PDT (History)
9 users (show)
dveditz: sec‑bounty-
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
affected
affected
fixed
wontfix


Attachments
repro case (1.29 KB, text/plain)
2012-08-15 11:44 PDT, miaubiz
no flags Details
asan log linux (1.83 KB, text/plain)
2012-08-15 11:44 PDT, miaubiz
no flags Details
asan log osx (1.95 KB, text/plain)
2012-08-15 11:45 PDT, miaubiz
no flags Details
alternative repro (1.55 KB, text/plain)
2012-08-15 11:45 PDT, miaubiz
no flags Details
alternative repro (2.36 KB, text/plain)
2012-08-15 11:45 PDT, miaubiz
no flags Details
last one (1.58 KB, text/plain)
2012-08-15 11:46 PDT, miaubiz
no flags Details
part 1, fix (1.58 KB, patch)
2012-08-15 14:48 PDT, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review
part 2, minor cleanup / perf improvement (1.40 KB, patch)
2012-08-15 14:52 PDT, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review
Crash tests (do not land before this bug report is public) (9.16 KB, patch)
2012-08-16 07:36 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review

Description miaubiz 2012-08-15 11:44:30 PDT
Created attachment 652159 [details]
repro case

opening this html crashes asan:

<html>
  <head>
    <style>
      #el7 {
        font-size:.92em 
      } 
      #el0 { 
        height: 200px ! important; 
        margin: 0px; 
        display: table;
        font-size:.92em 
      }

      #el3 { 
        line-height: 0.5px; 
        text-shadow: 0px 5px 5px, 0px -20px 10px;
        display: table-row-group; 
        transform: translate3d(-3px, -300px, 0px); 
      }

      #el5 { 
        height:1em;
        display:block; 
      } 

      .c4 { 
        margin: 1em;
        padding: 0.5em;
      } 
    </style>
    <script>
      onload = function() {
        el7=document.createElement('iframe')
        el7.setAttribute('id', 'el7')
        document.body.appendChild(el7)

        el0=document.createElement('span')
        el0.setAttribute('id','el0')
        document.body.appendChild(el0)
        el0.appendChild(document.createTextNode('A'))

        el3=document.createElement('q')
        el3.setAttribute('id','el3')
        el0.appendChild(el3)

        el5=document.createElement('q')
        el5.setAttribute('id','el5')
        el3.appendChild(el5)

        el0.appendChild(document.createTextNode('A'))

        document.body.offsetTop
        el0.setAttribute('class', 'c4'); 
        el7.setAttribute('class', 'c4'); 
      }
    </script>
  </head>
  <body>
  </body>
</html>


like so:

==7766== ERROR: AddressSanitizer heap-buffer-overflow on address 0x7fffc8308570 at pc 0x7ffff18196d7 bp 0x7fffffff22c0 sp 0x7fffffff22b8
READ of size 1 at 0x7fffc8308570 thread T0
    #0 0x7ffff18196d7 in mozilla::gfx::BoxBlurVertical(unsigned char*, unsigned char*, int, int, int, int, mozilla::gfx::IntRect const&) /builds/slave/try-lnx64/build/gfx/2d/Blur.cpp:148
0x7fffc8308570 is located 16 bytes to the left of 1-byte region [0x7fffc8308580,0x7fffc8308581)
allocated by thread T0 here:
    #0 0x424331 in __interceptor_malloc ??:0
    #1 0x7ffff1817c86 in mozilla::gfx::AlphaBoxBlur::Blur() /builds/slave/try-lnx64/build/gfx/2d/Blur.cpp:443

==80621== ERROR: AddressSanitizer heap-buffer-overflow on address 0x0001297b6e78 at pc 0x1080f96f7 bp 0x7fff5fbeaa60 sp 0x7fff5fbeaa58
READ of size 1 at 0x0001297b6e78 thread T0
    #0 0x1080f96f7 in _ZN7mozilla3gfxL15BoxBlurVerticalEPhS1_iiiiRKNS0_7IntRectE (in XUL) (Blur.cpp:148)
0x0001297b6e78 is located 8 bytes to the left of 1-byte region [0x0001297b6e80,0x0001297b6e81)
allocated by thread T0 here:
    #0 0x10000bd1d in (anonymous namespace)::mz_malloc(_malloc_zone_t*, unsigned long) (in firefox) + 45
    #1 0x7fff92d583c8 in __strtodg$UNIX2003 (in libsystem_c.dylib) + 945
    #2 0x7fff92d591a4 in __strtodg$UNIX2003 (in libsystem_c.dylib) + 4493
    #3 0x1080f7cb6 in mozilla::gfx::AlphaBoxBlur::Blur() (in XUL) (Blur.cpp:443)


I've seen anything from 4 to 84 bytes to the left depending on the exact values.
Comment 1 miaubiz 2012-08-15 11:44:58 PDT
Created attachment 652160 [details]
asan log linux
Comment 2 miaubiz 2012-08-15 11:45:14 PDT
Created attachment 652161 [details]
asan log osx
Comment 3 miaubiz 2012-08-15 11:45:34 PDT
Created attachment 652162 [details]
alternative repro
Comment 4 miaubiz 2012-08-15 11:45:50 PDT
Created attachment 652164 [details]
alternative repro
Comment 5 miaubiz 2012-08-15 11:46:06 PDT
Created attachment 652165 [details]
last one
Comment 6 Mats Palmgren (:mats) 2012-08-15 14:16:28 PDT
I get a fatal assertion on the first and fourth testcase using
a Linux64 asan debug build:
Assertion failure: aRows > 0, at gfx/2d/Blur.cpp:119
Comment 7 Mats Palmgren (:mats) 2012-08-15 14:48:53 PDT
Created attachment 652243 [details] [diff] [review]
part 1, fix

The problem is that we test IsEmpty() on 'rect' which is a gfx Rect
which use floating point members, and then later copying those
to 'mRect' which is IntRect, so a small rect.height makes IsEmpty()
false but mRect.height zero.  Later we depend on 'mData' being
NULL for an empty 'mRect' - the "aRows" in the assertion comes from
'mRect.height'.

Testing mRect.IsEmpty() instead should fix it.

I don't see anything that would do an illegal write operation
in this file (for either zero width/height) so the bug appears
non-exploitable, just an illegal read operation, which should
crash safely.

This patch applies also to Aurora and Beta.
The code is slightly different on esr10, but it looks like it's
also affected (gfxAlphaBoxBlur::Init in file gfx/thebes/gfxBlur.cpp).
It should be easy to make a similar fix for esr10.

https://tbpl.mozilla.org/?tree=Try&rev=aa94c6c8986e
Comment 8 Mats Palmgren (:mats) 2012-08-15 14:52:36 PDT
Created attachment 652246 [details] [diff] [review]
part 2, minor cleanup / perf improvement

'shadowIntRect' is redundant now - we can use mRect instead
which already is an IntRect.  s/IntersectRect/Intersect
since we don't use the return value of the former.
Comment 10 miaubiz 2012-08-16 03:11:29 PDT
if the adjacent memory is mapped readable, can the result of the bluring be used to leak contents of memory?
Comment 11 Mats Palmgren (:mats) 2012-08-16 07:14:33 PDT
(In reply to miaubiz from comment #10)
> can the result of the bluring be
> used to leak contents of memory?

No, the read values are only stored in a local variable and
does not propagate any further when the bug occurs.

The code is written in such a way that the write operations
does not occur when aWidth or aRows is zero:
http://hg.mozilla.org/mozilla-central/file/50e4ff05741e/gfx/2d/Blur.cpp
Comment 13 Mats Palmgren (:mats) 2012-08-16 07:36:01 PDT
Created attachment 652448 [details] [diff] [review]
Crash tests (do not land before this bug report is public)

miaubiz@gmail.com, I slapped the standard MPL2 onto your files:

<!-- This Source Code Form is subject to the terms of the Mozilla Public
   - License, v. 2.0. If a copy of the MPL was not distributed with this
   - file, You can obtain one at http://mozilla.org/MPL/2.0/.  -->

OK?
Comment 14 miaubiz 2012-08-16 08:41:28 PDT
@mats: ok with me
Comment 16 Alex Keybl [:akeybl] 2012-08-23 16:35:05 PDT
Sec-other implies that this bug is not exploitable. Is that the case? https://wiki.mozilla.org/Security_Severity_Ratings
Comment 17 Mats Palmgren (:mats) 2012-08-23 17:04:41 PDT
Yes, it's not exploitable AFAICT, see comment 7 and comment 11.
Comment 18 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-30 16:34:37 PDT
sec-other, non-exploitable, falls outside of esr criteria - wontfixing.
Comment 19 Daniel Veditz [:dveditz] 2012-09-23 08:59:22 PDT
Since this is not exploitable there shouldn't be a problem landing the tests and unhiding the bug.
Comment 20 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-10-16 15:26:26 PDT
Should we flag this status-firefox14:wontfix and status-firefox15:wontfix at this point?
Comment 21 Mats Palmgren (:mats) 2013-05-14 07:03:02 PDT
Crash tests:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c955c4e5eaff
Comment 22 Ryan VanderMeulen [:RyanVM] 2013-05-14 13:30:24 PDT
https://hg.mozilla.org/mozilla-central/rev/c955c4e5eaff
Comment 23 Tracy Walker [:tracy] 2014-01-10 10:42:22 PST
mass remove verifyme requests greater than 4 months old

Note You need to log in before you can comment on or make changes to this bug.