From 212e1ba0d4f65f1740bc8bc98551885b291b1be9 Mon Sep 17 00:00:00 2001
From: Andreas Kling <kling@serenityos.org>
Date: Fri, 14 May 2021 19:54:31 +0200
Subject: ImageViewer: Sandbox image decoding using the ImageDecoder service
 :^)

Instead of parsing untrusted and potentially malicious image files in
the ImageViewer GUI process, take advantage of the ImageDecoder service
that we already have on the system to sandbox the decode.

This prevents bugs in our image decoding libraries from being used as
an exploitation vector when viewing files in ImageViewer.
---
 Userland/Applications/ImageViewer/CMakeLists.txt |  2 +-
 Userland/Applications/ImageViewer/ViewWidget.cpp | 36 +++++++++++++++---------
 Userland/Applications/ImageViewer/ViewWidget.h   |  5 ++--
 3 files changed, 26 insertions(+), 17 deletions(-)

(limited to 'Userland/Applications/ImageViewer')

diff --git a/Userland/Applications/ImageViewer/CMakeLists.txt b/Userland/Applications/ImageViewer/CMakeLists.txt
index 3d6a7174b2..d1ee14ff7a 100644
--- a/Userland/Applications/ImageViewer/CMakeLists.txt
+++ b/Userland/Applications/ImageViewer/CMakeLists.txt
@@ -4,4 +4,4 @@ set(SOURCES
 )
 
 serenity_app(ImageViewer ICON filetype-image)
-target_link_libraries(ImageViewer LibDesktop LibGUI LibGfx)
+target_link_libraries(ImageViewer LibDesktop LibGUI LibGfx LibImageDecoderClient)
diff --git a/Userland/Applications/ImageViewer/ViewWidget.cpp b/Userland/Applications/ImageViewer/ViewWidget.cpp
index 08cd400463..5aff8e3b74 100644
--- a/Userland/Applications/ImageViewer/ViewWidget.cpp
+++ b/Userland/Applications/ImageViewer/ViewWidget.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018-2020, Andreas Kling <kling@serenityos.org>
+ * Copyright (c) 2018-2021, Andreas Kling <kling@serenityos.org>
  * Copyright (c) 2021, Linus Groh <linusg@serenityos.org>
  *
  * SPDX-License-Identifier: BSD-2-Clause
@@ -15,6 +15,7 @@
 #include <LibGfx/Bitmap.h>
 #include <LibGfx/Orientation.h>
 #include <LibGfx/Palette.h>
+#include <LibImageDecoderClient/Client.h>
 
 namespace ImageViewer {
 
@@ -241,22 +242,28 @@ void ViewWidget::load_from_file(const String& path)
     }
 
     auto& mapped_file = *file_or_error.value();
-    m_image_decoder = Gfx::ImageDecoder::create((const u8*)mapped_file.data(), mapped_file.size());
-    auto bitmap = m_image_decoder->bitmap();
-    if (!bitmap) {
+
+    // Spawn a new ImageDecoder service process and connect to it.
+    auto client = ImageDecoderClient::Client::construct();
+
+    // FIXME: Find a way to avoid the memory copying here.
+    auto decoded_image_or_error = client->decode_image(ByteBuffer::copy(mapped_file.bytes()));
+    if (!decoded_image_or_error.has_value()) {
         show_error();
         return;
     }
 
-    if (m_image_decoder->is_animated() && m_image_decoder->frame_count() > 1) {
-        const auto& first_frame = m_image_decoder->frame(0);
+    m_decoded_image = decoded_image_or_error.release_value();
+    m_bitmap = m_decoded_image->frames[0].bitmap;
+
+    if (m_decoded_image->is_animated && m_decoded_image->frames.size() > 1) {
+        const auto& first_frame = m_decoded_image->frames[0];
         m_timer->set_interval(first_frame.duration);
         m_timer->on_timeout = [this] { animate(); };
         m_timer->start();
     }
 
     m_path = path;
-    m_bitmap = bitmap;
     m_scale = -1;
     reset_view();
 }
@@ -304,18 +311,21 @@ void ViewWidget::set_bitmap(const Gfx::Bitmap* bitmap)
 // Same as ImageWidget::animate(), you probably want to keep any changes in sync
 void ViewWidget::animate()
 {
-    m_current_frame_index = (m_current_frame_index + 1) % m_image_decoder->frame_count();
+    if (!m_decoded_image.has_value())
+        return;
+
+    m_current_frame_index = (m_current_frame_index + 1) % m_decoded_image->frames.size();
 
-    const auto& current_frame = m_image_decoder->frame(m_current_frame_index);
-    set_bitmap(current_frame.image);
+    const auto& current_frame = m_decoded_image->frames[m_current_frame_index];
+    set_bitmap(current_frame.bitmap);
 
-    if (current_frame.duration != m_timer->interval()) {
+    if ((int)current_frame.duration != m_timer->interval()) {
         m_timer->restart(current_frame.duration);
     }
 
-    if (m_current_frame_index == m_image_decoder->frame_count() - 1) {
+    if (m_current_frame_index == m_decoded_image->frames.size() - 1) {
         ++m_loops_completed;
-        if (m_loops_completed > 0 && m_loops_completed == m_image_decoder->loop_count()) {
+        if (m_loops_completed > 0 && m_loops_completed == m_decoded_image->loop_count) {
             m_timer->stop();
         }
     }
diff --git a/Userland/Applications/ImageViewer/ViewWidget.h b/Userland/Applications/ImageViewer/ViewWidget.h
index 4239d338e4..9c5ed9919a 100644
--- a/Userland/Applications/ImageViewer/ViewWidget.h
+++ b/Userland/Applications/ImageViewer/ViewWidget.h
@@ -9,9 +9,8 @@
 
 #include <LibCore/Timer.h>
 #include <LibGUI/Frame.h>
-#include <LibGfx/Bitmap.h>
-#include <LibGfx/ImageDecoder.h>
 #include <LibGfx/Point.h>
+#include <LibImageDecoderClient/Client.h>
 
 namespace ImageViewer {
 
@@ -65,8 +64,8 @@ private:
     String m_path;
     RefPtr<Gfx::Bitmap> m_bitmap;
     Gfx::IntRect m_bitmap_rect;
+    Optional<ImageDecoderClient::DecodedImage> m_decoded_image;
 
-    RefPtr<Gfx::ImageDecoder> m_image_decoder;
     size_t m_current_frame_index { 0 };
     size_t m_loops_completed { 0 };
     NonnullRefPtr<Core::Timer> m_timer;
-- 
cgit v1.2.3