out-of-bounds read when blurring

RESOLVED FIXED in Firefox 17

Status

()

Core
Graphics
--
critical
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: miaubiz, Assigned: Mats Palmgren (vacation - back in August))

Tracking

(5 keywords)

unspecified
mozilla17
crash, csectype-dos, reproducible, sec-other, testcase
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty -
in-testsuite +

Firefox Tracking Flags

(firefox14 affected, firefox15 affected, firefox16 affected, firefox17 fixed, firefox-esr10 wontfix)

Details

(Whiteboard: [asan][adv-track-main17+])

Attachments

(9 attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
Created attachment 652160 [details]
asan log linux
(Reporter)

Comment 2

5 years ago
Created attachment 652161 [details]
asan log osx
(Reporter)

Comment 3

5 years ago
Created attachment 652162 [details]
alternative repro
(Reporter)

Comment 4

5 years ago
Created attachment 652164 [details]
alternative repro
(Reporter)

Comment 5

5 years ago
Created attachment 652165 [details]
last one
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
Assignee: nobody → matspal
Severity: normal → critical
status-firefox-esr10: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → affected
status-firefox16: --- → affected
status-firefox17: --- → affected
Component: Layout → Graphics
Keywords: crash, reproducible, sec-other, testcase
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [asan]
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
Attachment #652243 - Flags: review?(roc)
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.
Attachment #652246 - Flags: review?(roc)
Blocks: 771842
Attachment #652246 - Flags: review?(roc) → review+
Attachment #652243 - Flags: review?(roc) → review+
(Reporter)

Comment 10

5 years ago
if the adjacent memory is mapped readable, can the result of the bluring be used to leak contents of memory?
(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
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e3450bbd3e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/6383531428d1
Flags: in-testsuite?
Target Milestone: --- → mozilla17
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?
(Reporter)

Comment 14

5 years ago
@mats: ok with me
(In reply to Mats Palmgren [:mats] from comment #12)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/9e3450bbd3e3
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6383531428d1

https://hg.mozilla.org/mozilla-central/rev/9e3450bbd3e3
https://hg.mozilla.org/mozilla-central/rev/6383531428d1
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox17: affected → fixed
Resolution: --- → FIXED
Keywords: csec-dos
Blocks: 774207
Sec-other implies that this bug is not exploitable. Is that the case? https://wiki.mozilla.org/Security_Severity_Ratings
Yes, it's not exploitable AFAICT, see comment 7 and comment 11.
sec-other, non-exploitable, falls outside of esr criteria - wontfixing.
status-firefox-esr10: affected → wontfix
Keywords: verifyme
Since this is not exploitable there shouldn't be a problem landing the tests and unhiding the bug.
Should we flag this status-firefox14:wontfix and status-firefox15:wontfix at this point?
Whiteboard: [asan] → [asan][adv-track-main17+]
Alias: CVE-2012-5832
Alias: CVE-2012-5832
Group: core-security
Flags: sec-bounty-
Crash tests:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c955c4e5eaff
Flags: in-testsuite? → in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/c955c4e5eaff
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.