Throw exception on odd length Signatures

The old version of this code would silently truncate odd-length
Signatures. However, this masks some bugs. Add a throw of
IllegalArgumentException so users can easily see where they're getting
bad input for Signatures.

Also, go through the existing code and catch this exception or
pre-check the input strings so system_server doesn't crash later.

Bug: 5092338
Change-Id: I8c672c5eaeb738a92c4581ce0df09baf719980ef

3 files changed, 35 insertions, 14 deletions

diff --git a/core/java/android/content/pm/Signature.java b/core/java/android/content/pm/Signature.java
index b32664eefc3c..c6aefb87c723 100644
--- a/core/java/android/content/pm/Signature.java
+++ b/core/java/android/content/pm/Signature.java
@@ -16,7 +16,6 @@
 
 package android.content.pm;
 
-import android.content.ComponentName;
 import android.os.Parcel;
 import android.os.Parcelable;
 
@@ -40,26 +39,41 @@ public class Signature implements Parcelable {
         mSignature = signature.clone();
     }
 
+    private static final int parseHexDigit(int nibble) {
+        if ('0' <= nibble && nibble <= '9') {
+            return nibble - '0';
+        } else if ('a' <= nibble && nibble <= 'f') {
+            return nibble - 'a' + 10;
+        } else if ('A' <= nibble && nibble <= 'F') {
+            return nibble - 'A' + 10;
+        } else {
+            throw new IllegalArgumentException("Invalid character " + nibble + " in hex string");
+        }
+    }
+
     /**
      * Create Signature from a text representation previously returned by
-     * {@link #toChars} or {@link #toCharsString()}.
+     * {@link #toChars} or {@link #toCharsString()}. Signatures are expected to
+     * be a hex-encoded ASCII string.
+     *
+     * @param text hex-encoded string representing the signature
+     * @throws IllegalArgumentException when signature is odd-length
      */
     public Signature(String text) {
         final byte[] input = text.getBytes();
         final int N = input.length;
+
+        if (N % 2 != 0) {
+            throw new IllegalArgumentException("text size " + N + " is not even");
+        }
+
         final byte[] sig = new byte[N / 2];
         int sigIndex = 0;
 
         for (int i = 0; i < N;) {
-            int b;
-
-            final int hi = input[i++];
-            b = (hi >= 'a' ? (hi - 'a' + 10) : (hi - '0')) << 4;
-
-            final int lo = input[i++];
-            b |= (lo >= 'a' ? (lo - 'a' + 10) : (lo - '0')) & 0x0F;
-
-            sig[sigIndex++] = (byte) (b & 0xFF);
+            final int hi = parseHexDigit(input[i++]);
+            final int lo = parseHexDigit(input[i++]);
+            sig[sigIndex++] = (byte) ((hi << 4) | lo);
         }
 
         mSignature = sig;
@@ -100,8 +114,7 @@ public class Signature implements Parcelable {
     }
 
     /**
-     * Return the result of {@link #toChars()} as a String.  This result is
-     * cached so future calls will return the same String.
+     * Return the result of {@link #toChars()} as a String.
      */
     public String toCharsString() {
         String str = mStringRef == null ? null : mStringRef.get();
@@ -127,7 +140,7 @@ public class Signature implements Parcelable {
         try {
             if (obj != null) {
                 Signature other = (Signature)obj;
-                return Arrays.equals(mSignature, other.mSignature);
+                return this == other || Arrays.equals(mSignature, other.mSignature);
             }
         } catch (ClassCastException e) {
         }
diff --git a/services/java/com/android/server/BackupManagerService.java b/services/java/com/android/server/BackupManagerService.java
index 2e3d6dd44937..b489e337ca9b 100644
--- a/services/java/com/android/server/BackupManagerService.java
+++ b/services/java/com/android/server/BackupManagerService.java
@@ -3298,6 +3298,8 @@ class BackupManagerService extends IBackupManager.Stub {
                 }
             } catch (NumberFormatException e) {
                 Slog.w(TAG, "Corrupt restore manifest for package " + info.packageName);
+            } catch (IllegalArgumentException e) {
+                Slog.w(TAG, e.getMessage());
             }
 
             return policy;
diff --git a/services/java/com/android/server/pm/PackageSignatures.java b/services/java/com/android/server/pm/PackageSignatures.java
index a25ec6cee508..9a20be7c8d76 100644
--- a/services/java/com/android/server/pm/PackageSignatures.java
+++ b/services/java/com/android/server/pm/PackageSignatures.java
@@ -138,6 +138,12 @@ class PackageSignatures {
                                     "Error in package manager settings: <cert> "
                                        + "index " + index + " is not a number at "
                                        + parser.getPositionDescription());
+                        } catch (IllegalArgumentException e) {
+                            PackageManagerService.reportSettingsProblem(Log.WARN,
+                                    "Error in package manager settings: <cert> "
+                                       + "index " + index + " has an invalid signature at "
+                                       + parser.getPositionDescription() + ": "
+                                       + e.getMessage());
                         }
                     } else {
                         PackageManagerService.reportSettingsProblem(Log.WARN,