
Android Bug Superior to Master Key - piratebroadcast
http://www.saurik.com/id/18
======
MichaelGG
Perhaps interesting historical from Gosling on why Java has such limited
primitives:

"almost no C developers actually understand what goes on with unsigned, what
unsigned arithmetic is. Things like that made C complex. The language part of
Java is, I think, pretty simple"

I do agree with the sentiment. All that type coercion in C is insane. But
removing unsigned doesn't seem like the fix. Perhaps if languages stopped
doing automatic conversions that'd solve the problem.

~~~
chacham15
> "almost no C developers actually understand what goes on with unsigned, what
> unsigned arithmetic is.

This seems like a large exaggeration unless I am one of such people. Correct
me if I am wrong, but the arithmetic is simple (i.e. simple binary addition).
What tricks people is when they start to mix unsigned and signed arithmetic.
Even then, its only that they dont know which one is getting casted to the
other (or not knowing the consequence of such a cast; e.g. how right shift is
different with signed and unsigned numbers). That is a very simple thing that
takes all of a google rtt to figure out.

~~~
barrkel
Far too many C programmers think that unsigned ints are an appropriate type to
use simply because the quantity in question can never be negative, e.g. the
length of a list or the size of a memory allocation. What they fail to
appreciate is that almost inevitably such values will get used in subtraction
operations down the road, and then your chances of having a corner-case bug
grow exponentially.

I doubt more than 1% of non-security-hardened extant C programs are completely
safe when any unsigned quantity in the program may acquire a value with the
high bit set.

~~~
chacham15
> Far too many C programmers think that unsigned ints are an appropriate type
> to use simply because the quantity in question can never be negative, e.g.
> the length of a list or the size of a memory allocation.

size_t is the type for such things and it is defined by the standard to be
unsigned. I would like to point out that since it is the standard that defined
it, it is not the fault of an average programmer.

> What they fail to appreciate is that almost inevitably such values will get
> used in subtraction operations down the road, and then your chances of
> having a corner-case bug grow exponentially.

C has always had the approach that the language is hands off; meaning that if
the programmer wants to shoot themselves in the foot they very well can.
Furthermore, not using an unsigned int is not going to help the issue for two
reasons: you might very well need the entire address space provided by the 32
bits and somewhere down the line you will eventually need to cast to size_t
when you call malloc. The solution is to simply make sure that you test for
overflow/underflow where appropriate or choose a different language.

~~~
barrkel
The C standard specifies a good proportion of bad ideas, though mostly out of
backward compatibility.

Just consider size_t and its friend ptrdiff_t. Suppose you're trying to copy a
slice out of an array. If the array indices are size_t, the length of your
slice is ptrdiff_t, which you cannot then safely pass to malloc.

There are two solid solutions; (a) limit allocation to the positive range of
the signed integer type (e.g. 2GB in 32-bit apps), or (b) use the next signed
type up. C, being a systems-level language, doesn't want to take the
limitations of (a) nor the platform dependence of (b). The result is that it
makes it very hard to get right for average programmers not used to writing
OSes and standard libraries.

It's no theoretical issue. When I was working on the Delphi compiler, I had to
audit the runtime library to eliminate integer overflow issues. It wasn't
trivial to catch them all, and they can become security bugs with medium
difficulty. It was eye-opening enough to make me wary.

------
kevingadd
It's interesting to see that the old reader class had a method 'readShortLE'
that read an _unsigned_ LE short and returned it as a signed integer; the
'readShort' being called in the new buggy code reads a _signed_ short and
returns it as a signed integer. Unfortunate misuse of naming (in the first
case, naturally). I certainly would not have guessed the difference were I
just reading the code.

It's unfortunate that this breakage all started because the original (correct,
it seems?) code was too slow due to method call overhead. If people hadn't
felt the need to optimize, this bug probably wouldn't be there - but I guess
the Dalvik VM can't inline in that scenario, or something.

~~~
saurik
The code they have right now is slower with respect to method calls than all
previous implementations. The C code worked. The low-memory Java that replaced
it worked. The Java version that used less method calls worked. It was the
person who then came in later whose job was not optimization that broke the
reader while trying to fix a correctness bug.

One argument is that the code was now sufficiently complex due to the
optimization that it was bound to confuse him. Another is that the other bug
(the CRC one) was caused during the optimization sweeps that could have been
handled elsewhere. (So your point to me largely stands, but I wanted to
correct the timeline. I agree that spending time fixing inlining to help the
entire codebase would have stopped the timeline of events that led to here,
possibly not just on accident.)

------
Scaevolus
By comparison, Windows 8 app packages (.appx) signatures involve signing
hashes of the entire zip file minus the signature file itself.
[http://blogs.msdn.com/b/windowsappdev/archive/2012/12/04/des...](http://blogs.msdn.com/b/windowsappdev/archive/2012/12/04/designing-
a-simple-and-secure-app-package-appx.aspx)

If anyone knows how iOS .ipa packages are signed, I'd love to hear it.

~~~
saurik
Interestingly, they are doing both; this is similar to how SignApk (which is
used to sign Android update.zip files) works. Microsoft's first layer of "sign
all of the files" looks very similar to jarsign: both embed files inside of
the zip file that include the signature, and both have a signature that signs
a manifest that in turn is a list of hashes.

The only differences at that level of the signature process (before the whole
file signature) are that 1) jarsign has one further indirection (whose purpose
is to allow some metadata to be hashed, as well as to support multiple people
signing different parts of the file) and 2) the Microsoft manifest signs
blocks of files, not entire files (for efficiency).

That article does not describe the second layer of signature verification (the
one that signs the whole file) but does mention it a few times. FWIW, as I
bring up in an aside of my article, Android's update.zip files made a mistake
in how they attached the signature to the end of the zip file, and ended up
with a signing bug back in 2008 ;P.

------
jfb
Speaking as someone with no Android or security background, this was an
exceedingly well-written article. Big thumbs-up to saurik.

------
cremnob
Open always wins

------
MrKurtz
The folks at Android Police posted about it weeks ago:
[http://www.androidpolice.com/2013/07/11/second-all-access-
ap...](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/)

It also was already patched. Good write up though.

~~~
britta
saurik explains in his introduction why his article is different (and
potentially worth reading even if you've read other articles):

> Later articles were written about this bug, each examining the same exploit
> technique, coming to the conclusion that this vulnerability offered less
> abilities than the Master Key exploit, due to very narrow and unlikely
> requirements that the original signed application package must satisfy.

> In this article, I document different exploit techniques for this bug that
> do not have these limits; in fact, the second exploit described here
> provides much more capability than the Master Key exploit, allowing
> arbitrarily complex packages to take on the signatures from arbitrary signed
> packages. Finally, I look at the history of this bug in Android, showing
> when it was introduced.

~~~
saurik
Correct. In fact, in my section "Serious Limitations", I directly reference
the article from Android Police and quote from its at-the-time understanding
of the vulnerability.

