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

[security] warn users about security issues with pixelation #3763

Open
jrpie opened this issue Oct 23, 2024 · 5 comments · May be fixed by #3765
Open

[security] warn users about security issues with pixelation #3763

jrpie opened this issue Oct 23, 2024 · 5 comments · May be fixed by #3765
Labels
discussion Enhancement Feature requests and code enhancements Needs Decision This is something that should be discussed by community

Comments

@jrpie
Copy link

jrpie commented Oct 23, 2024

In the past there have been several issues regarding the problem of pixelation not being secure. Those were closed with comments like "use a black box instead" or "increase the kernel size". However the average user might not even be aware of the fact that there are security issues. What I find especially worring is the fact that pixelations looks much more secure on first glance than it actually is. So simply telling users to use a "large enough" kernel size doesn't help either.

I'd argue that users view pixelation as a security tool, hence it should be treated as such. It was previously proposed to abolish the feature entirely (#1466) - which I think would be the best solution - but that was rejected.

As a middle ground, I'd suggest to warn users about issues of the tool. E.g. when using pixelation for the first time or after a prolonged period of not using it, flameshot could show a popup saying something like:

Warning! Pixelation is not a security tool! Secrets - especially text - can be recovered from pixelated areas in many situations.
To securely hide contents from the screenshot use a black box instead. More information

@AlexP11223
Copy link
Contributor

when using pixelation for the first time or after a prolonged period of not using it, flameshot could show a popup

Properly adding a popup on the capture screen can be complex. Maybe it should be just in the description (on hover) instead. I think it also has a better chance that the user will actually read it at some point instead of just clicking "ok" while in a rush and forgetting.

@mmahmoudian
Copy link
Member

Thanks for your comments, details, and PRs.

I personally disagree with some of the parts mentioned here because:

  1. (Lets start with a fun one😅) This is analogous to putting a large warning on microwave and tell people not to put their cat inside the microwave to dry their hair! Or even worse, abolishing microwaves entirely! A tool is a tool, and the ultimate result depends on user's understanding and intelligence.
  2. The warning solution is no different than "use a black box instead". It is even slightly less productive as it does not provide an alternative solutions.
  3. There are feature requests amd PRs to randomize the pixels (e.g [FEAT] added randomize pixels function to pixelate tool #3368), and even one that is created by yourself yourself ([security] replace pixelation by pseudo-pixelation  #3765). Both are great efforts to improve security while giving the user some level of freedom, and should not go to waste imho.

I'm not totally against adding a benign warning, and I agree with @AlexP11223 that a tooltip has higher chance of being read by the user. History have proved time and time again that even the text that we show on every screenshot and is literally in the middle of the page is entirely ignored by users. Let alone a warning that is shown once.

I believe the better overall solution is to combine multiple approaches:

  1. Allow the user to choose between different type of pixelation (e.g. in the configuration, or by scrolling over the pixel tool button, or clicking it and holding Ctrl or ...)
  2. If/when the least secure mode was selected, change the tooltip with required information
  3. Add the information that @jrpie, others, and me have gathered along the years regarding this issue into one documentation page and add it to Flameshot website. Education imho is a key factor.

This way we are not dictating to the user what to use, but we have given them the info and the freedom of choice.

What do you all think about this compound approach? It would beed some additional code to keep things side-by-side.

@mmahmoudian mmahmoudian added Enhancement Feature requests and code enhancements Needs Decision This is something that should be discussed by community discussion labels Oct 30, 2024
@jrpie
Copy link
Author

jrpie commented Oct 30, 2024

I totally agree. When opening this issue I was mostly concerned to do something as quickly as possible. Similar issues had been raised multiple times in the past and the maintainers didn't want to remove the tool, so warning the users seemed like the next best thing.
A popup is of course bad security but seemed better than doing nothing and I also agree that a tooltip is better.

The current implementation of #3368 has severe flaws and I'm sceptical of the general approach.

Of course I'm in favor #3765 ^^ However when opening this issue I neither thought that I would have time so soon nor that the effect would look as nice.

I like the idea of letting the user switch between different modes, but I'm not sure about the UI for that. In particular:

  • Should the change be permanent?
    I'd argue against that, as one might want to use pixelation once (e.g. for a large image, where it might actually be sane - and is visually nicer than pseudo-pixelation) and forget about it. IMO the default should be secure.
  • How do we make the feature discoverable?
  • How do we prevent the user from switching accidentally?
    Scrolling while hovering over the icon seems like something that can easily haplen by accident

@FelixJochems
Copy link

FelixJochems commented Nov 2, 2024

Didn't have the time to fully read it before now, I vastly prefer #3765 over #3368

Personally I think the change should just be permanent, the original pixelation tool is from back when Unredacter & Depix weren't invented yet. It's analogous to passwords being "1234", just not viable anymore at this point in time.

The main reason people want to use pixelation is to show people what kind of content was behind it (eg. a picture or a piece of text) without revealing said information.
I think #3765 is a nice compromise, it doesn't fully look like static white noise as #3368 and it doesn't outright remove the feature. It's also well documented.

Having said that, @jrpie is it okay if I copy your code for my fork?

@jrpie
Copy link
Author

jrpie commented Nov 2, 2024

Having said that, @jrpie is it okay if I copy your code for my fork?

Of course, that's why it is FOSS after all ^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Enhancement Feature requests and code enhancements Needs Decision This is something that should be discussed by community
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants