From 1b4edff5d8ae2ceb64d59ca71b9d5dd64dd7c9a2 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Fri, 1 Nov 2019 15:45:44 -0400 Subject: [PATCH 1/2] Reject rounds=0 for SHA1 hashes Port of https://github.com/firebase/firebase-admin-node/pull/677 --- .../com/google/firebase/auth/hash/Md5.java | 2 +- .../com/google/firebase/auth/hash/Sha1.java | 2 +- .../com/google/firebase/auth/hash/Sha256.java | 2 +- .../com/google/firebase/auth/hash/Sha512.java | 2 +- .../firebase/auth/UserImportHashTest.java | 86 +++++++++++++++++++ 5 files changed, 90 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/google/firebase/auth/hash/Md5.java b/src/main/java/com/google/firebase/auth/hash/Md5.java index f45362ae3..2abbe55ba 100644 --- a/src/main/java/com/google/firebase/auth/hash/Md5.java +++ b/src/main/java/com/google/firebase/auth/hash/Md5.java @@ -23,7 +23,7 @@ public class Md5 extends RepeatableHash { private Md5(Builder builder) { - super("MD5", 0, 120000, builder); + super("MD5", 0, 8192, builder); } public static Builder builder() { diff --git a/src/main/java/com/google/firebase/auth/hash/Sha1.java b/src/main/java/com/google/firebase/auth/hash/Sha1.java index d14975fda..385f4310c 100644 --- a/src/main/java/com/google/firebase/auth/hash/Sha1.java +++ b/src/main/java/com/google/firebase/auth/hash/Sha1.java @@ -23,7 +23,7 @@ public class Sha1 extends RepeatableHash { private Sha1(Builder builder) { - super("SHA1", 0, 120000, builder); + super("SHA1", 1, 8192, builder); } public static Builder builder() { diff --git a/src/main/java/com/google/firebase/auth/hash/Sha256.java b/src/main/java/com/google/firebase/auth/hash/Sha256.java index ecc0e7280..f65aee19a 100644 --- a/src/main/java/com/google/firebase/auth/hash/Sha256.java +++ b/src/main/java/com/google/firebase/auth/hash/Sha256.java @@ -23,7 +23,7 @@ public class Sha256 extends RepeatableHash { private Sha256(Builder builder) { - super("SHA256", 0, 120000, builder); + super("SHA256", 1, 8192, builder); } public static Builder builder() { diff --git a/src/main/java/com/google/firebase/auth/hash/Sha512.java b/src/main/java/com/google/firebase/auth/hash/Sha512.java index 858d16e05..e582520a9 100644 --- a/src/main/java/com/google/firebase/auth/hash/Sha512.java +++ b/src/main/java/com/google/firebase/auth/hash/Sha512.java @@ -23,7 +23,7 @@ public class Sha512 extends RepeatableHash { private Sha512(Builder builder) { - super("SHA512", 0, 120000, builder); + super("SHA512", 1, 8192, builder); } public static Builder builder() { diff --git a/src/test/java/com/google/firebase/auth/UserImportHashTest.java b/src/test/java/com/google/firebase/auth/UserImportHashTest.java index e4a3af14a..67444c3a0 100644 --- a/src/test/java/com/google/firebase/auth/UserImportHashTest.java +++ b/src/test/java/com/google/firebase/auth/UserImportHashTest.java @@ -17,6 +17,7 @@ package com.google.firebase.auth; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; import com.google.common.collect.ImmutableMap; import com.google.common.io.BaseEncoding; @@ -145,6 +146,91 @@ public void testBasicHash() { } } + private void assertBuilderThrowsIllegalArgumentException(Md5.Builder builder) { + try { + builder.build(); + fail("Expected IllegalArgumentException to be thrown but no expecption occurred."); + } catch (IllegalArgumentException expected) { + } + } + + private void assertBuilderThrowsIllegalArgumentException(Sha1.Builder builder) { + try { + builder.build(); + fail("Expected IllegalArgumentException to be thrown but no expecption occurred."); + } catch (IllegalArgumentException expected) { + } + } + + private void assertBuilderThrowsIllegalArgumentException(Sha256.Builder builder) { + try { + builder.build(); + fail("Expected IllegalArgumentException to be thrown but no expecption occurred."); + } catch (IllegalArgumentException expected) { + } + } + + private void assertBuilderThrowsIllegalArgumentException(Sha512.Builder builder) { + try { + builder.build(); + fail("Expected IllegalArgumentException to be thrown but no expecption occurred."); + } catch (IllegalArgumentException expected) { + } + } + + private void assertBuilderThrowsIllegalArgumentException(PbkdfSha1.Builder builder) { + try { + builder.build(); + fail("Expected IllegalArgumentException to be thrown but no expecption occurred."); + } catch (IllegalArgumentException expected) { + } + } + + private void assertBuilderThrowsIllegalArgumentException(Pbkdf2Sha256.Builder builder) { + try { + builder.build(); + fail("Expected IllegalArgumentException to be thrown but no expecption occurred."); + } catch (IllegalArgumentException expected) { + } + } + + @Test + public void testInvalidHashRounds() { + // TODO(rsgowman): Once we can update to Java8, we could just do something like this instead of + // having all of the helpers: + // assertThrows(IllegalArgumentException.class, ()-> Md5.builder().setRounds(-1).build()); + // + + assertBuilderThrowsIllegalArgumentException(Md5.builder().setRounds(-1)); + assertBuilderThrowsIllegalArgumentException(Md5.builder().setRounds(8193)); + assertBuilderThrowsIllegalArgumentException(Sha1.builder().setRounds(0)); + assertBuilderThrowsIllegalArgumentException(Sha1.builder().setRounds(8193)); + assertBuilderThrowsIllegalArgumentException(Sha256.builder().setRounds(0)); + assertBuilderThrowsIllegalArgumentException(Sha256.builder().setRounds(8193)); + assertBuilderThrowsIllegalArgumentException(Sha512.builder().setRounds(0)); + assertBuilderThrowsIllegalArgumentException(Sha512.builder().setRounds(8193)); + assertBuilderThrowsIllegalArgumentException(PbkdfSha1.builder().setRounds(-1)); + assertBuilderThrowsIllegalArgumentException(PbkdfSha1.builder().setRounds(120001)); + assertBuilderThrowsIllegalArgumentException(Pbkdf2Sha256.builder().setRounds(-1)); + assertBuilderThrowsIllegalArgumentException(Pbkdf2Sha256.builder().setRounds(120001)); + } + + @Test + public void testValidHashRounds() { + Md5.builder().setRounds(0).build(); + Md5.builder().setRounds(8192).build(); + Sha1.builder().setRounds(1).build(); + Sha1.builder().setRounds(8192).build(); + Sha256.builder().setRounds(1).build(); + Sha256.builder().setRounds(8192).build(); + Sha512.builder().setRounds(1).build(); + Sha512.builder().setRounds(8192).build(); + PbkdfSha1.builder().setRounds(0).build(); + PbkdfSha1.builder().setRounds(120000).build(); + Pbkdf2Sha256.builder().setRounds(0).build(); + Pbkdf2Sha256.builder().setRounds(120000).build(); + } + @Test public void testBcryptHash() { UserImportHash bcrypt = Bcrypt.getInstance(); From 7f9ffcf1d136d5f7134fc3522ae3784d8e4541e9 Mon Sep 17 00:00:00 2001 From: Rich Gowman Date: Fri, 1 Nov 2019 16:23:51 -0400 Subject: [PATCH 2/2] Move test to more sensible location --- .../firebase/auth/UserImportHashTest.java | 86 ------------------- .../firebase/auth/hash/InvalidHashTest.java | 34 ++++++-- 2 files changed, 27 insertions(+), 93 deletions(-) diff --git a/src/test/java/com/google/firebase/auth/UserImportHashTest.java b/src/test/java/com/google/firebase/auth/UserImportHashTest.java index 67444c3a0..e4a3af14a 100644 --- a/src/test/java/com/google/firebase/auth/UserImportHashTest.java +++ b/src/test/java/com/google/firebase/auth/UserImportHashTest.java @@ -17,7 +17,6 @@ package com.google.firebase.auth; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.fail; import com.google.common.collect.ImmutableMap; import com.google.common.io.BaseEncoding; @@ -146,91 +145,6 @@ public void testBasicHash() { } } - private void assertBuilderThrowsIllegalArgumentException(Md5.Builder builder) { - try { - builder.build(); - fail("Expected IllegalArgumentException to be thrown but no expecption occurred."); - } catch (IllegalArgumentException expected) { - } - } - - private void assertBuilderThrowsIllegalArgumentException(Sha1.Builder builder) { - try { - builder.build(); - fail("Expected IllegalArgumentException to be thrown but no expecption occurred."); - } catch (IllegalArgumentException expected) { - } - } - - private void assertBuilderThrowsIllegalArgumentException(Sha256.Builder builder) { - try { - builder.build(); - fail("Expected IllegalArgumentException to be thrown but no expecption occurred."); - } catch (IllegalArgumentException expected) { - } - } - - private void assertBuilderThrowsIllegalArgumentException(Sha512.Builder builder) { - try { - builder.build(); - fail("Expected IllegalArgumentException to be thrown but no expecption occurred."); - } catch (IllegalArgumentException expected) { - } - } - - private void assertBuilderThrowsIllegalArgumentException(PbkdfSha1.Builder builder) { - try { - builder.build(); - fail("Expected IllegalArgumentException to be thrown but no expecption occurred."); - } catch (IllegalArgumentException expected) { - } - } - - private void assertBuilderThrowsIllegalArgumentException(Pbkdf2Sha256.Builder builder) { - try { - builder.build(); - fail("Expected IllegalArgumentException to be thrown but no expecption occurred."); - } catch (IllegalArgumentException expected) { - } - } - - @Test - public void testInvalidHashRounds() { - // TODO(rsgowman): Once we can update to Java8, we could just do something like this instead of - // having all of the helpers: - // assertThrows(IllegalArgumentException.class, ()-> Md5.builder().setRounds(-1).build()); - // - - assertBuilderThrowsIllegalArgumentException(Md5.builder().setRounds(-1)); - assertBuilderThrowsIllegalArgumentException(Md5.builder().setRounds(8193)); - assertBuilderThrowsIllegalArgumentException(Sha1.builder().setRounds(0)); - assertBuilderThrowsIllegalArgumentException(Sha1.builder().setRounds(8193)); - assertBuilderThrowsIllegalArgumentException(Sha256.builder().setRounds(0)); - assertBuilderThrowsIllegalArgumentException(Sha256.builder().setRounds(8193)); - assertBuilderThrowsIllegalArgumentException(Sha512.builder().setRounds(0)); - assertBuilderThrowsIllegalArgumentException(Sha512.builder().setRounds(8193)); - assertBuilderThrowsIllegalArgumentException(PbkdfSha1.builder().setRounds(-1)); - assertBuilderThrowsIllegalArgumentException(PbkdfSha1.builder().setRounds(120001)); - assertBuilderThrowsIllegalArgumentException(Pbkdf2Sha256.builder().setRounds(-1)); - assertBuilderThrowsIllegalArgumentException(Pbkdf2Sha256.builder().setRounds(120001)); - } - - @Test - public void testValidHashRounds() { - Md5.builder().setRounds(0).build(); - Md5.builder().setRounds(8192).build(); - Sha1.builder().setRounds(1).build(); - Sha1.builder().setRounds(8192).build(); - Sha256.builder().setRounds(1).build(); - Sha256.builder().setRounds(8192).build(); - Sha512.builder().setRounds(1).build(); - Sha512.builder().setRounds(8192).build(); - PbkdfSha1.builder().setRounds(0).build(); - PbkdfSha1.builder().setRounds(120000).build(); - Pbkdf2Sha256.builder().setRounds(0).build(); - Pbkdf2Sha256.builder().setRounds(120000).build(); - } - @Test public void testBcryptHash() { UserImportHash bcrypt = Bcrypt.getInstance(); diff --git a/src/test/java/com/google/firebase/auth/hash/InvalidHashTest.java b/src/test/java/com/google/firebase/auth/hash/InvalidHashTest.java index aabe4444c..5186a443a 100644 --- a/src/test/java/com/google/firebase/auth/hash/InvalidHashTest.java +++ b/src/test/java/com/google/firebase/auth/hash/InvalidHashTest.java @@ -48,17 +48,21 @@ public void testInvalidHmac() { @Test public void testInvalidRepeatableHash() { + // TODO(rsgowman): Once we can update to Java8, we could just do something like this instead of + // having all of the helpers: + // assertThrows(IllegalArgumentException.class, ()-> Md5.builder().setRounds(-1).build()); + List builders = ImmutableList.builder() - .add(Sha512.builder().setRounds(-1)) - .add(Sha256.builder().setRounds(-1)) - .add(Sha1.builder().setRounds(-1)) + .add(Sha512.builder().setRounds(0)) + .add(Sha256.builder().setRounds(0)) + .add(Sha1.builder().setRounds(0)) .add(Md5.builder().setRounds(-1)) .add(Pbkdf2Sha256.builder().setRounds(-1)) .add(PbkdfSha1.builder().setRounds(-1)) - .add(Sha512.builder().setRounds(120001)) - .add(Sha256.builder().setRounds(120001)) - .add(Sha1.builder().setRounds(120001)) - .add(Md5.builder().setRounds(120001)) + .add(Sha512.builder().setRounds(8193)) + .add(Sha256.builder().setRounds(8193)) + .add(Sha1.builder().setRounds(8193)) + .add(Md5.builder().setRounds(8193)) .add(Pbkdf2Sha256.builder().setRounds(120001)) .add(PbkdfSha1.builder().setRounds(120001)) .build(); @@ -72,6 +76,22 @@ public void testInvalidRepeatableHash() { } } + @Test + public void testValidRepeatableHash() { + Md5.builder().setRounds(0).build(); + Md5.builder().setRounds(8192).build(); + Sha1.builder().setRounds(1).build(); + Sha1.builder().setRounds(8192).build(); + Sha256.builder().setRounds(1).build(); + Sha256.builder().setRounds(8192).build(); + Sha512.builder().setRounds(1).build(); + Sha512.builder().setRounds(8192).build(); + PbkdfSha1.builder().setRounds(0).build(); + PbkdfSha1.builder().setRounds(120000).build(); + Pbkdf2Sha256.builder().setRounds(0).build(); + Pbkdf2Sha256.builder().setRounds(120000).build(); + } + @Test public void testInvalidScrypt() { List builders = ImmutableList.of(