[omni]

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.

[ajross]

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.

[ryanjshaw]

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.

[progval]

> 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.

[ryanjshaw]

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...

[hgsgm]

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?

[Retr0id]

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.

[omni]

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.

[ajross]

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.

[omni]

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.

[LambdaComplex]

Someone has read their Dekker :)