Skip to content

Commit 4d4bef3

Browse files
authored
Disable libvips unfuzzed operations (#460)
and add test coverage for (un)supported file types. BMP, ICO, and PSD attachments are no longer variable, and the picture views call `variant(:large)` unconditionally, which raises `ActiveStorage::InvariableError`. `Picture#large_image` will fall back to the original attachment.
1 parent 8598072 commit 4d4bef3

10 files changed

Lines changed: 155 additions & 4 deletions

File tree

app/models/picture.rb

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,10 @@ class Picture < ApplicationRecord
55
attachable.variant :large, resize_to_limit: [ 1500, 1500 ]
66
end
77

8+
def large_image
9+
image.variable? ? image.variant(:large) : image
10+
end
11+
812
def markable
913
caption
1014
end

app/views/leafables/show.html.erb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@
2727
action: "lightbox#open:prevent",
2828
lightbox_target: "image",
2929
lightbox_url_value: rails_blob_path(@leaf.picture.image, disposition: "attachment", only_path: true) } do %>
30-
<%= image_tag @leaf.picture.image.variant(:large), loading: "lazy" %>
30+
<%= image_tag @leaf.picture.large_image, loading: "lazy" %>
3131
<% end %>
3232
<% else %>
3333
<%= image_tag "default-picture.webp", alt: "No image uploaded", loading: "lazy" %>

app/views/leaves/_edit.html.erb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
<%= link_to leafable_path(leaf), class: "toc__thumbnail", data: { turbo_frame: "_top" } do %>
88
<%= leaf.section.body if leaf.section? %>
99
<%= sanitize_content(leaf.leafable.body.to_html) if leaf.page? %>
10-
<%= image_tag leaf.leafable.image.variant(:large) if leaf.picture&.image&.attached? %>
10+
<%= image_tag leaf.leafable.large_image if leaf.picture&.image&.attached? %>
1111
<% end %>
1212

1313
<div class="toc__title flex align-center min-width">

app/views/leaves/_leaf.html.erb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@
2727
<%= sanitize_content leaf.page.html_preview if leaf.page? %>
2828

2929
<% if leaf.picture? %>
30-
<%= image_tag leaf.leafable.image.attached? ? leaf.leafable.image.variant(:large) : "default-picture.webp" %>
30+
<%= image_tag leaf.leafable.image.attached? ? leaf.leafable.large_image : "default-picture.webp" %>
3131
<% end %>
3232
</div>
3333

app/views/pictures/_form.html.erb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
<%= leafable_edit_form(picture, id: "leafable-editor") do |form| %>
22
<label class="input input--file input--picture unpad" data-controller="upload-preview">
3-
<%= image_tag picture.image.attached? ? picture.image.variant(:large) : "default-picture.webp", alt: "Picture",
3+
<%= image_tag picture.image.attached? ? picture.large_image : "default-picture.webp", alt: "Picture",
44
data: { upload_preview_target: "image" } %>
55
<%= form.file_field :image, class: "input", accept: "image/png, image/jpeg, image/jpg, image/webp", autofocus: true,
66
data: { upload_preview_target: "input", action: "upload-preview#previewImage" } %>

config/initializers/vips.rb

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
# Disable unfuzzed libvips operations.
2+
#
3+
# Reference the Active Storage transformer to ensure Vips is loaded before we block operations.
4+
ActiveStorage::Transformers::Vips
5+
Vips.block_untrusted(true)
6+
Vips.block("VipsForeignLoadOpenslide", true) # prevent sqlite segfault in forked parallel workers
7+
Rails.application.config.active_storage.variable_content_types -=
8+
%w[ image/bmp image/vnd.microsoft.icon image/vnd.adobe.photoshop ]

test/controllers/leafables_controller_test.rb

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,16 @@ class LeafablesControllerTest < ActionDispatch::IntegrationTest
7676
assert_in_body "title: \"Reading\""
7777
end
7878

79+
test "show a picture whose image cannot be resized" do
80+
leaves(:reading_picture).leafable.image.attach io: file_fixture("pixel.bmp").open,
81+
filename: "pixel.bmp", content_type: "image/bmp"
82+
83+
get leafable_slug_path(leaves(:reading_picture))
84+
85+
assert_response :success
86+
assert_select "figure img[src*=\"pixel.bmp\"]"
87+
end
88+
7989
test "show with markdown format does not escape HTML entities" do
8090
leaves(:welcome_page).leafable.update!(body: "This has <a href='http://example.com'>a link</a>")
8191

test/fixtures/files/pixel.bmp

