Bug 959652: Use NativeCrypto.sha1. r=rnewman
☠☠ backed out by 31f9a148b33c ☠ ☠
authorMichael Comella <michael.l.comella@gmail.com>
Tue, 11 Mar 2014 19:43:20 -0700
changeset 191365 9fe5610363b3f8e35f248bb2ce726c8e2665a8a1
parent 191364 c75d5601b1547ef8ce7f39d8ed566f6c9eed495b
child 191366 bd0463063293e9e2fdfcea2dce80bc849316af0f
push id474
push userasasaki@mozilla.com
push dateMon, 02 Jun 2014 21:01:02 +0000
treeherdermozilla-release@967f4cf1b31c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrnewman
bugs959652
milestone30.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 959652: Use NativeCrypto.sha1. r=rnewman
mobile/android/base/background/healthreport/EnvironmentV1.java
mobile/android/tests/background/junit3/android-services-files.mk
mobile/android/tests/background/junit3/src/healthreport/TestEnvironmentV1HashAppender.java
--- a/mobile/android/base/background/healthreport/EnvironmentV1.java
+++ b/mobile/android/base/background/healthreport/EnvironmentV1.java
@@ -1,24 +1,24 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 package org.mozilla.gecko.background.healthreport;
 
 import java.io.UnsupportedEncodingException;
-import java.security.MessageDigest;
 import java.security.NoSuchAlgorithmException;
 import java.util.Iterator;
 import java.util.SortedSet;
 
 import org.json.JSONException;
 import org.json.JSONObject;
 import org.mozilla.apache.commons.codec.binary.Base64;
 import org.mozilla.gecko.background.common.log.Logger;
+import org.mozilla.gecko.background.nativecode.NativeCrypto;
 
 public abstract class EnvironmentV1 {
   private static final String LOG_TAG = "GeckoEnvironment";
   private static final int VERSION = 1;
 
   protected final Class<? extends EnvironmentAppender> appenderClass;
 
   protected volatile String hash = null;
@@ -65,46 +65,51 @@ public abstract class EnvironmentV1 {
    * own appender that just records strings, for example.
    */
   public static abstract class EnvironmentAppender {
     public abstract void append(String s);
     public abstract void append(int v);
   }
 
   public static class HashAppender extends EnvironmentAppender {
-    final MessageDigest hasher;
+    private final StringBuilder builder;
 
     public HashAppender() throws NoSuchAlgorithmException {
-      // Note to the security-minded reader: we deliberately use SHA-1 here, not
-      // a stronger hash. These identifiers don't strictly need a cryptographic
-      // hash function, because there is negligible value in attacking the hash.
-      // We use SHA-1 because it's *shorter* -- the exact same reason that Git
-      // chose SHA-1.
-      hasher = MessageDigest.getInstance("SHA-1");
+      builder = new StringBuilder();
     }
 
     @Override
     public void append(String s) {
-      try {
-        hasher.update(((s == null) ? "null" : s).getBytes("UTF-8"));
-      } catch (UnsupportedEncodingException e) {
-        // This can never occur. Thanks, Java.
-      }
+      builder.append((s == null) ? "null" : s);
     }
 
     @Override
     public void append(int profileCreation) {
       append(Integer.toString(profileCreation, 10));
     }
 
     @Override
     public String toString() {
       // We *could* use ASCII85… but the savings would be negated by the
       // inclusion of JSON-unsafe characters like double-quote.
-      return new Base64(-1, null, false).encodeAsString(hasher.digest());
+      final byte[] inputBytes;
+      try {
+        inputBytes = builder.toString().getBytes("UTF-8");
+      } catch (UnsupportedEncodingException e) {
+        Logger.warn(LOG_TAG, "Invalid charset String passed to getBytes", e);
+        return null;
+      }
+
+      // Note to the security-minded reader: we deliberately use SHA-1 here, not
+      // a stronger hash. These identifiers don't strictly need a cryptographic
+      // hash function, because there is negligible value in attacking the hash.
+      // We use SHA-1 because it's *shorter* -- the exact same reason that Git
+      // chose SHA-1.
+      final byte[] hash = NativeCrypto.sha1(inputBytes);
+      return new Base64(-1, null, false).encodeAsString(hash);
     }
   }
 
   /**
    * Ensure that the {@link Environment} has been registered with its
    * storage layer, and can be used to annotate events.
    *
    * It's safe to call this method more than once, and each time you'll
--- a/mobile/android/tests/background/junit3/android-services-files.mk
+++ b/mobile/android/tests/background/junit3/android-services-files.mk
@@ -25,16 +25,17 @@ BACKGROUND_TESTS_JAVA_FILES := \
   src/fxa/TestBrowserIDKeyPairGeneration.java \
   src/healthreport/MockDatabaseEnvironment.java \
   src/healthreport/MockHealthReportDatabaseStorage.java \
   src/healthreport/MockHealthReportSQLiteOpenHelper.java \
   src/healthreport/MockProfileInformationCache.java \
   src/healthreport/prune/TestHealthReportPruneService.java \
   src/healthreport/prune/TestPrunePolicyDatabaseStorage.java \
   src/healthreport/TestEnvironmentBuilder.java \
+  src/healthreport/TestEnvironmentV1HashAppender.java \
   src/healthreport/TestHealthReportBroadcastService.java \
   src/healthreport/TestHealthReportDatabaseStorage.java \
   src/healthreport/TestHealthReportGenerator.java \
   src/healthreport/TestHealthReportProvider.java \
   src/healthreport/TestHealthReportSQLiteOpenHelper.java \
   src/healthreport/TestProfileInformationCache.java \
   src/healthreport/upload/TestAndroidSubmissionClient.java \
   src/healthreport/upload/TestHealthReportUploadService.java \
new file mode 100644
--- /dev/null
+++ b/mobile/android/tests/background/junit3/src/healthreport/TestEnvironmentV1HashAppender.java
@@ -0,0 +1,146 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+package org.mozilla.gecko.background.healthreport;
+
+import java.io.UnsupportedEncodingException;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
+import java.util.Arrays;
+import java.util.LinkedList;
+
+import org.mozilla.apache.commons.codec.binary.Base64;
+import org.mozilla.gecko.background.healthreport.EnvironmentV1.EnvironmentAppender;
+import org.mozilla.gecko.background.healthreport.EnvironmentV1.HashAppender;
+import org.mozilla.gecko.background.helpers.FakeProfileTestCase;
+import org.mozilla.gecko.sync.Utils;
+
+/**
+ * Tests the HashAppender functionality. Note that these tests must be run on an Android
+ * device because the SHA-1 native library needs to be loaded.
+ */
+public class TestEnvironmentV1HashAppender extends FakeProfileTestCase {
+  // input and expected values via: http://oauth.googlecode.com/svn/code/c/liboauth/src/sha1.c
+  private final static String[] INPUTS = new String[] {
+    "abc",
+    "abcdbcdecdefdefgefghfghighijhijkijkljklmklmnlmnomnopnopq",
+    "" // To be filled in below.
+  };
+  static {
+    final String baseStr = "01234567";
+    final int repetitions = 80;
+    final StringBuilder builder = new StringBuilder(baseStr.length() * repetitions);
+    for (int i = 0; i < 80; ++i) {
+      builder.append(baseStr);
+    }
+    INPUTS[2] = builder.toString();
+  }
+
+  private final static String[] EXPECTEDS = new String[] {
+    "a9993e364706816aba3e25717850c26c9cd0d89d",
+    "84983e441c3bd26ebaae4aa1f95129e5e54670f1",
+    "dea356a2cddd90c7a7ecedc5ebb563934f460452"
+  };
+  static {
+    for (int i = 0; i < EXPECTEDS.length; ++i) {
+      EXPECTEDS[i] = new Base64(-1, null, false).encodeAsString(Utils.hex2Byte(EXPECTEDS[i]));
+    }
+  }
+
+  public void testSHA1Hashing() throws Exception {
+    for (int i = 0; i < INPUTS.length; ++i) {
+      final String input = INPUTS[i];
+      final String expected = EXPECTEDS[i];
+
+      final HashAppender appender = new HashAppender();
+      addStringToAppenderInParts(appender, input);
+      final String result = appender.toString();
+
+      assertEquals(expected, result);
+    }
+  }
+
+  /**
+   * Tests to ensure output is the same as the former MessageDigest implementation (bug 959652).
+   */
+  public void testAgainstMessageDigestImpl() throws Exception {
+    // List.add doesn't allow add(null) so we make a LinkedList here.
+    final LinkedList<String> inputs = new LinkedList<String>(Arrays.asList(INPUTS));
+    inputs.add(null);
+
+    for (final String input : inputs) {
+      final HashAppender hAppender = new HashAppender();
+      final MessageDigestHashAppender mdAppender = new MessageDigestHashAppender();
+
+      hAppender.append(input);
+      mdAppender.append(input);
+
+      final String hResult = hAppender.toString();
+      final String mdResult = mdAppender.toString();
+      assertEquals(mdResult, hResult);
+    }
+  }
+
+  public void testIntegersAgainstMessageDigestImpl() throws Exception {
+    final int[] INPUTS = {Integer.MIN_VALUE, -1337, -42, 0, 42, 1337, Integer.MAX_VALUE};
+    for (final int input : INPUTS) {
+      final HashAppender hAppender = new HashAppender();
+      final MessageDigestHashAppender mdAppender = new MessageDigestHashAppender();
+
+      hAppender.append(input);
+      mdAppender.append(input);
+
+      final String hResult = hAppender.toString();
+      final String mdResult = mdAppender.toString();
+      assertEquals(mdResult, hResult);
+    }
+  }
+
+  private void addStringToAppenderInParts(final EnvironmentAppender appender, final String input) {
+    int substrInd = 0;
+    int substrLength = 1;
+    while (substrInd < input.length()) {
+      final int endInd = Math.min(substrInd + substrLength, input.length());
+
+      appender.append(input.substring(substrInd, endInd));
+
+      substrInd = endInd;
+      ++substrLength;
+    }
+  }
+
+  // --- COPY-PASTA'D CODE, FOR TESTING PURPOSES. ---
+  public static class MessageDigestHashAppender extends EnvironmentAppender {
+    final MessageDigest hasher;
+
+    public MessageDigestHashAppender() throws NoSuchAlgorithmException {
+      // Note to the security-minded reader: we deliberately use SHA-1 here, not
+      // a stronger hash. These identifiers don't strictly need a cryptographic
+      // hash function, because there is negligible value in attacking the hash.
+      // We use SHA-1 because it's *shorter* -- the exact same reason that Git
+      // chose SHA-1.
+      hasher = MessageDigest.getInstance("SHA-1");
+    }
+
+    @Override
+    public void append(String s) {
+      try {
+        hasher.update(((s == null) ? "null" : s).getBytes("UTF-8"));
+      } catch (UnsupportedEncodingException e) {
+        // This can never occur. Thanks, Java.
+      }
+    }
+
+    @Override
+    public void append(int profileCreation) {
+      append(Integer.toString(profileCreation, 10));
+    }
+
+    @Override
+    public String toString() {
+      // We *could* use ASCII85… but the savings would be negated by the
+      // inclusion of JSON-unsafe characters like double-quote.
+      return new Base64(-1, null, false).encodeAsString(hasher.digest());
+    }
+  }
+}