Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add .webp support #614

Closed
eloquence opened this issue Sep 11, 2020 · 17 comments
Closed

Add .webp support #614

eloquence opened this issue Sep 11, 2020 · 17 comments

Comments

@eloquence
Copy link
Member

We should add a .webp capable image viewer to the sd-viewer template. Currently we can convert these files via ImageMagick, but they cannot be successfully opened from the SecureDrop Client.

@eloquence
Copy link
Member Author

(Just confirming that's still on an issue on 4.1/Bullseye)

@philmcmahon
Copy link

We can confirm this is still an issue on 4.1 - the viewer VM starts up then shuts down with no warning

@eaon
Copy link
Contributor

eaon commented Sep 23, 2022

Unfortunately the library that would enable eog to open .webp images isn't available in regular bullseye repos, but it is available in bullseye-backports

I'm curious what folks think about adding the backports repo for this, or maybe we want to mirror the package in our own repo?

@legoktm
Copy link
Member

legoktm commented Sep 23, 2022

I'm curious what folks think about adding the backports repo for this, or maybe we want to mirror the package in our own repo?

I double-checked and all the dependencies of webp-pixbuf-loader are in bullseye (and not also in backports), so mirroring the package will be straightforward, if that's what we go with.

IMO -backports repos are the equivalent of testing, in that it's often changing, with only a 2-5 days advance warning. My preference is mirroring, just so we can test any updates ahead of time (and we can use the same apt-test automation we use for Tor packages).

The downside of -backports packages (regardless of whether we mirror or not) is that there's no coverage by the Debian Security Team, it's up to the maintainer, with a 2-5 day delay. Though of course the use of Qubes mitigates some of the security issues, though I defer to others on actual analysis/weighting of that.


My other naive suggestion if we're unhappy with backports would be to use something like imagemagick to convert to jpg/png/etc. on the fly and then pass that over to eog. Unsure if that kind of conversion is too much for journalists who want to see the original file.

@eaon
Copy link
Contributor

eaon commented Sep 27, 2022

@L3th3 @lsd-cat What's your take on using webp-pixbuf-loader? There is aruiz/webp-pixbuf-loader#50 which makes me wonder if it's a bit premature to add it, but conversion doesn't sound amazing either (especially since .webp supports animation IIRC and I dunno how annoying it would be dealing with that)

@nathandyer
Copy link

Just to add a bit of outside context, webp-pixbuf-loader was just added as a core dependency to GNOME Shell (they're switching to .webp for backgrounds): https://gitlab.gnome.org/GNOME/gnome-build-meta/-/commit/4bf5ae30f4a3d39e6aa87d6f116e8da2bbfaae63

Eventually this would trickle down anyway, but I think that shows it's fairly battle-tested if we want to jump the gun and include it/mirror it.

@ghost
Copy link

ghost commented Sep 28, 2022

Took a look at the project, and it really doesn't seem battle-tested. As far as I can tell, the PR that @eaon linked seems to be the only type of security review its gotten, and it was just fuzzing (and the fixes proposed are a perfect showcase of why we should generally start favoring Rust over C/C++). As for its adoption into GNOME, looking over their issue & PR, security doesn't seem to have been a consideration at all.

I really wouldn't be surprised if there weren't more bugs in the codebase, so recommending it in its current state would be a no from me

@eaon
Copy link
Contributor

eaon commented Sep 28, 2022

From a quick survey, I haven't found any native Rust implementations for webp decoding that we could use. Everything seems to fall back on libwebp, the same library that webp-pixbuf-loader uses. ImageMagick uses that as well, and the history of https://github.com/ImageMagick/ImageMagick/blob/main/coders/webp.c doesn't inspire all that much confidence either.

Other than a "no", from your point of view, what's an approach that would allow us to support .webp images in one way or another that you'd be OK with?

@eaon
Copy link
Contributor

eaon commented Oct 28, 2022

(As I was cleaning up tabs I came across the post-fuzzing webp-pixbuf-loader PR again - it was closed but changes to address the issue were pushed to the repo a couple hours ago.)

@zenmonkeykstop
Copy link
Contributor

zenmonkeykstop commented Oct 28, 2022

It's kindof basic, but in Debian adding webp support via the 🥁 webp package also installs vwebp, which would seem to do the job. It uses https://chromium.googlesource.com/webm/libwebp which seems pretty active and well-supported. So we could just skip the gnome bits at the cost of some UX iffyness. (Converting files to png via dwebp and then viewing them might also be an option.)

@eloquence
Copy link
Member Author

So, to recap, if I understand correctly, there are two primary options under discussion:

  • eog, which would require webp-pixbuf-loader from Bullseye backports. @L3th3 expresses concerns above about the maturity of the webp-pixbuf-loader component.
  • vwebp, which is installable via the webp package in Bullseye (but which provides no friendly GUI like eog does, nor any advanced viewer functionality like zooming; it only displays the file in a frame).

I think implicit in this choice are a couple of questions:

  • Does vwebp have any technical maturity issues similar to webp-pixbuf-loader that would make it not a suitable option?
  • What level of maturity is required for a viewer application to be considered suitable to run in a networkless, disposable VM?
  • If eog with webp-pixbuf-loader was determined to be the best option, would we consider it acceptable to install it from the backports channel?

If I understand correctly, we would have to solve separately for the printability of webp files.

What do y'all think, particularly @L3th3 (and @lsd-cat if you have a chance to weigh in) from a security perspective? I'll post my personal take as a separate comment to distinguish it from the summary above.

@eloquence
Copy link
Member Author

Personal take: Based on the comments above, it seems like installing vwebp via the webp package seems like the pragmatic option for Bullseye (so we satisfy the immediate user need), and we could have a longer range discussion (informed by our threat model) about better defining our standards for viewer applications, which is also relevant in the context of other issues like #842.

@legoktm
Copy link
Member

legoktm commented Sep 16, 2023

CVE-2023-4863 in libwebp was actively exploited by NSO Group and required critical fixes in iOS, Firefox, Chrome, Tor Browser and more so it seems both important that we can provide this functionality in safe manner as well as treading carefully on which webp rendering implementation we pick.

@eaon
Copy link
Contributor

eaon commented Sep 18, 2023

FYI, it looks like https://github.com/image-rs/image can decode webp images without the use of libwebp

@legoktm
Copy link
Member

legoktm commented Jul 9, 2024

Now that we're on bookworm, eog now depends on webp-pixbuf-loader, so it's already installed in the large template.

@legoktm
Copy link
Member

legoktm commented Jul 9, 2024

I tested this and it all works fine on 1.0.0rc2.

I tried a still webp image, and an animated webp image, both rendered just like how Firefox does.

I will put it on the agenda for tomorrow's meeting just to make sure there are no unresolved security issues here, but as it stands, webp should be supported in the next SDW release.

@legoktm
Copy link
Member

legoktm commented Jul 11, 2024

I will put it on the agenda for tomorrow's meeting just to make sure there are no unresolved security issues here, but as it stands, webp should be supported in the next SDW release.

There were no security concerns. There was some concern that it didn't require us to edit any of our mime lists, and that stronger control would be ideal, but that doesn't block this. Some related tickets for follow-up: freedomofpress/securedrop-client#2007 (comment), #1139.

I'll close this issue by adding a changelog entry.

legoktm added a commit that referenced this issue Jul 11, 2024
This was an indirect consequence of switching to bookworm, but let's
explicitly flag it since it was a user-requested feature.

Fixes #614.
@legoktm legoktm closed this as completed Jul 11, 2024
legoktm added a commit that referenced this issue Jul 16, 2024
This was an indirect consequence of switching to bookworm, but let's
explicitly flag it since it was a user-requested feature.

Fixes #614.

(cherry picked from commit a9e3cb4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants