Acropalypse: a vulnerability in Google's screenshot editing tool | Hacker News Reader

Acropalypse: a vulnerability in Google's screenshot editing tool

twitter.com

For a hint at how the bug works, see this https://issuetracker.google.com/issues/180526528 (more details coming soon™)

From https://twitter.com/David3141593/status/1636979466860744704

Also: you [can] do a basic check with tools like exiftool - it will report "Warning: [minor] Trailer data after PNG IEND chunk" on vulnerable images.

From: https://twitter.com/David3141593/status/1636981307891671041

I still can’t believe they changed the meaning of the “w” flag. I had never heard of the “wt” file mode. Does that exist on other POSIX systems?

Python already uses the "t" character with a very different meaning: opening in text mode.

That part is amazing: it calls into question the entire Android code review process that nobody thought breaking compatibility wasn’t a problem, much less doing so in a way which looks like one of the most familiar interfaces in the world. It seems unlikely that this isn’t just the first, most visible bug.

In case anybody is interested, it looks like they refactored the mode translation code to reuse another function, and the behaviour of that function was different from the original.

There were no unit tests written for the original implementation, but they did update the tests for the refactored function [1], and the tests clearly show different behaviour from the original implementation [2].

My best guess would be that the code wasn't reviewed.

[1] https://cs.android.com/android/_/android/platform/frameworks...

[2] https://cs.android.com/android/_/android/platform/frameworks...

Thanks for digging into that history!

As bad (but on the png side, not the fs library side), if the app crashes mid crop, then this misuse of the posix API means the original image will be corrupted.

They should be doing a “mktemp; write; sync; rename”, which atomically and durably replaces the file in most linux file systems.

There might also be an exploitable race where you overwrite the file in place while it is being parsed, leading to undefined behavior in applications attempting to read the file.

Nice write-up here: https://www.da.vidbuchanan.co.uk/blog/exploiting-acropalypse...

The fact this wasnt triaged as a security bug is unforgivable https://issuetracker.google.com/issues/180526528

> The fact this wasnt triaged as a security bug is unforgivable

Hyperbole like this is unhelpful. The reporter didn't think of it as a security bug, and the discussion in the bug itself is about API compatibility and documentation concerns.

Pretending in hindsight that we're all too smart to have ever missed this isn't helping software quality for anyone, and good postmortem analysis doesn't throw around words like "unforgivable".

It casts serious doubt on the Android team as a steward of the platform if this was reviewed by multiple "product and engineering teams" and not a single person thought that fundamentally changing a filesystem API might have security (or at least data integrity!) repercussions. It doesn't seem like any follow-up analysis was done at all here, other than a "thanks for the fix." Hyperbole or not, that's a terrible response.

And I repeat: that is exactly the "pretending in hindsight that we're all too smart to have missed this" trap I warned about. It's exactly the opposite of good postmortem analysis, because it inevitably leads to a "be smarter" proscription, which is unactionable.

Also, in practice, you and I and everyone here are absolutely dumb enough to do this. Hubris is another terrible postmortem technique.

I'm quite confident that I'd have spotted the security relevance at the time, and I have a track record of finding "implementation bugs" given only APIs and specifications. But, I'm a security researcher, not a software engineer.

My takeaway would be that they should have security-brained people screening "non-security" bugs, to check for potential security relevance.

But there's a bar somewhere, right? If I reported a bug that your lock screen could be bypassed by entering 0000, you'd expect that to trigger a security response, right? If you're saying it's always hubris to expect better, then you're being foolish. We're arguing over where the bar is.

I'm not defending anything here. I'm saying that you guys launching verbiage like "unforgivable" or "casts serious doubt on" is unhelpful. Instead, try thinking about how problems like this can be avoided.

Here's an example: the original change was a compatibility regression. Clearly there should have been a test of the original code somewhere that opened a file with "w" and validated that it was truncated per the documentation. And there wasn't. So one recommendation might be an audit of unit tests to verify that there's a process for getting from documented behavior to validated behavior.

And importantly, there's no need to "doubt" or "forgive" to do that.

You seem to be coming at this as a member of the Android team. I'm coming at it as a user. It's not my job to make it better, but it is my job to make informed decisions over whether or not this software will screw me if I use it. So yes, their ability to identify security issues is important and relevant.

Not sure how you came to this conclusion in this particular case. File APIs have been around for decades, and are well understood.

Changing the behaviour of a file mode from truncate to not-truncate is a questionable decision because these are very explicit options a developer would carefully select, and therefore would not expect to see a change in behaviour to something already covered by a different option.

I work in finance and I can confidently say it would be at the top of my mind that this kind of change would result in leaking data or corrupting data because I regularly explicitly choose truncation to avoid exactly that when I generate new reports.

