New issue
Advanced search Search tips

Issue 887196 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner: ----
Closed: Sep 22
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: ----
Type: Bug-Security



Sign in to add a comment

Security: Skia: Mishandling of the SkPipeDeserializer::do_playback return value.

Reported by quangn...@gmail.com, Sep 20

Issue description

VULNERABILITY DETAILS
I have found a vulnerability in SkPipeDeserializer::definePicture_handler. The attached poc can crash skia.

Check the following line: 
https://cs.chromium.org/chromium/src/third_party/skia/src/pipe/SkPipeReader.cpp?g=0&l=686


```
static void definePicture_handler(SkPipeReader& reader, uint32_t packedVerb, SkCanvas* canvas) {
    SkASSERT(SkPipeVerb::kDefinePicture == unpack_verb(packedVerb));
    int deleteIndex = unpack_verb_extra(packedVerb);

    SkPipeInflator* inflator = (SkPipeInflator*)reader.getInflator();

    if (deleteIndex) {
        inflator->setPicture(deleteIndex - 1, nullptr);
    } else {
        SkPictureRecorder recorder;
        int pictureIndex = -1;  // invalid
        const SkRect* cull = reader.skipT<SkRect>();
        if (!cull) {
            return;
        }
        do_playback(reader, recorder.beginRecording(*cull), &pictureIndex); // BUG HERE
        SkASSERT(pictureIndex > 0);
        sk_sp<SkPicture> picture = recorder.finishRecordingAsPicture();
        inflator->setPicture(pictureIndex, std::move(picture));
    }
}
```

In `definePicture_handler` function, developer doesnot check the `SkPipeDeserializer::do_playback` return value. if `do_playback` function returns `false`, `pictureIndex` is equal to -1, `pictureIndex` cannot use as index of an array.

VERSION
Operating System: Linux

REPRODUCTION CASE

Build Skia with asserts disabled
```
bin/gn gen out/Release --args='is_debug=false'
ninja -C out/Release
```

Run testcase:

```
./out/Release/fuzz -t pipe -b SkPipeDeserializer_do_playback_crash
```

