From 80c7cb52adfe553da3f1a3390373ec7092274658 Mon Sep 17 00:00:00 2001 From: Elliotte Rusty Harold Date: Wed, 7 Aug 2019 06:46:05 -0400 Subject: [PATCH 1/3] stop exposing internal private state --- .../json/webtoken/JsonWebSignature.java | 9 +++--- .../json/webtoken/JsonWebSignatureTest.java | 28 +++++++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/google-http-client/src/main/java/com/google/api/client/json/webtoken/JsonWebSignature.java b/google-http-client/src/main/java/com/google/api/client/json/webtoken/JsonWebSignature.java index 0777c8b92..50da3b4ae 100644 --- a/google-http-client/src/main/java/com/google/api/client/json/webtoken/JsonWebSignature.java +++ b/google-http-client/src/main/java/com/google/api/client/json/webtoken/JsonWebSignature.java @@ -32,6 +32,7 @@ import java.security.Signature; import java.security.cert.X509Certificate; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import javax.net.ssl.TrustManager; import javax.net.ssl.TrustManagerFactory; @@ -471,14 +472,14 @@ private static X509TrustManager getDefaultX509TrustManager() { } } - /** Returns the modifiable array of bytes of the signature. */ + /** Returns the bytes of the signature. */ public final byte[] getSignatureBytes() { - return signatureBytes; + return Arrays.copyOf(signatureBytes, signatureBytes.length); } - /** Returns the modifiable array of bytes of the signature content. */ + /** Returns the bytes of the signature content. */ public final byte[] getSignedContentBytes() { - return signedContentBytes; + return Arrays.copyOf(signedContentBytes, signedContentBytes.length); } /** diff --git a/google-http-client/src/test/java/com/google/api/client/json/webtoken/JsonWebSignatureTest.java b/google-http-client/src/test/java/com/google/api/client/json/webtoken/JsonWebSignatureTest.java index 312c1d971..bfecc68e6 100644 --- a/google-http-client/src/test/java/com/google/api/client/json/webtoken/JsonWebSignatureTest.java +++ b/google-http-client/src/test/java/com/google/api/client/json/webtoken/JsonWebSignatureTest.java @@ -17,9 +17,15 @@ import com.google.api.client.testing.json.MockJsonFactory; import com.google.api.client.testing.json.webtoken.TestCertificates; import com.google.api.client.testing.util.SecurityTestUtils; + +import java.io.IOException; import java.security.cert.X509Certificate; import java.security.interfaces.RSAPrivateKey; import javax.net.ssl.X509TrustManager; + +import org.junit.Assert; +import org.junit.Test; + import junit.framework.TestCase; /** @@ -29,6 +35,7 @@ */ public class JsonWebSignatureTest extends TestCase { + @Test public void testSign() throws Exception { JsonWebSignature.Header header = new JsonWebSignature.Header(); header.setAlgorithm("RS256"); @@ -51,13 +58,34 @@ private X509Certificate verifyX509WithCaCert(TestCertificates.CertData caCert) t X509TrustManager trustManager = caCert.getTrustManager(); return signature.verifySignature(trustManager); } + + @Test + public void testImmutableSignatureBytes() throws IOException { + JsonWebSignature signature = TestCertificates.getJsonWebSignature(); + byte[] bytes = signature.getSignatureBytes(); + bytes[0] = (byte) (bytes[0] + 1); + byte[] bytes2 = signature.getSignatureBytes(); + Assert.assertNotEquals(bytes2[0], bytes[0]); + } + + @Test + public void testImmutableSignedContentBytes() throws IOException { + JsonWebSignature signature = TestCertificates.getJsonWebSignature(); + byte[] bytes = signature.getSignedContentBytes(); + bytes[0] = (byte) (bytes[0] + 1); + byte[] bytes2 = signature.getSignedContentBytes(); + Assert.assertNotEquals(bytes2[0], bytes[0]); + } + + @Test public void testVerifyX509() throws Exception { X509Certificate signatureCert = verifyX509WithCaCert(TestCertificates.CA_CERT); assertNotNull(signatureCert); assertTrue(signatureCert.getSubjectDN().getName().startsWith("CN=foo.bar.com")); } + @Test public void testVerifyX509WrongCa() throws Exception { assertNull(verifyX509WithCaCert(TestCertificates.BOGUS_CA_CERT)); } From c31121af5d1155d781fc66bc3d9d057ae0f9a4af Mon Sep 17 00:00:00 2001 From: Elliotte Rusty Harold Date: Wed, 7 Aug 2019 06:49:23 -0400 Subject: [PATCH 2/3] JUnit 4 --- .../json/webtoken/JsonWebSignatureTest.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/google-http-client/src/test/java/com/google/api/client/json/webtoken/JsonWebSignatureTest.java b/google-http-client/src/test/java/com/google/api/client/json/webtoken/JsonWebSignatureTest.java index bfecc68e6..1e4126c16 100644 --- a/google-http-client/src/test/java/com/google/api/client/json/webtoken/JsonWebSignatureTest.java +++ b/google-http-client/src/test/java/com/google/api/client/json/webtoken/JsonWebSignatureTest.java @@ -19,6 +19,7 @@ import com.google.api.client.testing.util.SecurityTestUtils; import java.io.IOException; +import java.security.GeneralSecurityException; import java.security.cert.X509Certificate; import java.security.interfaces.RSAPrivateKey; import javax.net.ssl.X509TrustManager; @@ -26,14 +27,12 @@ import org.junit.Assert; import org.junit.Test; -import junit.framework.TestCase; - /** * Tests {@link JsonWebSignature}. * * @author Yaniv Inbar */ -public class JsonWebSignatureTest extends TestCase { +public class JsonWebSignatureTest { @Test public void testSign() throws Exception { @@ -47,13 +46,14 @@ public void testSign() throws Exception { .setIssuedAtTimeSeconds(0L) .setExpirationTimeSeconds(3600L); RSAPrivateKey privateKey = SecurityTestUtils.newRsaPrivateKey(); - assertEquals( + Assert.assertEquals( "..kDmKaHNYByLmqAi9ROeLcFmZM7W_emsceKvDZiEGAo-ineCunC6_Nb0HEpAuzIidV-LYTMHS3BvI49KFz9gi6hI3" + "ZndDL5EzplpFJo1ZclVk1_hLn94P2OTAkZ4ydsTfus6Bl98EbCkInpF_2t5Fr8OaHxCZCDdDU7W5DSnOsx4", JsonWebSignature.signUsingRsaSha256(privateKey, new MockJsonFactory(), header, payload)); } - private X509Certificate verifyX509WithCaCert(TestCertificates.CertData caCert) throws Exception { + private X509Certificate verifyX509WithCaCert(TestCertificates.CertData caCert) + throws IOException, GeneralSecurityException { JsonWebSignature signature = TestCertificates.getJsonWebSignature(); X509TrustManager trustManager = caCert.getTrustManager(); return signature.verifySignature(trustManager); @@ -81,12 +81,12 @@ public void testImmutableSignedContentBytes() throws IOException { @Test public void testVerifyX509() throws Exception { X509Certificate signatureCert = verifyX509WithCaCert(TestCertificates.CA_CERT); - assertNotNull(signatureCert); - assertTrue(signatureCert.getSubjectDN().getName().startsWith("CN=foo.bar.com")); + Assert.assertNotNull(signatureCert); + Assert.assertTrue(signatureCert.getSubjectDN().getName().startsWith("CN=foo.bar.com")); } @Test public void testVerifyX509WrongCa() throws Exception { - assertNull(verifyX509WithCaCert(TestCertificates.BOGUS_CA_CERT)); + Assert.assertNull(verifyX509WithCaCert(TestCertificates.BOGUS_CA_CERT)); } } From edfe4e6b954d15199f5b6a1eca4411d6273126e9 Mon Sep 17 00:00:00 2001 From: Elliotte Rusty Harold Date: Wed, 7 Aug 2019 07:08:25 -0400 Subject: [PATCH 3/3] don't expose internal lists --- .../json/webtoken/JsonWebSignature.java | 17 +++++++------ .../json/webtoken/JsonWebSignatureTest.java | 25 +++++++++++++++++++ 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/google-http-client/src/main/java/com/google/api/client/json/webtoken/JsonWebSignature.java b/google-http-client/src/main/java/com/google/api/client/json/webtoken/JsonWebSignature.java index 50da3b4ae..727e93569 100644 --- a/google-http-client/src/main/java/com/google/api/client/json/webtoken/JsonWebSignature.java +++ b/google-http-client/src/main/java/com/google/api/client/json/webtoken/JsonWebSignature.java @@ -136,7 +136,7 @@ public static class Header extends JsonWebToken.Header { * @since 1.19.1. */ @Key("x5c") - private List x509Certificates; + private ArrayList x509Certificates; /** * Array listing the header parameter names that define extensions that are used in the JWS @@ -300,7 +300,7 @@ public final String getX509Certificate() { * @since 1.19.1. */ public final List getX509Certificates() { - return x509Certificates; + return new ArrayList<>(x509Certificates); } /** @@ -332,22 +332,25 @@ public Header setX509Certificate(String x509Certificate) { * @since 1.19.1. */ public Header setX509Certificates(List x509Certificates) { - this.x509Certificates = x509Certificates; + this.x509Certificates = new ArrayList<>(x509Certificates); return this; } /** - * Returns the array listing the header parameter names that define extensions that are used in + * Returns an array listing the header parameter names that define extensions used in * the JWS header that MUST be understood and processed or {@code null} for none. * * @since 1.16 */ public final List getCritical() { - return critical; + if (critical == null || critical.isEmpty()) { + return null; + } + return new ArrayList<>(critical); } /** - * Sets the array listing the header parameter names that define extensions that are used in the + * Sets the header parameter names that define extensions used in the * JWS header that MUST be understood and processed or {@code null} for none. * *

Overriding is only supported for the purpose of calling the super implementation and @@ -356,7 +359,7 @@ public final List getCritical() { * @since 1.16 */ public Header setCritical(List critical) { - this.critical = critical; + this.critical = new ArrayList<>(critical); return this; } diff --git a/google-http-client/src/test/java/com/google/api/client/json/webtoken/JsonWebSignatureTest.java b/google-http-client/src/test/java/com/google/api/client/json/webtoken/JsonWebSignatureTest.java index 1e4126c16..8bff77f93 100644 --- a/google-http-client/src/test/java/com/google/api/client/json/webtoken/JsonWebSignatureTest.java +++ b/google-http-client/src/test/java/com/google/api/client/json/webtoken/JsonWebSignatureTest.java @@ -22,6 +22,9 @@ import java.security.GeneralSecurityException; import java.security.cert.X509Certificate; import java.security.interfaces.RSAPrivateKey; +import java.util.ArrayList; +import java.util.List; + import javax.net.ssl.X509TrustManager; import org.junit.Assert; @@ -76,7 +79,29 @@ public void testImmutableSignedContentBytes() throws IOException { byte[] bytes2 = signature.getSignedContentBytes(); Assert.assertNotEquals(bytes2[0], bytes[0]); } + + @Test + public void testImmutableCertificates() throws IOException { + JsonWebSignature signature = TestCertificates.getJsonWebSignature(); + List certificates = signature.getHeader().getX509Certificates(); + certificates.set(0, "foo"); + Assert.assertNotEquals("foo", signature.getHeader().getX509Certificates().get(0)); + } + + @Test + public void testImmutableCritical() throws IOException { + JsonWebSignature signature = TestCertificates.getJsonWebSignature(); + List critical = new ArrayList<>(); + signature.getHeader().setCritical(critical); + critical.add("bar"); + Assert.assertNull(signature.getHeader().getCritical()); + } + @Test + public void testCriticalNullForNone() throws IOException { + JsonWebSignature signature = TestCertificates.getJsonWebSignature(); + Assert.assertNull(signature.getHeader().getCritical()); + } @Test public void testVerifyX509() throws Exception {