It's also ironic that you complain about hyperbole then accuse people of "pretending to be smart". You don't need to be smart to see this as a security issue, you just need some real world development experience.

> Changing the behaviour of a file mode from truncate to not-truncate is a questionable decision

Looking at the commit which changed this behavior: https://cs.android.com/android/_/android/platform/frameworks...

it seems like it was a refactoring gone wrong, and not a conscious decision to change the behaviour.

Nice find. You may be correct that it wasn't intentional due to the lack of documentation around the change, but I do note they updated tests for the refactored function [1], and the tests clearly show different behaviour from the original implementation [2].

I'm surprised this wasn't picked up during code review. Also surprised no unit tests for the original implementation, just the new one - if you're going to refactor something, add tests for the original implementation if there aren't any already.

[1] https://cs.android.com/android/_/android/platform/frameworks...

[2] https://cs.android.com/android/_/android/platform/frameworks...

Who gets promoted faster: The person who spends an extra hour or two on every code review investigating the behavior of hacky wrappers 50-year-old awkward file APIs, looking for the one and 100 critical security bug? Or The person who gets more features launched?

Someone has read their Dekker :)

Individuals can miss anything, regardless of how smart they think they are. I ment the processes which failed to pick up this oversight where unforgivable.

Who missed what? The bug tracker response suggests this was an intentional change. You are trying to whatabout it in a million ways but the reality is that, no, subtly changing the meaning of POSIX open modes is unhinged.

What the reporter thought is irrelevant, he's not expected to know any better. The triaging team on the other hand is

These types of issues are exactly why whenever it’s sensitive, I screenshot, crop/edit, then screenshot the crop’d/edited screenshot. There’s other possible issues than this bug (like iOS’s non-destructive edits by default), so it’s better to be safe than sorry.

> These types of issues are exactly why whenever it’s sensitive, I screenshot, crop/edit, then screenshot the crop’d/edited screenshot.

Yeah I always do the same and I'm happy to see I'm not the only one. And a CVE like these shows that we're the ones "not seeing things".

Redacting PDFs is similarly tricky. These days I use macOS Preview (they have a new-ish feature that explicitly allows for redacting) which works but I sometimes still open it in an editor to make sure the data isn't there lol.

Wait. Wouldn’t that mean that cropped images have the same file size as the uncropped version? Nobody noticed that in all those years?

Correct! Or rather, nobody investigated deep enough. Frustratingly, I noticed at one point, and incorrectly concluded that fractional image scaling must've been worsening the compression ratio. I was only using my phone in the first place because I wasn't at a PC, so a deeper investigation wasn't really an option at the time.

A demo is available here: https://acropalypse.app/

I tried a bunch of old screenshots and always got error. Is the demo broken or is this not always possible to recover?

The latter. (although the former is also plausible)

I've found that these conditions are needed on my Pixel 5:

1. Using the cropping tool given when taking the screenshot (using the Markup tool in any other way does not work for me)

2. You have to crop at least 2 sides

Also, I'm not able to recover the rest of the image once it is put through Discord.

I was not able to replicate with a 7 pro.

Did you install the update from yesterday? It contains the fix so if you installed it, you won't be able to replicate anymore. Try finding old screenshots which meet the requirements.

I don't see any sort of background at all on how it's working. There's metadata in the image that helps reconstruct the original? Or something about how Discord does the attachment, or?

Ah, one commenter offered this:

"It looks like when the edits make the PNG smaller it saves the original number of bytes, overflowing its own buffer and leaving a bunch of unintended IDAT chunks to find :). Did you talk to Google about this before taking to twitter though?"

https://twitter.com/Bottersnike237/status/163689272301266534...

The quoted comment is pure speculation, by the way - there is no buffer overrun (at least, not in the traditional sense), they just forgot to truncate the original file on-disk before writing the cropped version.

Ah, makes sense...I see your other comment leading to more detail also. Thanks!

It wasn’t so much that they forgot but that Android 10 changed the meaning of the “w” file mode flag for open.

IIUC the Markup tool was introduced after that change, but I could be wrong. (Although it was before the change was documented either way...)

I can crop screenshots with a Google screenshot app??? I always use Snapseed

Little detail on how this works. I initially thought it's based on thumbnails, but the high quality of the recovered image and the damage at the beginning makes me think it's something else.

tl;dr they open the original image without O_TRUNC, and write the cropped version into the same file.

This thread provides a good overview and some sample images https://mastodon.delroth.net/@delroth/110043776803548821

Explanation here https://www.da.vidbuchanan.co.uk/blog/exploiting-acropalypse...

Reply on news.ycombinator.com