[HN Gopher] The undocumented Android change that led to aCropaly...
___________________________________________________________________
The undocumented Android change that led to aCropalypse was
reported during beta
Author : luu
Score : 90 points
Date : 2023-03-30 20:01 UTC (1 days ago)
(HTM) web link (iliana.fyi)
(TXT) w3m dump (iliana.fyi)
| TazeTSchnitzel wrote:
| I wonder if some developer of an app working with ZIP files will
| have noticed this bug and had to update their app. ZIP is an
| unusually terrible file format because it has a footer rather
| than a header, so the truncation bug would lead to a ZIP file
| with two headers!
| canucker2016 wrote:
| Some ZIP-file handling apps go straight to the end of the ZIP
| file and work backwards looking for the central directory.
|
| I believe others will start from the beginning of the file and
| parse each local directory header and 1) build up their own
| central directory truth, or 2) stop once they've reached the
| start of the central directory.
|
| I recall that the footer-only format was to enable streaming
| out a ZIP file esp. when rewinding to the front of the
| file/stream was not possible, like sending over a network - at
| least that's one of the use cases I recall.
| amethyst wrote:
| Unrelated: the nostalagia of iliana's favicon is incredible. So
| much time spent on old modems watching that animation spin while
| waiting for pages to slowly load in...
| neilv wrote:
| That one was great, and stylish.
|
| The original NCSA Mosaic one with the globe could've been a bit
| much, but it was appropriate, given the impact of the Web, even
| for pre-Web Internet natives: "OMG, this is a tingly step
| forward in distributed global hypertext, it's happening right
| now -- interacting around the world, and also superpowering the
| world." With implied altruistic and benevolent goodness.
|
| (The earlier Mcom/Netscape throbbing "N", when I guess they
| were still rebranding Navigator from Mosaic, looked silly even
| at the time. Then they did a good one. And there were a bunch
| of creative alternatives, including some easter egg ones, which
| JWZ cataloged at one time.)
| neilv wrote:
| Kudos to the engineer who originally not only identified the
| problem, but also raised the concern about breaking behavior, and
| about documenting it.
|
| Looks like the ball was first dropped when the issue was deferred
| without acknowledging the reporter's concise suggestions of how
| to handle it, engineering-wise. Nor a sign that existing code was
| reviewed for the same problem.
|
| Then looks like the ball was dropped again, in what I'm guessing
| might've been: "old version, forget all the old open issues, all
| the issues that still apply, someone will rediscover the hard
| way".
|
| This "aCropalypse" event is an example of how making the wrong
| triage of an issue report can turn out very expensive. All the
| costs to the world of aCropalypse could've been averted. Some of
| those costs might eventually come back to the company.
|
| It's easy to guess why the problem happened and the report wasn't
| handled responsibly. By the time the problem exhibited in a way
| that couldn't be ignored, the pertinent metrics/KPIs/OKRs about
| clearing issues, resource allocations, and product shipment
| schedules were already in the past, bonuses had been paid,
| promotions for shipping new things earned, etc. And security
| problems are treated as inevitable, even if there's a constant
| stream of them, and they're produced faster than they're fixed.
|
| We're going to need software engineers to be accountable for
| things we've done and signed off on.
|
| (Bonus if accountability changes happen near-term: GPT-powered
| open source laundering gets a pause, because bridges start
| falling on everyone foolish enough to sign off on that mangled
| statistical plagiarism.)
| justin_oaks wrote:
| Another case of poorly chosen defaults leading to bugs, leading
| to security issues.
|
| Also a case of violating the Principle of Least Astonishment [0].
| How many people would expect that when you write a file you would
| also have specify that you want it truncated?
|
| [0]
| https://en.wikipedia.org/wiki/Principle_of_least_astonishmen...
| [deleted]
| intelVISA wrote:
| Some say certain agencies wanted it this way.
| jandrese wrote:
| It's notable that in practically every other programming
| language on every platform the default is to truncate the file
| to 0 bytes when opened in write (not append) mode. This isn't
| one of those things where there are two schools of thought
| where people take sides. It's a completely baffling decision to
| buck all convention vs. common sense.
| Someone wrote:
| > It's notable that in practically every other programming
| language on every platform the default is to truncate the
| file to 0 bytes when opened in write (not append) mode
|
| Examples of 'Open' calls that require setting a bit in the
| 'flags' argument to truncate an existing file:
|
| - https://learn.microsoft.com/en-
| us/windows/win32/api/winbase/...
|
| -
| https://man.freebsd.org/cgi/man.cgi?query=open&sektion=2&n=1
| canucker2016 wrote:
| Mentioned in the link.
|
| This seems to be the original PR for the API in question,
| ParcelFileDescriptor#parseMode():
|
| https://android.googlesource.com/platform/frameworks/base/+/...
| + public static int parseMode(String mode) { +
| final int modeBits; + if ("r".equals(mode)) {
| + modeBits = ParcelFileDescriptor.MODE_READ_ONLY;
| + } else if ("w".equals(mode) || "wt".equals(mode)) {
| + modeBits = ParcelFileDescriptor.MODE_WRITE_ONLY
| + | ParcelFileDescriptor.MODE_CREATE +
| | ParcelFileDescriptor.MODE_TRUNCATE; + } else if
| ("wa".equals(mode)) { ...
|
| You can see that "w" and "wt" map to the same mode for file
| operations, including the MODE_TRUNCATE flag.
|
| But we jump ahead a few years and the code is reworked.
|
| from
| https://android.googlesource.com/platform/frameworks/base/+/...:
| public static int parseMode(String mode) { - final
| int modeBits; - if ("r".equals(mode)) { -
| modeBits = ParcelFileDescriptor.MODE_READ_ONLY; -
| } else if ("w".equals(mode) || "wt".equals(mode)) { -
| modeBits = ParcelFileDescriptor.MODE_WRITE_ONLY -
| | ParcelFileDescriptor.MODE_CREATE - |
| ParcelFileDescriptor.MODE_TRUNCATE; - } else if
| ("wa".equals(mode)) { ... - return
| modeBits; + return FileUtils.translateModePosixToP
| fd(FileUtils.translateModeStringToPosix(mode)); }
|
| So what does FileUtils.translateModePosixToPfd(FileUtils.translat
| eModeStringToPosix(mode)) do?
|
| from
| https://android.googlesource.com/platform/frameworks/base/+/...:
| + public static int translateModeStringToPosix(String mode) {
| + int res = 0; + if (mode.startsWith("rw"))
| { + res |= O_RDWR | O_CREAT; +
| } else if (mode.startsWith("w")) { + res |=
| O_WRONLY | O_CREAT; + } else if
| (mode.startsWith("r")) { + res |= O_RDONLY;
| + } else { + throw new
| IllegalArgumentException("Bad mode: " + mode); + }
| + if (mode.indexOf('t') != -1) { + res
| |= O_TRUNC; + } + if
| (mode.indexOf('a') != -1) { + res |= O_APPEND;
| + } + return res; + }
|
| Now "w" and "wt" map to different Posix open mode flags since one
| has to explicitly pass in "t" to get file truncation.
|
| A dev was careless, plain and simple.
|
| Having code in a monorepo doesn't help if you don't check how
| your code changes affects consumers/clients of the code.
|
| If they couldn't change translateModeStringToPosix() behaviour,
| then the dev could've amended parseMode() to map "w' to "wt" to
| keep the old behaviour (ugly and non-orthogonal, yes, but that's
| the way the API was designed).
| canucker2016 wrote:
| Looking at the reworked parseMode API some more and it looks
| like a total mess.
|
| Original code would only accept one of these modes: "r", "w",
| "wt", "wa", "rw", "rwt".
|
| Anything else, and you'll get an IllegalArgumentException
| exception.
|
| which matches the docs:
|
| from
| https://developer.android.com/reference/android/os/ParcelFil...
| Parameters mode String: The string representation of
| the file mode. Can be "r", "w", "wt", "wa", "rw" or "rwt".
| Throws IllegalArgumentException if the given string
| does not match a known file mode.
|
| ----
|
| The modified parseMode code will only throw the
| IllegalArgumentException if the mode string does NOT start with
| "rw", "w", or "r".
|
| You can append any other character(s) to the mode string ("a",
| "t", "x", "y", "z", your fav emoji char), and you won't get an
| IllegalArgumentException exception now.
|
| I wonder what happens when open() gets a mode flag with both
| "a" and "t" set?
|
| parseMode API contract thrown out the window...
| btown wrote:
| A cautionary tale here: when you are testing, if you mock out
| even low-level I/O utilities, you're vulnerable to those
| utilities changing specification subtly. You can supply-chain
| attack yourself by simply updating a library.
|
| Full filesystem/service-engaging integration tests are helpful
| but far from exhaustive - here, one wouldn't have just needed to
| read the cropped file using standard display systems and count
| the pixels, but hash the underlying file in its entirety.
|
| For critical libraries in an I/O pipeline, encourage team members
| to read CHANGELOGs - both before choosing a library to make sure
| they're maintained, and closely when even doing a minor upgrade.
___________________________________________________________________
(page generated 2023-03-31 23:00 UTC)