7 ms·

Android Bug Superior to Master Key

jfb 13y ago
Speaking as someone with no Android or security background, this was an exceedingly well-written article. Big thumbs-up to saurik.
kevingadd 13y ago
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 13y ago
  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.)
MrKurtz 13y ago
The folks at Android Police posted about it weeks ago: 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/ http://www.androidpolice.com/2013/07/11/second-all-access-ap... It also was already patched. Good write up though.
- britta 13y ago
  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 13y ago
    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.
- agent123 13y ago
  A commit to a repository does not equal a patch to the half billion vulnerable devices in the field.
  - brokenparser 13y ago
    Yes, captain obvious? What do you suggest?
    - agent123 13y ago
      Instead of minimizing or dismissing serious problems as 'already patched', hackers could help by warning consumers about the proliferation of Android malware. If sales were jeopardized, vendors would start caring about updates.
      - brokenparser 13y ago
cremnob 13y ago
Open always wins
MichaelGG 13y ago
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.
- ajross 13y ago
  I think the real problem with the logic is that it equates "reasoning about unsigned logic is hard" with "unsigned logic isn't required". Sadly it is. There are solutions that can only be expressed cleanly in unsigned logic, and others that can only be done signed. Now, clearly there are more problems out there that need a sign bit and comparison operators that honor it than there are problems that don't. So Java made the "right" choice about which one to eject. Still, hard code remains hard code, and there is no free lunch. Hard code is going to have bugs, and designing your language to make hard code harder (in the interest of making easy code easy) is probably a bad tradeoff in hindsight.
- chacham15 13y ago
  > "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.
  - mpyne 13y ago
    A growing concern IMO is that there are many situations in C where you can hit undefined behavior with signed arithmetic, where the behavior would have been perfectly defined (if undesirable) with unsigned logic.
Scaevolus 13y ago
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/designing-a-simple-and-secure-app-package-appx.aspx http://blogs.msdn.com/b/windowsappdev/archive/2012/12/04/des... If anyone knows how iOS .ipa packages are signed, I'd love to hear it.
- saurik 13y ago
  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.
  - deleted 13y ago
    [deleted]

jfb 13y ago

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

kevingadd 13y ago

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 13y ago
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.)

MrKurtz 13y ago

The folks at Android Police posted about it weeks ago: 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/ http://www.androidpolice.com/2013/07/11/second-all-access-ap... It also was already patched. Good write up though.

britta 13y ago
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 13y ago
  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.
agent123 13y ago
A commit to a repository does not equal a patch to the half billion vulnerable devices in the field.
- brokenparser 13y ago
  Yes, captain obvious? What do you suggest?
  - agent123 13y ago
    Instead of minimizing or dismissing serious problems as 'already patched', hackers could help by warning consumers about the proliferation of Android malware. If sales were jeopardized, vendors would start caring about updates.
    - brokenparser 13y ago

cremnob 13y ago

Open always wins

MichaelGG 13y ago

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.

ajross 13y ago
I think the real problem with the logic is that it equates "reasoning about unsigned logic is hard" with "unsigned logic isn't required". Sadly it is. There are solutions that can only be expressed cleanly in unsigned logic, and others that can only be done signed. Now, clearly there are more problems out there that need a sign bit and comparison operators that honor it than there are problems that don't. So Java made the "right" choice about which one to eject. Still, hard code remains hard code, and there is no free lunch. Hard code is going to have bugs, and designing your language to make hard code harder (in the interest of making easy code easy) is probably a bad tradeoff in hindsight.
chacham15 13y ago
> "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.
- mpyne 13y ago
  A growing concern IMO is that there are many situations in C where you can hit undefined behavior with signed arithmetic, where the behavior would have been perfectly defined (if undesirable) with unsigned logic.

Scaevolus 13y ago

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/designing-a-simple-and-secure-app-package-appx.aspx http://blogs.msdn.com/b/windowsappdev/archive/2012/12/04/des... If anyone knows how iOS .ipa packages are signed, I'd love to hear it.

saurik 13y ago
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.
- deleted 13y ago
  [deleted]