Crate state:
```
gdb-peda$ r
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
[terminated] Didn't crash while decoding/drawing animated image!
[terminated] Couldn't deserialize Colorspace.
[terminated] filter_fuzz didn't crash!
[terminated] Didn't crash while decoding/drawing image!
Scaling factor: 1.000000
Mode: 0
Decoding
[terminated] Couldn't create codec.
Scaling factor: 1.000000
Mode: 0
Decoding
[terminated] Couldn't create codec.
[terminated] Done parsing!
[terminated] path_deserialize didn't crash!
[terminated] Couldn't initialize SkRegion.
[terminated] region_set_path didn't crash!
Decoding
------- bad verb 97

Program received signal SIGSEGV, Segmentation fault.
[----------------------------------registers-----------------------------------]
RAX: 0xfffffffffffffff0
RBX: 0xfffffffffffffffe
RCX: 0x0
RDX: 0x0
RSI: 0x7ffff6405b20 --> 0x100000000
RDI: 0x0
RBP: 0x7fffffffc960 --> 0xffffca00
RSP: 0x7fffffffc950 --> 0x0
RIP: 0x729f8b (<definePicture_handler(SkPipeReader&, uint32_t, SkCanvas*)+571>: mov    rdi,QWORD PTR [rax])
R8 : 0x7ffff7fc5740 (0x00007ffff7fc5740)
R9 : 0x14
R10: 0x0
R11: 0x246
R12: 0x1b15c38 --> 0x0
   0x729f87 <definePicture_handler(SkPipeReader&, uint32_t, SkCanvas*)+567>:    lea    rax,[rax+rbx*8]
=> 0x729f8b <definePicture_handler(SkPipeReader&, uint32_t, SkCanvas*)+571>:    mov    rdi,QWORD PTR [rax]
   0x729f8e <definePicture_handler(SkPipeReader&, uint32_t, SkCanvas*)+574>:    mov    QWORD PTR [rax],r13
   0x729f91 <definePicture_handler(SkPipeReader&, uint32_t, SkCanvas*)+577>:    test   rdi,rdi
   0x729f94 <definePicture_handler(SkPipeReader&, uint32_t, SkCanvas*)+580>:
    je     0x729fa1 <definePicture_handler(SkPipeReader&, uint32_t, SkCanvas*)+593>
   0x729f96 <definePicture_handler(SkPipeReader&, uint32_t, SkCanvas*)+582>:    lock sub DWORD PTR [rdi+0x8],0x1
[------------------------------------stack-------------------------------------]
0000| 0x7fffffffc950 --> 0x0
0008| 0x7fffffffc958 --> 0x7fffffffca00 --> 0x1b15c68 --> 0x0
0016| 0x7fffffffc960 --> 0xffffca00
0024| 0x7fffffffc968 --> 0x80
0032| 0x7fffffffc970 --> 0x6e6961506b530000 ('')
0040| 0x7fffffffc978 --> 0x0
0048| 0x7fffffffc980 --> 0x1b175c0 --> 0x1ad21b0 --> 0x6f27a0 (<SkRecorder::~SkRecorder()>:     push   rbp)
0056| 0x7fffffffc988 --> 0x1b15e50 --> 0x1ad2128 --> 0x6ec3e0 (<SkRecord::~SkRecord()>: push   r15)
[------------------------------------------------------------------------------]
Legend: code, data, rodata, value
Stopped reason: SIGSEGV
SkRefSet<SkPicture>::set (value=..., index=<optimized out>, this=<optimized out>) at ../../src/pipe/SkRefSet.h:23
23                  fArray[index] = std::move(value);
gdb-peda$ bt
#0  SkRefSet<SkPicture>::set (value=..., index=<optimized out>, this=<optimized out>) at ../../src/pipe/SkRefSet.h:23
#1  SkPipeInflator::setPicture (this=<optimized out>, pic=..., index=<optimized out>) at ../../src/pipe/SkPipeReader.cpp:58
#2  definePicture_handler (reader=..., packedVerb=packedVerb@entry=0x23000000, canvas=canvas@entry=0x0) at ../../src/pipe/SkPipeReader.cpp:689
#3  0x000000000072c770 in SkPipeDeserializer::readPicture (this=this@entry=0x7fffffffcb30, data=<optimized out>, size=<optimized out>)
    at ../../src/pipe/SkPipeReader.cpp:816
#4  0x00000000005d9c4c in SkPipeDeserializer::readPicture (data=<optimized out>, this=0x7fffffffcb30) at ../../src/core/SkPipe.h:63
#5  fuzz_skpipe (bytes=...) at ../../fuzz/FuzzMain.cpp:614
#6  0x00000000005da52d in fuzz_file (path=..., type=...) at ../../fuzz/FuzzMain.cpp:189
#7  0x00000000005c015e in main (argc=argc@entry=0x3, argv=argv@entry=0x7fffffffe1a8) at ../../fuzz/FuzzMain.cpp:109
#8  0x00007ffff6061830 in __libc_start_main (main=0x5bff70 <main(int, char**)>, argc=0x3, argv=0x7fffffffe1a8, init=<optimized out>,
    fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fffffffe198) at ../csu/libc-start.c:291
#9  0x00000000005c2ea9 in _start ()
```

Patch for this issue:
```
diff --git a/src/pipe/SkPipeReader.cpp b/src/pipe/SkPipeReader.cpp
index 78db581..ea641cb 100644
--- a/src/pipe/SkPipeReader.cpp
+++ b/src/pipe/SkPipeReader.cpp
@@ -683,7 +683,9 @@ static void definePicture_handler(SkPipeReader& reader, uint32_t packedVerb, SkC
         if (!cull) {
             return;
         }
-        do_playback(reader, recorder.beginRecording(*cull), &pictureIndex);
+        if (!do_playback(reader, recorder.beginRecording(*cull), &pictureIndex)) {
+            return;
+        }
         SkASSERT(pictureIndex > 0);
         sk_sp<SkPicture> picture = recorder.finishRecordingAsPicture();
         inflator->setPicture(pictureIndex, std::move(picture));
```

 
Cc: kjlubick@chromium.org hcm@google.com mtklein@chromium.org reed@google.com
Components: Internals>Skia
Labels: Security_Impact-None
Status: Available (was: Unconfirmed)
Bulk edit of recently filed skia pipe bugs.
Status: WontFix (was: Available)
See https://bugs.chromium.org/p/chromium/issues/detail?id=886713#c8
Project Member

Comment 3 by sheriffbot@chromium.org, Dec 29

Labels: -Restrict-View-SecurityTeam allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Sign in to add a comment