From b52fb903803c2dfca578c140dfdc776927c6fc4b Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Fri, 2 Jan 2015 01:57:49 +0100 Subject: fix and test for bad certificate version numbers (#1012) closes #1012 --- .../keychain/pgp/UncachedKeyringTest.java | 17 +++++++++++++++++ .../test/resources/test-keys/broken_cert_version.asc | 17 +++++++++++++++++ .../keychain/pgp/UncachedKeyRing.java | 20 +++++++++++++------- extern/spongycastle | 2 +- 4 files changed, 48 insertions(+), 8 deletions(-) create mode 100644 OpenKeychain-Test/src/test/resources/test-keys/broken_cert_version.asc diff --git a/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringTest.java b/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringTest.java index 34c637abf..89fefb767 100644 --- a/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringTest.java +++ b/OpenKeychain-Test/src/test/java/org/sufficientlysecure/keychain/pgp/UncachedKeyringTest.java @@ -34,6 +34,7 @@ import org.sufficientlysecure.keychain.service.SaveKeyringParcel.ChangeUnlockPar import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; +import java.io.IOException; import java.util.Iterator; @RunWith(RobolectricTestRunner.class) @@ -129,4 +130,20 @@ public class UncachedKeyringTest { pubRing.extractPublicKeyRing(); } + @Test(expected = IOException.class) + public void testBrokenVersionCert() throws Throwable { + // this is a test for one of the patches we use on top of stock bouncycastle, which + // returns an IOException rather than a RuntimeException in case of a bad certificate + // version byte + readRingFromResource("/test-keys/broken_cert_version.asc"); + } + + UncachedKeyRing readRingFromResource(String name) throws Throwable { + try { + return UncachedKeyRing.fromStream(UncachedKeyringTest.class.getResourceAsStream(name)).next(); + } catch (RuntimeException e) { + throw e.getCause(); + } + } + } diff --git a/OpenKeychain-Test/src/test/resources/test-keys/broken_cert_version.asc b/OpenKeychain-Test/src/test/resources/test-keys/broken_cert_version.asc new file mode 100644 index 000000000..e2d2abd8e --- /dev/null +++ b/OpenKeychain-Test/src/test/resources/test-keys/broken_cert_version.asc @@ -0,0 +1,17 @@ +-----BEGIN PGP PUBLIC KEY BLOCK----- + +mQENBFSl5DIBCADqsGJJ8RhV4Uw6a3Q48QyTMrUtvZquOTlLVaqOdEFZNL5/OBal +prft9LNkcOkIVA89Hdn715WwkmG2OJiJoQ/ZAKwal1CPGm4Q8kZIM7k57ISJL6J5 +300e7UIznc74XbG7eFNxNcjCM9wG12vW2rFwc+ogJtkBSf0IXukPwtUkRK+H5ufO +lpqS5NNZfiGbNQCrb+YsGZNRk4QTGR6WGyaIRHlcG8G00VPGNSauTqe/11MO9MoF +BvPgFeur3nefWunCQ+uDmzIEs8r94gaHu3LWbctd5w5x/o/PDfTSSiO+U8zzXrKC +4ZpEl5bk7t7jH1hYMLWyO6nn0vWTOMO1EYLBABEBAAG0GGJyb2tlbiBzaWduYXR1 +cmUgdmVyc2lvbokBOMATAQIAIgUCVKXkMgIbAwYLCQgHAwIGFQgCCQoLBBYCAwEC +HgECF4AACgkQDe00lH/2SnprLggAh64TsdHDfIhTNc1DeJLCuvuHsitAcUdEEnue +yJjodxboKNSplIwnmb5CpM3P8f736dNaW77Yd6aO4IeAy6cBlxT1tSRkJMsp+cBt +kBa3lRr+GnWZlLZs3coL2g0t5RbuyYKyQxm2qvgFJGi/7Qfty5nJOW5U1ElT3VT8 +jISNdQdDAIaBsCE+TuyW3VsP3PqnJ7x14K7VhkFuCyvYB9paLcJBnan93R0Ja0Ip +Cv1pbrNxXp0UELf0RYc2X5C1m6otZ9LKf3PmzxlEkApkb1TZUEBak2Za5p99koZT ++pg/XpZPyawi+gZeYkBAohxRGmzG/a4L+YacAZHbchfN0eG7lg== +=mxTR +-----END PGP PUBLIC KEY BLOCK----- diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java index c4cd0b3e5..006af6073 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/pgp/UncachedKeyRing.java @@ -143,17 +143,22 @@ public class UncachedKeyRing { throw new PgpGeneralException("Object not recognized as PGPKeyRing!"); } - UncachedKeyRing ring = parsed.next(); + try { + UncachedKeyRing ring = parsed.next(); - if (parsed.hasNext()) { - throw new PgpGeneralException("Expected single keyring in stream, found at least two"); - } + if (parsed.hasNext()) { + throw new PgpGeneralException("Expected single keyring in stream, found at least two"); + } - return ring; + return ring; + } catch (RuntimeException e) { + // yes this is bad style. we should rework this in a better way + throw new PgpGeneralException(e.getCause()); + } } - public static Iterator fromStream(final InputStream stream) throws IOException { + public static Iterator fromStream(final InputStream stream) { return new Iterator() { @@ -190,7 +195,8 @@ public class UncachedKeyRing { mObjectFactory = null; } } catch (IOException e) { - Log.e(Constants.TAG, "IOException while processing stream. ArmoredInputStream CRC check failed?", e); + throw new RuntimeException(e); + // Log.e(Constants.TAG, "IOException while processing stream. ArmoredInputStream CRC check failed?", e); } catch (ArrayIndexOutOfBoundsException e) { Log.e(Constants.TAG, "ArmoredInputStream decode failed, symbol is not in decodingTable!", e); } diff --git a/extern/spongycastle b/extern/spongycastle index 375084d55..1d9ee197d 160000 --- a/extern/spongycastle +++ b/extern/spongycastle @@ -1 +1 @@ -Subproject commit 375084d55341b575274e49d9a69fa4cf9356682a +Subproject commit 1d9ee197d8fcc18dcdd1ae9649a5dc53e910b18c -- cgit v1.2.3