0

Prefix SkottieWrapper factory methods with "Unsafe".

The skottie library uses an internal json parser written
in C++. If a caller were to pass unsafe json data (ex:
downloaded over the network from an untrusted site) to
these factory methods in the browser process, we would be
decoding untrusted data in an unsandboxed process in an
unsafe language (rule of 2 violation).

Prefixing these methods with "Unsafe" is a deterrent. We
considered always rinsing the dadta in SkottieWrapper::Create()
but this introduces added complexity from the asynchronous
nature of the operation, and there' no use case for this
today.

Bug: b:217957131
Change-Id: I9afd6f696f32e6de5f7063dfa3a870399cd07624
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5430278
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Eric Sum <esum@google.com>
Reviewed-by: Florin Malita <fmalita@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1284315}
This commit is contained in:
Eric Sum
2024-04-09 06:54:02 +00:00
committed by Chromium LUCI CQ
parent 9f8c3561b1
commit a20bd3962f
19 changed files with 44 additions and 37 deletions

@ -108,10 +108,12 @@ scoped_refptr<cc::SkottieWrapper> CreateSkottieWrapper(
if (serializable) {
// Create a serializable SkottieWrapper since the SkottieWrapper may have to
// be serialized and transmitted over IPC for out-of-process rasterization.
animation = cc::SkottieWrapper::CreateSerializable(std::vector<uint8_t>(
lottie_data_bytes.begin(), lottie_data_bytes.end()));
animation =
cc::SkottieWrapper::UnsafeCreateSerializable(std::vector<uint8_t>(
lottie_data_bytes.begin(), lottie_data_bytes.end()));
} else {
animation = cc::SkottieWrapper::CreateNonSerializable(lottie_data_bytes);
animation =
cc::SkottieWrapper::UnsafeCreateNonSerializable(lottie_data_bytes);
}
DCHECK(animation);
DCHECK(animation->is_valid());

@ -73,7 +73,7 @@ void BootingAnimationView::Play() {
}
void BootingAnimationView::SetAnimatedImage(const std::string& animation_data) {
auto skottie = cc::SkottieWrapper::CreateSerializable(
auto skottie = cc::SkottieWrapper::UnsafeCreateSerializable(
std::vector<uint8_t>(animation_data.begin(), animation_data.end()));
if (!skottie->is_valid()) {
LOG(ERROR) << "Invalid animation data.";

@ -157,7 +157,7 @@ std::unique_ptr<lottie::Animation> GetCheckmarkAnimation(
std::unique_ptr<lottie::Animation> animation =
std::make_unique<lottie::Animation>(
cc::SkottieWrapper::CreateSerializable(lottie_data.value()),
cc::SkottieWrapper::UnsafeCreateSerializable(lottie_data.value()),
std::move(color_map));
animation->SetPlaybackSpeed(kCheckmarkAnimationPlaybackSpeed);

@ -159,7 +159,7 @@ class ASH_EXPORT TopRowView : public views::View {
IDR_DICTATION_BUBBLE_ANIMATION);
if (json.has_value()) {
use_standby_animation_ = true;
auto skottie = cc::SkottieWrapper::CreateSerializable(
auto skottie = cc::SkottieWrapper::UnsafeCreateSerializable(
std::vector<uint8_t>(json.value().begin(), json.value().end()));
return views::Builder<views::AnimatedImageView>()
.CopyAddressTo(&standby_animation_)

@ -27,7 +27,7 @@ std::unique_ptr<lottie::Animation> GetEqualizerAnimation() {
CHECK(lottie_data.has_value());
return std::make_unique<lottie::Animation>(
cc::SkottieWrapper::CreateSerializable(lottie_data.value()));
cc::SkottieWrapper::UnsafeCreateSerializable(lottie_data.value()));
}
} // namespace

@ -58,7 +58,7 @@ std::unique_ptr<lottie::Animation> GetLottieAnimationData(int animation_id) {
std::unique_ptr<lottie::Animation> animation =
std::make_unique<lottie::Animation>(
cc::SkottieWrapper::CreateSerializable(lottie_data.value()));
cc::SkottieWrapper::UnsafeCreateSerializable(lottie_data.value()));
return animation;
}

@ -42,7 +42,7 @@ bool ServiceSkottieTransferCacheEntry::Deserialize(
GrDirectContext* context,
skgpu::graphite::Recorder* graphite_recorder,
base::span<const uint8_t> data) {
skottie_ = SkottieWrapper::CreateNonSerializable(data);
skottie_ = SkottieWrapper::UnsafeCreateNonSerializable(data);
cached_size_ = data.size();
return skottie_->is_valid();
}

@ -24,7 +24,7 @@ TEST(SkottieTransferCacheEntryTest, SerializationDeserialization) {
kLottieDataWithoutAssets1.length());
scoped_refptr<SkottieWrapper> skottie =
SkottieWrapper::CreateSerializable(std::move(a_data));
SkottieWrapper::UnsafeCreateSerializable(std::move(a_data));
// Serialize
auto client_entry(std::make_unique<ClientSkottieTransferCacheEntry>(skottie));

@ -38,13 +38,17 @@ namespace cc {
class CC_PAINT_EXPORT SkottieWrapper
: public base::RefCountedThreadSafe<SkottieWrapper> {
public:
// Factory methods are "Unsafe" because they assume the `data` comes from a
// trusted source. If this is not the case, use the
// `data_decoder::JsonSanitizer` to rinse the data first.
//
// Creates an instance that can be serialized for IPC. This uses additional
// memory to store the raw animation data.
static scoped_refptr<SkottieWrapper> CreateSerializable(
static scoped_refptr<SkottieWrapper> UnsafeCreateSerializable(
std::vector<uint8_t> data);
// Creates a non serializable instance of the class. This uses less memory.
static scoped_refptr<SkottieWrapper> CreateNonSerializable(
static scoped_refptr<SkottieWrapper> UnsafeCreateNonSerializable(
base::span<const uint8_t> data);
SkottieWrapper(const SkottieWrapper&) = delete;

@ -393,7 +393,7 @@ class SkottieWrapperImpl : public SkottieWrapper {
} // namespace
// static
scoped_refptr<SkottieWrapper> SkottieWrapper::CreateSerializable(
scoped_refptr<SkottieWrapper> SkottieWrapper::UnsafeCreateSerializable(
std::vector<uint8_t> data) {
base::span<const uint8_t> data_span(data);
return base::WrapRefCounted(
@ -401,7 +401,7 @@ scoped_refptr<SkottieWrapper> SkottieWrapper::CreateSerializable(
}
// static
scoped_refptr<SkottieWrapper> SkottieWrapper::CreateNonSerializable(
scoped_refptr<SkottieWrapper> SkottieWrapper::UnsafeCreateNonSerializable(
base::span<const uint8_t> data) {
return base::WrapRefCounted(
new SkottieWrapperImpl(data,

@ -13,14 +13,14 @@ namespace cc {
// a concrete SkottieWrapper implementation is not required.
// static
scoped_refptr<SkottieWrapper> SkottieWrapper::CreateSerializable(
scoped_refptr<SkottieWrapper> SkottieWrapper::UnsafeCreateSerializable(
std::vector<uint8_t> data) {
CHECK(false) << "Skottie is not supported on this platform";
return nullptr;
}
// static
scoped_refptr<SkottieWrapper> SkottieWrapper::CreateNonSerializable(
scoped_refptr<SkottieWrapper> SkottieWrapper::UnsafeCreateNonSerializable(
base::span<const uint8_t> data) {
CHECK(false) << "Skottie is not supported on this platform";
return nullptr;

@ -63,7 +63,7 @@ class MockFrameDataCallback {
TEST(SkottieWrapperTest, LoadsValidLottieFileNonSerializable) {
scoped_refptr<SkottieWrapper> skottie =
SkottieWrapper::CreateNonSerializable(base::span<const uint8_t>(
SkottieWrapper::UnsafeCreateNonSerializable(base::span<const uint8_t>(
reinterpret_cast<const uint8_t*>(kLottieDataWithoutAssets1.data()),
kLottieDataWithoutAssets1.length()));
EXPECT_TRUE(skottie->is_valid());
@ -71,7 +71,7 @@ TEST(SkottieWrapperTest, LoadsValidLottieFileNonSerializable) {
TEST(SkottieWrapperTest, LoadsValidLottieFileSerializable) {
scoped_refptr<SkottieWrapper> skottie =
SkottieWrapper::CreateSerializable(std::vector<uint8_t>(
SkottieWrapper::UnsafeCreateSerializable(std::vector<uint8_t>(
reinterpret_cast<const uint8_t*>(kLottieDataWithoutAssets1.data()),
reinterpret_cast<const uint8_t*>(kLottieDataWithoutAssets1.data()) +
kLottieDataWithoutAssets1.length()));
@ -81,7 +81,7 @@ TEST(SkottieWrapperTest, LoadsValidLottieFileSerializable) {
TEST(SkottieWrapperTest, DetectsInvalidLottieFile) {
static constexpr std::string_view kInvalidJson = "this is invalid json";
scoped_refptr<SkottieWrapper> skottie =
SkottieWrapper::CreateNonSerializable(base::span<const uint8_t>(
SkottieWrapper::UnsafeCreateNonSerializable(base::span<const uint8_t>(
reinterpret_cast<const uint8_t*>(kInvalidJson.data()),
kInvalidJson.length()));
EXPECT_FALSE(skottie->is_valid());
@ -89,11 +89,11 @@ TEST(SkottieWrapperTest, DetectsInvalidLottieFile) {
TEST(SkottieWrapperTest, IdMatchesForSameLottieFile) {
scoped_refptr<SkottieWrapper> skottie_1 =
SkottieWrapper::CreateNonSerializable(base::span<const uint8_t>(
SkottieWrapper::UnsafeCreateNonSerializable(base::span<const uint8_t>(
reinterpret_cast<const uint8_t*>(kLottieDataWithoutAssets1.data()),
kLottieDataWithoutAssets1.length()));
scoped_refptr<SkottieWrapper> skottie_2 =
SkottieWrapper::CreateSerializable(std::vector<uint8_t>(
SkottieWrapper::UnsafeCreateSerializable(std::vector<uint8_t>(
reinterpret_cast<const uint8_t*>(kLottieDataWithoutAssets1.data()),
reinterpret_cast<const uint8_t*>(kLottieDataWithoutAssets1.data()) +
kLottieDataWithoutAssets1.length()));
@ -104,11 +104,11 @@ TEST(SkottieWrapperTest, IdMatchesForSameLottieFile) {
TEST(SkottieWrapperTest, IdDoesNotMatchForDifferentLottieFile) {
scoped_refptr<SkottieWrapper> skottie_1 =
SkottieWrapper::CreateNonSerializable(base::span<const uint8_t>(
SkottieWrapper::UnsafeCreateNonSerializable(base::span<const uint8_t>(
reinterpret_cast<const uint8_t*>(kLottieDataWithoutAssets1.data()),
kLottieDataWithoutAssets1.length()));
scoped_refptr<SkottieWrapper> skottie_2 =
SkottieWrapper::CreateNonSerializable(base::span<const uint8_t>(
SkottieWrapper::UnsafeCreateNonSerializable(base::span<const uint8_t>(
reinterpret_cast<const uint8_t*>(kLottieDataWithoutAssets2.data()),
kLottieDataWithoutAssets2.length()));
ASSERT_TRUE(skottie_1->is_valid());
@ -118,7 +118,7 @@ TEST(SkottieWrapperTest, IdDoesNotMatchForDifferentLottieFile) {
TEST(SkottieWrapperTest, LoadsImageAssetsMetadata) {
scoped_refptr<SkottieWrapper> skottie =
SkottieWrapper::CreateNonSerializable(base::span<const uint8_t>(
SkottieWrapper::UnsafeCreateNonSerializable(base::span<const uint8_t>(
reinterpret_cast<const uint8_t*>(kLottieDataWith2Assets.data()),
kLottieDataWith2Assets.length()));
ASSERT_TRUE(skottie->is_valid());

@ -260,7 +260,7 @@ scoped_refptr<SkottieWrapper> CreateSkottie(const gfx::Size& size,
scoped_refptr<SkottieWrapper> CreateSkottieFromString(std::string_view json) {
base::span<const uint8_t> json_span = base::as_bytes(base::make_span(json));
return SkottieWrapper::CreateSerializable(
return SkottieWrapper::UnsafeCreateSerializable(
std::vector<uint8_t>(json_span.begin(), json_span.end()));
}

@ -181,7 +181,8 @@ class AuthenticatorRequestBubbleDelegate
ui::ResourceBundle::GetSharedInstance().GetLottieData(
bubble_contents_->illustration_light_id);
scoped_refptr<cc::SkottieWrapper> skottie =
cc::SkottieWrapper::CreateSerializable(std::move(*lottie_bytes));
cc::SkottieWrapper::UnsafeCreateSerializable(
std::move(*lottie_bytes));
auto animation = std::make_unique<views::AnimatedImageView>();
animation->SetPreferredSize(gfx::Size(320, 106));
animation->SetAnimatedImage(std::make_unique<lottie::Animation>(skottie));

@ -245,7 +245,7 @@ void AuthenticatorRequestSheetView::UpdateIconImageFromModel() {
std::optional<std::vector<uint8_t>> lottie_bytes =
ui::ResourceBundle::GetSharedInstance().GetLottieData(lottie_id);
scoped_refptr<cc::SkottieWrapper> skottie =
cc::SkottieWrapper::CreateSerializable(std::move(*lottie_bytes));
cc::SkottieWrapper::UnsafeCreateSerializable(std::move(*lottie_bytes));
child_views_.step_illustration_animation_->SetAnimatedImage(
std::make_unique<lottie::Animation>(skottie));
child_views_.step_illustration_animation_->SizeToPreferredSize();

@ -219,7 +219,7 @@ class AnimationTest : public testing::Test {
void SetUp() override {
canvas_ = std::make_unique<gfx::Canvas>(
gfx::Size(kAnimationWidth, kAnimationHeight), 1.f, false);
skottie_ = cc::SkottieWrapper::CreateNonSerializable(
skottie_ = cc::SkottieWrapper::UnsafeCreateNonSerializable(
base::as_bytes(base::make_span(kData, std::strlen(kData))));
animation_ = std::make_unique<Animation>(skottie_);
}
@ -338,7 +338,7 @@ class AnimationWithImageAssetsTest : public AnimationTest {
};
TEST_F(AnimationTest, InitializationAndLoadingData) {
skottie_ = cc::SkottieWrapper::CreateNonSerializable(
skottie_ = cc::SkottieWrapper::UnsafeCreateNonSerializable(
base::as_bytes(base::make_span(kData, std::strlen(kData))));
animation_ = std::make_unique<Animation>(skottie_);
EXPECT_FLOAT_EQ(animation_->GetOriginalSize().width(), kAnimationWidth);
@ -346,7 +346,7 @@ TEST_F(AnimationTest, InitializationAndLoadingData) {
EXPECT_EQ(animation_->GetAnimationDuration(), kAnimationDuration);
EXPECT_TRUE(IsStopped());
skottie_ = cc::SkottieWrapper::CreateNonSerializable(
skottie_ = cc::SkottieWrapper::UnsafeCreateNonSerializable(
base::as_bytes(base::make_span(kData, std::strlen(kData))));
animation_ = std::make_unique<Animation>(skottie_);
EXPECT_FLOAT_EQ(animation_->GetOriginalSize().width(), kAnimationWidth);

@ -141,7 +141,7 @@ gfx::ImageSkia CreateImageSkiaWithCurrentTheme(
std::vector<uint8_t> bytes,
const ui::ColorProvider* color_provider) {
auto content = std::make_unique<Animation>(
cc::SkottieWrapper::CreateSerializable(std::move(bytes)),
cc::SkottieWrapper::UnsafeCreateSerializable(std::move(bytes)),
CreateColorMap(color_provider));
return CreateImageSkia(content.get());
}
@ -151,15 +151,15 @@ gfx::ImageSkia CreateImageSkiaWithCurrentTheme(
gfx::ImageSkia ParseLottieAsStillImage(std::vector<uint8_t> data) {
auto content = std::make_unique<Animation>(
cc::SkottieWrapper::CreateSerializable(std::move(data)));
cc::SkottieWrapper::UnsafeCreateSerializable(std::move(data)));
return CreateImageSkia(content.get());
}
#if BUILDFLAG(IS_CHROMEOS_ASH)
ui::ImageModel ParseLottieAsThemedStillImage(std::vector<uint8_t> data) {
const gfx::Size size =
std::make_unique<Animation>(cc::SkottieWrapper::CreateSerializable(data))
->GetOriginalSize();
const gfx::Size size = std::make_unique<Animation>(
cc::SkottieWrapper::UnsafeCreateSerializable(data))
->GetOriginalSize();
return ui::ImageModel::FromImageGenerator(
base::BindRepeating(&CreateImageSkiaWithCurrentTheme, std::move(data)),
size);

@ -111,7 +111,7 @@ class AnimationGallery : public BoxLayoutView, public TextfieldController {
base::FilePath path(base::UTF16ToWide(file_chooser_->GetText()));
#endif // BUILDFLAG(IS_POSIX)
if (base::ReadFileToString(path, &json)) {
auto skottie = cc::SkottieWrapper::CreateSerializable(
auto skottie = cc::SkottieWrapper::UnsafeCreateSerializable(
std::vector<uint8_t>(json.begin(), json.end()));
animated_image_view_->SetAnimatedImage(
std::make_unique<lottie::Animation>(skottie));

@ -209,7 +209,7 @@ void CreateBitmapsFromAnimatedLottie(int resource_id,
std::optional<std::vector<uint8_t>> lottie_bytes =
ui::ResourceBundle::GetSharedInstance().GetLottieData(resource_id);
scoped_refptr<cc::SkottieWrapper> skottie =
cc::SkottieWrapper::CreateSerializable(std::move(*lottie_bytes));
cc::SkottieWrapper::UnsafeCreateSerializable(std::move(*lottie_bytes));
cursor_animations[type] = std::make_unique<lottie::Animation>(skottie);
}
lottie::Animation* animation = cursor_animations[type].get();