21 ms·

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

For a hint at how the bug works, see this https://issuetracker.google.com/issues/180526528 https://issuetracker.google.com/issues/180526528 (more details coming soon™) From https://twitter.com/David3141593/status/1636979466860744704 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 https://twitter.com/David3141593/status/1636981307891671041

wffurr 4y ago
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?
- progval 4y ago
  Python already uses the "t" character with a very different meaning: opening in text mode.
- acdha 4y ago
  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.
  - ryanjshaw 4y ago
    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/base/+/63280e06fc64672ab36d14f852b13df2274cc328:core/tests/coretests/src/android/os/FileUtilsTest.java;dlc=7bd671e11fcfe956b78087c9ac27f25c1dee6e3e https://cs.android.com/android/_/android/platform/frameworks... [2] https://cs.android.com/android/_/android/platform/frameworks/base/+/63280e06fc64672ab36d14f852b13df2274cc328:core/java/android/os/ParcelFileDescriptor.java;dlc=7bd671e11fcfe956b78087c9ac27f25c1dee6e3e https://cs.android.com/android/_/android/platform/frameworks...
    - acdha 4y ago
      Thanks for digging into that history!
- hedora 4y ago
  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.

wffurr 4y ago

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?

progval 4y ago
Python already uses the "t" character with a very different meaning: opening in text mode.
acdha 4y ago
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.
- ryanjshaw 4y ago
  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/base/+/63280e06fc64672ab36d14f852b13df2274cc328:core/tests/coretests/src/android/os/FileUtilsTest.java;dlc=7bd671e11fcfe956b78087c9ac27f25c1dee6e3e https://cs.android.com/android/_/android/platform/frameworks... [2] https://cs.android.com/android/_/android/platform/frameworks/base/+/63280e06fc64672ab36d14f852b13df2274cc328:core/java/android/os/ParcelFileDescriptor.java;dlc=7bd671e11fcfe956b78087c9ac27f25c1dee6e3e https://cs.android.com/android/_/android/platform/frameworks...
  - acdha 4y ago
    Thanks for digging into that history!
hedora 4y ago
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.