On Sat, Jul 13, 2013 at 1:42 AM, Nick Kralevich <n...@google.com> wrote: > According to the ZIP spec, the values are suppose to be unsigned > (http://www.pkware.com/documents/casestudies/APPNOTE.TXT sections 4.3.12 and > 4.4.1.1). The bug was interpreting those fields as a signed value instead of > an unsigned value. A negative { namelen, extralen, or bytecount } doesn't > make any sense - how do you have a negative length? Exactly. Why is/was it being read as a signed value rather than an unsigned short? As you stated, it violated the specification and made no logical sense.
The problem appears to be BufferIterator design, which was not fixed (https://android.googlesource.com/platform/libcore/+/master/luni/src/main/java/libcore/io/BufferIterator.java). It does not offer unsigned reads. In the absence of fixing BufferIterator, DataInputStream is a possibility. DataInputStream has a readUnsignedShort (http://developer.android.com/reference/java/io/DataInputStream.html). And that's not the only instance of using the wrong datatypes (https://android.googlesource.com/platform/libcore/+/d0bd4c19678429b19ac4b9c7477b28241d5dd1db/luni/src/main/java/java/util/zip/ZipEntry.java) (sans the uncommitted fixes). nameLength is not validated, so a 0 size allocation could occur. It will likely be found later with an obscure error rather than the point of failure (I've experienced it before with BigIntegers and exponent sizes). Interestingly, commentByteCount and extraLength are validated. > It's perfectly legal, according to the ZIP standard, to have a { namelen, > extralen, or bytecount } of length 65534. The best I can tell, the code still allows "holes" (forgive me if I am misreading the source files). Are there any reasons not to enforce a well defined sequential layout? What problem is trying to be solved by allowing the sloppiness? I respect Jon Postel, but his law is poison. Look for any reason to reject an input. If you can't find a reason, then begrudgingly perform the operation. Code stability will increase by orders of magnitude. Jeff > On Fri, Jul 12, 2013 at 10:24 PM, Jeffrey Walton <noloa...@gmail.com> wrote: >> ... >> I was reading through Bug 9695860 write up (available at >> >> http://www.androidpolice.com/2013/07/11/second-all-access-apk-exploit-is-revealed-just-two-days-after-master-key-goes-public-already-patched-by-google/). >> >> Am I the only guy scratching my head in disbelief in AOSP's >> unwillingness to validate fields properly? What's with the stupid >> programmer tricks of forcing a negative value to positive? Those silly >> tricks just turned -2 into 65534, which is still probably incorrect. >> Is there any reason an APK is not rejected as malformed? -- You received this message because you are subscribed to the Google Groups "Android Security Discussions" group. To unsubscribe from this group and stop receiving emails from it, send an email to android-security-discuss+unsubscr...@googlegroups.com. To post to this group, send email to android-security-discuss@googlegroups.com. Visit this group at http://groups.google.com/group/android-security-discuss. For more options, visit https://groups.google.com/groups/opt_out.
