> 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.
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.
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?
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.
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.
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.
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.
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.
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.
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.
30 comments
[ 3.8 ms ] story [ 80.3 ms ] threadHyperbole 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".
Also, in practice, you and I and everyone here are absolutely dumb enough to do this. Hubris is another terrible postmortem technique.
My takeaway would be that they should have security-brained people screening "non-security" bugs, to check for potential security relevance.
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.
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.
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.
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...
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...
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".
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.
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.