58 Bytes
Binary file not shown.
Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
require "test_helper"
2+
require "vips"
3+
require "tempfile"
4+
5+
# libvips selects a loader from a file's actual bytes, not from its declared content type. These
6+
# tests pin which loader is selected for each file type under the app's configured loader policy
7+
# (config/initializers/vips.rb).
8+
class VipsLoaderPolicyTest < ActiveSupport::TestCase
9+
# Header bytes are enough for libvips to identify a format; native types are encoded live, exotic
10+
# ones are represented by their magic bytes.
11+
FTYP_AVIF = "\x00\x00\x00\x1cftypavif\x00\x00\x00\x00avifmif1miaf".b
12+
FTYP_HEIC = "\x00\x00\x00\x1cftypheic\x00\x00\x00\x00heicmif1miaf".b
13+
BMP = "BM" + [ 0, 0, 54 ].pack("V3") + "\x00" * 40
14+
PSD = "8BPS" + [ 1 ].pack("n") + "\x00" * 26
15+
ICO = "\x00\x00\x01\x00\x01\x00" + "\x00" * 16
16+
SVG = %q(<svg xmlns="http://www.w3.org/2000/svg" width="8" height="8"/>)
17+
18+
test "loads PNG" do
19+
assert_equal "VipsForeignLoadPngFile", loader_for(encode("png"))
20+
end
21+
22+
test "loads GIF" do
23+
assert_equal "VipsForeignLoadNsgifFile", loader_for(encode("gif"))
24+
end
25+
26+
test "loads JPEG" do
27+
assert_equal "VipsForeignLoadJpegFile", loader_for(encode("jpg"))
28+
end
29+
30+
test "loads TIFF" do
31+
assert_equal "VipsForeignLoadTiffFile", loader_for(encode("tif"))
32+
end
33+
34+
test "loads WebP" do
35+
assert_equal "VipsForeignLoadWebpFile", loader_for(encode("webp"))
36+
end
37+
38+
test "loads AVIF" do
39+
assert_equal "VipsForeignLoadHeifFile", loader_for(FTYP_AVIF)
40+
end
41+
42+
test "loads HEIC" do
43+
assert_equal "VipsForeignLoadHeifFile", loader_for(FTYP_HEIC)
44+
end
45+
46+
test "denies BMP through magickload" do
47+
assert_nil loader_for(BMP)
48+
end
49+
50+
test "denies PSD through magickload" do
51+
assert_nil loader_for(PSD)
52+
end
53+
54+
test "denies ICO through magickload" do
55+
assert_nil loader_for(ICO)
56+
end
57+
58+
test "denies SVG through svgload" do
59+
assert_nil loader_for(SVG)
60+
end
61+
62+
test "denies OpenSlide files through openslideload" do
63+
# OpenSlide files can segfault the embedded sqlite in forked parallel workers
64+
assert_loader_blocked :openslideload, ".svs"
65+
end
66+
67+
test "denies FITS files through fitsload" do
68+
assert_loader_blocked :fitsload, ".fits"
69+
end
70+
71+
test "denies MATLAB files through matload" do
72+
assert_loader_blocked :matload, ".mat"
73+
end
74+
75+
test "denies NIFTI files through niftiload" do
76+
assert_loader_blocked :niftiload, ".nii"
77+
end
78+
79+
test "denies RAW files through dcrawload" do
80+
assert_loader_blocked :dcrawload, ".raw"
81+
end
82+
83+
test "denies VIPS files through vipsload" do
84+
assert_loader_blocked :vipsload, ".vips"
85+
end
86+
87+
private
88+
# Invoke a specific libvips loader directly and assert it is refused because the
89+
# operation is blocked (rather than because the bytes are not a valid image).
90+
def assert_loader_blocked(operation, extension)
91+
Tempfile.create([ "blocked_loader", extension ], binmode: true) do |file|
92+
file.write "not an image"
93+
file.flush
94+
95+
error = assert_raises(Vips::Error) { Vips::Image.public_send(operation, file.path) }
96+
actual = error.message.chomp
97+
98+
# note that exception message may include multiple errors on separate lines,
99+
# so `^` and `$` anchors are used instead of `\A` and `\z`.
100+
if actual =~ /^VipsOperation: class \"#{operation}\" not found$/
101+
skip "libvips does not support #{operation} on this system"
102+
end
103+
assert_match(/^#{operation}: operation is blocked$/, actual)
104+
end
105+
end
106+
107+
def encode(ext)
108+
Vips::Image.black(8, 8).add(128).cast("uchar").write_to_buffer(".#{ext}")
109+
end
110+
111+
def loader_for(bytes)
112+
Tempfile.create(%w[loader_probe .img], binmode: true) do |file|
113+
file.write bytes
114+
file.flush
115+
Vips.vips_foreign_find_load(file.path)
116+
end
117+
end
118+
end

test/models/picture_test.rb

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,4 +12,15 @@ class PictureTest < ActiveSupport::TestCase
1212

1313
assert_nil picture.markable
1414
end
15+
16+
test "large_image is the resized variant of a variable image" do
17+
assert_kind_of ActiveStorage::VariantWithRecord, pictures(:reading).large_image
18+
end
19+
20+
test "large_image is the original image when the image cannot be resized" do
21+
picture = pictures(:reading)
22+
picture.image.attach io: file_fixture("pixel.bmp").open, filename: "pixel.bmp", content_type: "image/bmp"
23+
24+
assert_equal picture.image, picture.large_image
25+
end
1526
end

0 commit comments

Comments
 (0)