From ca4e72ca5d9b9fdc03aa39e53ec3eca11f2aae86 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Mon, 15 May 2017 10:23:22 -0700 Subject: [PATCH 1/8] Issue #1553 - Guard against X509.isCertSign AIOOBE --- .../java/org/eclipse/jetty/util/ssl/X509.java | 8 +- .../util/ssl/X509CertificateAdapter.java | 192 ++++++++++++++++++ .../org/eclipse/jetty/util/ssl/X509Test.java | 126 ++++++++++++ 3 files changed, 324 insertions(+), 2 deletions(-) create mode 100644 jetty-util/src/test/java/org/eclipse/jetty/util/ssl/X509CertificateAdapter.java create mode 100644 jetty-util/src/test/java/org/eclipse/jetty/util/ssl/X509Test.java diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/X509.java b/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/X509.java index 692d0c1a946..03c637eee82 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/X509.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/X509.java @@ -52,8 +52,12 @@ public class X509 public static boolean isCertSign(X509Certificate x509) { - boolean[] key_usage=x509.getKeyUsage(); - return key_usage!=null && key_usage[KEY_USAGE__KEY_CERT_SIGN]; + boolean[] key_usage = x509.getKeyUsage(); + if ((key_usage == null) || (key_usage.length <= KEY_USAGE__KEY_CERT_SIGN)) + { + return false; + } + return key_usage[KEY_USAGE__KEY_CERT_SIGN]; } private final X509Certificate _x509; diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/ssl/X509CertificateAdapter.java b/jetty-util/src/test/java/org/eclipse/jetty/util/ssl/X509CertificateAdapter.java new file mode 100644 index 00000000000..3a9111a5135 --- /dev/null +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/ssl/X509CertificateAdapter.java @@ -0,0 +1,192 @@ +// +// ======================================================================== +// Copyright (c) 1995-2017 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.util.ssl; + +import java.math.BigInteger; +import java.security.InvalidKeyException; +import java.security.NoSuchAlgorithmException; +import java.security.NoSuchProviderException; +import java.security.Principal; +import java.security.PublicKey; +import java.security.SignatureException; +import java.security.cert.CertificateEncodingException; +import java.security.cert.CertificateException; +import java.security.cert.CertificateExpiredException; +import java.security.cert.CertificateNotYetValidException; +import java.security.cert.X509Certificate; +import java.util.Date; +import java.util.Set; + +/** + * Bogus X509Certificate to aide in testing + */ +public class X509CertificateAdapter extends X509Certificate +{ + @Override + public void checkValidity() throws CertificateExpiredException, CertificateNotYetValidException + { + } + + @Override + public void checkValidity(Date date) throws CertificateExpiredException, CertificateNotYetValidException + { + } + + @Override + public byte[] getEncoded() throws CertificateEncodingException + { + return new byte[0]; + } + + @Override + public boolean hasUnsupportedCriticalExtension() + { + return false; + } + + @Override + public Set getCriticalExtensionOIDs() + { + return null; + } + + @Override + public Set getNonCriticalExtensionOIDs() + { + return null; + } + + @Override + public byte[] getExtensionValue(String oid) + { + return new byte[0]; + } + + @Override + public void verify(PublicKey key) throws CertificateException, NoSuchAlgorithmException, InvalidKeyException, NoSuchProviderException, SignatureException + { + } + + @Override + public void verify(PublicKey key, String sigProvider) throws CertificateException, NoSuchAlgorithmException, InvalidKeyException, NoSuchProviderException, SignatureException + { + } + + @Override + public String toString() + { + return null; + } + + @Override + public PublicKey getPublicKey() + { + return null; + } + + @Override + public int getVersion() + { + return 0; + } + + @Override + public BigInteger getSerialNumber() + { + return null; + } + + @Override + public Principal getIssuerDN() + { + return null; + } + + @Override + public Principal getSubjectDN() + { + return null; + } + + @Override + public Date getNotBefore() + { + return null; + } + + @Override + public Date getNotAfter() + { + return null; + } + + @Override + public byte[] getTBSCertificate() throws CertificateEncodingException + { + return new byte[0]; + } + + @Override + public byte[] getSignature() + { + return new byte[0]; + } + + @Override + public String getSigAlgName() + { + return null; + } + + @Override + public String getSigAlgOID() + { + return null; + } + + @Override + public byte[] getSigAlgParams() + { + return new byte[0]; + } + + @Override + public boolean[] getIssuerUniqueID() + { + return new boolean[0]; + } + + @Override + public boolean[] getSubjectUniqueID() + { + return new boolean[0]; + } + + @Override + public boolean[] getKeyUsage() + { + return new boolean[0]; + } + + @Override + public int getBasicConstraints() + { + return 0; + } +} diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/ssl/X509Test.java b/jetty-util/src/test/java/org/eclipse/jetty/util/ssl/X509Test.java new file mode 100644 index 00000000000..98b0c918a95 --- /dev/null +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/ssl/X509Test.java @@ -0,0 +1,126 @@ +// +// ======================================================================== +// Copyright (c) 1995-2017 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.util.ssl; + +import static org.hamcrest.CoreMatchers.is; +import static org.junit.Assert.assertThat; + +import java.security.cert.X509Certificate; + +import org.junit.Test; + +public class X509Test +{ + @Test + public void testIsCertSign_Normal() + { + X509Certificate bogusX509 = new X509CertificateAdapter() + { + @Override + public boolean[] getKeyUsage() + { + boolean[] keyUsage = new boolean[8]; + keyUsage[5] = true; + return keyUsage; + } + }; + + assertThat("Normal X509", X509.isCertSign(bogusX509), is(true)); + } + + @Test + public void testIsCertSign_Normal_NoSupported() + { + X509Certificate bogusX509 = new X509CertificateAdapter() + { + @Override + public boolean[] getKeyUsage() + { + boolean[] keyUsage = new boolean[8]; + keyUsage[5] = false; + return keyUsage; + } + }; + + assertThat("Normal X509", X509.isCertSign(bogusX509), is(false)); + } + + @Test + public void testIsCertSign_NonStandard_Short() + { + X509Certificate bogusX509 = new X509CertificateAdapter() + { + @Override + public boolean[] getKeyUsage() + { + boolean[] keyUsage = new boolean[6]; // at threshold + keyUsage[5] = true; + return keyUsage; + } + }; + + assertThat("NonStandard X509", X509.isCertSign(bogusX509), is(true)); + } + + @Test + public void testIsCertSign_NonStandard_Shorter() + { + X509Certificate bogusX509 = new X509CertificateAdapter() + { + @Override + public boolean[] getKeyUsage() + { + boolean[] keyUsage = new boolean[5]; // just below threshold + return keyUsage; + } + }; + + assertThat("NonStandard X509", X509.isCertSign(bogusX509), is(false)); + } + + @Test + public void testIsCertSign_Normal_Null() + { + X509Certificate bogusX509 = new X509CertificateAdapter() + { + @Override + public boolean[] getKeyUsage() + { + return null; + } + }; + + assertThat("Normal X509", X509.isCertSign(bogusX509), is(false)); + } + + @Test + public void testIsCertSign_Normal_Empty() + { + X509Certificate bogusX509 = new X509CertificateAdapter() + { + @Override + public boolean[] getKeyUsage() + { + return new boolean[0]; + } + }; + + assertThat("Normal X509", X509.isCertSign(bogusX509), is(false)); + } +} From c5a0c5e7616b13008579355687fdc514117ecd36 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Mon, 15 May 2017 11:03:12 -0700 Subject: [PATCH 2/8] Issue #1546 - more leniency testcase Signed-off-by: Joakim Erdfelt --- .../jetty/server/CookieCutterTest.java | 22 ++++++++++++++--- .../server/CookieCutter_LenientTest.java | 24 ++++++++++++++++++- 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutterTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutterTest.java index e1303d8d80a..21e2db965d6 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutterTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutterTest.java @@ -138,10 +138,9 @@ public class CookieCutterTest * Example from RFC2965 */ @Test - @Ignore + @Ignore("comma separation no longer supported by RFC6265") public void testRFC2965_CookieSpoofingExample() { - // Ignored because comma separation no longer supported by RFC6265 String rawCookie = "$Version=\"1\"; session_id=\"1234\", " + "$Version=\"1\"; session_id=\"1111\"; $Domain=\".cracker.edu\""; @@ -182,7 +181,7 @@ public class CookieCutterTest } /** - * Basic key=value, following RFC6265 rules + * Basic name=value, following RFC6265 rules */ @Test public void testKeyValue() @@ -194,4 +193,21 @@ public class CookieCutterTest assertThat("Cookies.length", cookies.length, is(1)); assertCookie("Cookies[0]", cookies[0], "key", "value", 0, null); } + + /** + * Multiple name=value, heavy abuse, badly terminated quotes, lenient behavior test + */ + @Test + public void testMultiName_BadQuoteTerminate() + { + // TODO: this seems very hokey, and allowing this as 3 separate entries is probably a security issue. + String rawCookie = "a=\"b; $Path=/a; c=d; $PATH=/c; e=f\"; $Path=/e/"; + + Cookie cookies[] = parseCookieHeaders(rawCookie); + + assertThat("Cookies.length", cookies.length, is(3)); + assertCookie("Cookies[0]", cookies[0], "a", "\"b", 0, "/a"); + assertCookie("Cookies[1]", cookies[1], "c", "d", 0, "/c"); + assertCookie("Cookies[2]", cookies[2], "e", "f\"", 0, "/e/"); + } } diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutter_LenientTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutter_LenientTest.java index ece684758e8..4eb0f7d29a8 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutter_LenientTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutter_LenientTest.java @@ -76,7 +76,7 @@ public class CookieCutter_LenientTest ret.add(new String[]{"some-thing-else=to-parse", "some-thing-else", "to-parse"}); // RFC2109 - names with attr/token syntax starting with '$' (and not a cookie reserved word) // See https://tools.ietf.org/html/draft-ietf-httpbis-cookie-prefixes-00#section-5.2 - // Cannot pass names through as Cookie class does not allow them + // Cannot pass names through as javax.servlet.http.Cookie class does not allow them ret.add(new String[]{"$foo=bar", null, null}); // Tests that conform to RFC6265 @@ -94,12 +94,34 @@ public class CookieCutter_LenientTest ret.add(new String[]{"query=\"?b=c\"&\"d=e\"", "query", "?b=c\"&\"d=e"}); // Escaped quotes ret.add(new String[]{"foo=\"bar\\\"=\\\"baz\"", "foo", "bar\"=\"baz"}); + + // Unterminated Quotes + ret.add(new String[]{"x=\"abc", "x", "\"abc"}); + // Unterminated Quotes with valid cookie params after it + ret.add(new String[]{"x=\"abc $Path=/", "x", "\"abc"}); // UTF-8 values ret.add(new String[]{"2sides=\u262F", "2sides", "\u262f"}); // 2 byte ret.add(new String[]{"currency=\"\u20AC\"", "currency", "\u20AC"}); // 3 byte ret.add(new String[]{"gothic=\"\uD800\uDF48\"", "gothic", "\uD800\uDF48"}); // 4 byte + // Spaces + ret.add(new String[]{"foo=bar baz", "foo", "bar baz"}); + ret.add(new String[]{"foo=\"bar baz\"", "foo", "bar baz"}); + ret.add(new String[]{"z=a b c d e f g", "z", "a b c d e f g"}); + + // Bad tspecials usage + ret.add(new String[]{"foo=bar;baz", "foo", "bar;baz"}); // TODO: not sure supporting this is sane + ret.add(new String[]{"foo=\"bar;baz\"", "foo", "bar;baz"}); + ret.add(new String[]{"z=a;b,c:d;e/f[g]", "z", "a;b,c:d;e/f[g]"}); + ret.add(new String[]{"z=\"a;b,c:d;e/f[g]\"", "z", "a;b,c:d;e/f[g]"}); + + // Quoted with other Cookie keywords + ret.add(new String[]{"x=\"$Version=0\"", "x", "$Version=0"}); + ret.add(new String[]{"x=\"$Path=/\"", "x", "$Path=/"}); + ret.add(new String[]{"x=\"$Path=/ $Domain=.foo.com\"", "x", "$Path=/ $Domain=.foo.com"}); + ret.add(new String[]{"x=\" $Path=/ $Domain=.foo.com \"", "x", " $Path=/ $Domain=.foo.com "}); + // Lots of equals signs ret.add(new String[]{"query=b=c&d=e", "query", "b=c&d=e"}); From 166736db554dda8f7fe5623230da2bee22aca0b6 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Mon, 15 May 2017 21:01:15 +0200 Subject: [PATCH 3/8] Issue #1546 - more cookie leniency fixes --- .../eclipse/jetty/server/CookieCutter.java | 65 ++++++++++++------- .../jetty/server/CookieCutterTest.java | 17 ----- .../server/CookieCutter_LenientTest.java | 12 +++- 3 files changed, 50 insertions(+), 44 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/CookieCutter.java b/jetty-server/src/main/java/org/eclipse/jetty/server/CookieCutter.java index d17e38c5be3..189ad45c5d2 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/CookieCutter.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/CookieCutter.java @@ -126,17 +126,19 @@ public class CookieCutter Cookie cookie = null; boolean invalue=false; + boolean inQuoted=false; boolean quoted=false; - boolean unquotedToken=false; boolean escaped=false; int tokenstart=-1; int tokenend=-1; for (int i = 0, length = hdr.length(), last=length-1; i < length; i++) { char c = hdr.charAt(i); - + + // System.err.printf("i=%d c=%s v=%b q=%b e=%b u=%s s=%d e=%d%n" ,i,""+c,invalue,inQuoted,escaped,unquoted,tokenstart,tokenend); + // Handle quoted values for name or value - if (quoted) + if (inQuoted) { if (escaped) { @@ -148,25 +150,43 @@ public class CookieCutter switch (c) { case '"': - quoted=false; + inQuoted=false; if (i==last) { value = unquoted.toString(); } else { - unquotedToken=true; + quoted=true; tokenstart=i; tokenend=-1; } break; case '\\': - escaped=true; + if (i==last) + { + unquoted.setLength(0); + inQuoted = false; + i--; + } + else + { + escaped=true; + } continue; default: - unquoted.append(c); + if (i==last) + { + unquoted.setLength(0); + inQuoted = false; + i--; + } + else + { + unquoted.append(c); + } continue; } } @@ -183,16 +203,14 @@ public class CookieCutter continue; case ';': - if (unquotedToken) + if (quoted) { value = unquoted.toString(); unquoted.setLength(0); - unquotedToken = false; + quoted = false; } else if(tokenstart>=0 && tokenend>=0) value = hdr.substring(tokenstart, tokenend+1); - else - value=""; tokenstart = -1; invalue=false; @@ -201,7 +219,8 @@ public class CookieCutter case '"': if (tokenstart<0) { - quoted=true; + tokenstart=i; + inQuoted=true; if (unquoted==null) unquoted=new StringBuilder(); continue; @@ -209,12 +228,12 @@ public class CookieCutter // fall through to default case default: - if (unquotedToken) + if (quoted) { // must have been a bad internal quote. let's fix as best we can unquoted.append(hdr.substring(tokenstart,i)); - quoted = true; - unquotedToken = false; + inQuoted = true; + quoted = false; i--; continue; } @@ -239,27 +258,26 @@ public class CookieCutter continue; case ';': - if (unquotedToken) + if (quoted) { name = unquoted.toString(); unquoted.setLength(0); - unquotedToken = false; + quoted = false; } else if(tokenstart>=0 && tokenend>=0) { name = hdr.substring(tokenstart, tokenend+1); } - value = ""; tokenstart = -1; break; case '=': - if (unquotedToken) + if (quoted) { name = unquoted.toString(); unquoted.setLength(0); - unquotedToken = false; + quoted = false; } else if(tokenstart>=0 && tokenend>=0) { @@ -271,12 +289,12 @@ public class CookieCutter continue; default: - if (unquotedToken) + if (quoted) { // must have been a bad internal quote. let's fix as best we can unquoted.append(hdr.substring(tokenstart,i)); - quoted = true; - unquotedToken = false; + inQuoted = true; + quoted = false; i--; continue; } @@ -286,7 +304,6 @@ public class CookieCutter if (i==last) { name = hdr.substring(tokenstart, tokenend+1); - value = ""; break; } continue; diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutterTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutterTest.java index 21e2db965d6..02f53e46719 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutterTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutterTest.java @@ -193,21 +193,4 @@ public class CookieCutterTest assertThat("Cookies.length", cookies.length, is(1)); assertCookie("Cookies[0]", cookies[0], "key", "value", 0, null); } - - /** - * Multiple name=value, heavy abuse, badly terminated quotes, lenient behavior test - */ - @Test - public void testMultiName_BadQuoteTerminate() - { - // TODO: this seems very hokey, and allowing this as 3 separate entries is probably a security issue. - String rawCookie = "a=\"b; $Path=/a; c=d; $PATH=/c; e=f\"; $Path=/e/"; - - Cookie cookies[] = parseCookieHeaders(rawCookie); - - assertThat("Cookies.length", cookies.length, is(3)); - assertCookie("Cookies[0]", cookies[0], "a", "\"b", 0, "/a"); - assertCookie("Cookies[1]", cookies[1], "c", "d", 0, "/c"); - assertCookie("Cookies[2]", cookies[2], "e", "f\"", 0, "/e/"); - } } diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutter_LenientTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutter_LenientTest.java index 4eb0f7d29a8..7fab2dc1f72 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutter_LenientTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutter_LenientTest.java @@ -64,6 +64,8 @@ public class CookieCutter_LenientTest // RFC2109 - quoted-string values // quoted-string = ( <"> *(qdtext) <"> ) // qdtext = > + + ret.add(new String[]{"abc=\"\"", "abc", ""}); // The backslash character ("\") may be used as a single-character quoting // mechanism only within quoted-string and comment constructs. // quoted-pair = "\" CHAR @@ -98,7 +100,9 @@ public class CookieCutter_LenientTest // Unterminated Quotes ret.add(new String[]{"x=\"abc", "x", "\"abc"}); // Unterminated Quotes with valid cookie params after it - ret.add(new String[]{"x=\"abc $Path=/", "x", "\"abc"}); + ret.add(new String[]{"x=\"abc $Path=/", "x", "\"abc $Path=/"}); + // Unterminated Quotes with trailing escape + ret.add(new String[]{"x=\"abc\\", "x", "\"abc\\"}); // UTF-8 values ret.add(new String[]{"2sides=\u262F", "2sides", "\u262f"}); // 2 byte @@ -111,9 +115,9 @@ public class CookieCutter_LenientTest ret.add(new String[]{"z=a b c d e f g", "z", "a b c d e f g"}); // Bad tspecials usage - ret.add(new String[]{"foo=bar;baz", "foo", "bar;baz"}); // TODO: not sure supporting this is sane + ret.add(new String[]{"foo=bar;baz", "foo", "bar"}); ret.add(new String[]{"foo=\"bar;baz\"", "foo", "bar;baz"}); - ret.add(new String[]{"z=a;b,c:d;e/f[g]", "z", "a;b,c:d;e/f[g]"}); + ret.add(new String[]{"z=a;b,c:d;e/f[g]", "z", "a"}); ret.add(new String[]{"z=\"a;b,c:d;e/f[g]\"", "z", "a;b,c:d;e/f[g]"}); // Quoted with other Cookie keywords @@ -121,6 +125,7 @@ public class CookieCutter_LenientTest ret.add(new String[]{"x=\"$Path=/\"", "x", "$Path=/"}); ret.add(new String[]{"x=\"$Path=/ $Domain=.foo.com\"", "x", "$Path=/ $Domain=.foo.com"}); ret.add(new String[]{"x=\" $Path=/ $Domain=.foo.com \"", "x", " $Path=/ $Domain=.foo.com "}); + ret.add(new String[]{"a=\"b; $Path=/a; c=d; $PATH=/c; e=f\"; $Path=/e/", "a", "b; $Path=/a; c=d; $PATH=/c; e=f"}); // Lots of equals signs ret.add(new String[]{"query=b=c&d=e", "query", "b=c&d=e"}); @@ -161,4 +166,5 @@ public class CookieCutter_LenientTest assertThat("Cookie.value", cookies[0].getValue(), is(expectedValue)); } } + } From fc6ca37c770cde81335ced47a66a9cfdf28c13f6 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Mon, 15 May 2017 13:22:01 -0700 Subject: [PATCH 4/8] Updating expectations in RequestTest from changes in CookieCutter --- .../org/eclipse/jetty/server/RequestTest.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java index 696bf6809c1..40c4dd7c439 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java @@ -1343,14 +1343,14 @@ public class RequestTest assertEquals("/path", cookies.get(3).getPath()); assertEquals("acme.com", cookies.get(3).getDomain()); assertEquals("$port=8080", cookies.get(3).getComment()); - assertEquals("name4", cookies.get(4).getName()); - assertEquals("", cookies.get(4).getValue()); - assertEquals("name5", cookies.get(5).getName()); - assertEquals("", cookies.get(5).getValue()); - assertEquals("name6", cookies.get(6).getName()); - assertEquals("", cookies.get(6).getValue()); - assertEquals("name7", cookies.get(7).getName()); - assertEquals("value7", cookies.get(7).getValue()); + // assertEquals("name4", cookies.get(4).getName()); + // assertEquals("", cookies.get(4).getValue()); + // assertEquals("name5", cookies.get(5).getName()); + // assertEquals("", cookies.get(5).getValue()); + // assertEquals("name6", cookies.get(6).getName()); + // assertEquals("", cookies.get(6).getValue()); + assertEquals("name7", cookies.get(4).getName()); + assertEquals("value7", cookies.get(4).getValue()); cookies.clear(); response=_connector.getResponse( From 5141085fed2f5ff8cdc11f566580ba74b7c104ad Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Mon, 15 May 2017 23:34:28 +0200 Subject: [PATCH 5/8] Issue #1546 - more cookie fixes --- .../org/eclipse/jetty/server/CookieCutter.java | 1 + .../java/org/eclipse/jetty/server/RequestTest.java | 14 +++++++------- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/CookieCutter.java b/jetty-server/src/main/java/org/eclipse/jetty/server/CookieCutter.java index 189ad45c5d2..60885ee3470 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/CookieCutter.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/CookieCutter.java @@ -154,6 +154,7 @@ public class CookieCutter if (i==last) { value = unquoted.toString(); + unquoted.setLength(0); } else { diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java index 40c4dd7c439..0165a7692f8 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java @@ -1326,7 +1326,7 @@ public class RequestTest "POST / HTTP/1.1\r\n"+ "Host: whatever\r\n"+ "Cookie: name0=value0; name1 = value1 ; name2 = \"\\\"value2\\\"\" \n" + - "Cookie: $Version=2; name3=value3=value3;$path=/path;$domain=acme.com;$port=8080; name4=; name5 = ; name6\n" + + "Cookie: $Version=2; name3=value3=value3;$path=/path;$domain=acme.com;$port=8080; name4=\"\"; name5 = x ; name6\n" + "Cookie: name7=value7;\n" + "Connection: close\r\n"+ "\r\n"); @@ -1343,14 +1343,14 @@ public class RequestTest assertEquals("/path", cookies.get(3).getPath()); assertEquals("acme.com", cookies.get(3).getDomain()); assertEquals("$port=8080", cookies.get(3).getComment()); - // assertEquals("name4", cookies.get(4).getName()); - // assertEquals("", cookies.get(4).getValue()); - // assertEquals("name5", cookies.get(5).getName()); - // assertEquals("", cookies.get(5).getValue()); + assertEquals("name4", cookies.get(4).getName()); + assertEquals("", cookies.get(4).getValue()); + assertEquals("name5", cookies.get(5).getName()); + assertEquals("x", cookies.get(5).getValue()); // assertEquals("name6", cookies.get(6).getName()); // assertEquals("", cookies.get(6).getValue()); - assertEquals("name7", cookies.get(4).getName()); - assertEquals("value7", cookies.get(4).getValue()); + assertEquals("name7", cookies.get(6).getName()); + assertEquals("value7", cookies.get(6).getValue()); cookies.clear(); response=_connector.getResponse( From 155e3e9bcd92759683bce28633da015c132be154 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Tue, 16 May 2017 06:05:55 +0200 Subject: [PATCH 6/8] Issue #1546 - more cookie fixes --- .../eclipse/jetty/server/CookieCutter.java | 31 ++++++++++++++----- .../server/CookieCutter_LenientTest.java | 13 +++++++- .../org/eclipse/jetty/server/RequestTest.java | 4 +-- 3 files changed, 37 insertions(+), 11 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/CookieCutter.java b/jetty-server/src/main/java/org/eclipse/jetty/server/CookieCutter.java index 60885ee3470..13929c1d491 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/CookieCutter.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/CookieCutter.java @@ -180,6 +180,7 @@ public class CookieCutter default: if (i==last) { + // unterminated quote, let's ignore quotes unquoted.setLength(0); inQuoted = false; i--; @@ -201,7 +202,7 @@ public class CookieCutter { case ' ': case '\t': - continue; + break; case ';': if (quoted) @@ -212,6 +213,8 @@ public class CookieCutter } else if(tokenstart>=0 && tokenend>=0) value = hdr.substring(tokenstart, tokenend+1); + else + value = ""; tokenstart = -1; invalue=false; @@ -224,7 +227,7 @@ public class CookieCutter inQuoted=true; if (unquoted==null) unquoted=new StringBuilder(); - continue; + break; } // fall through to default case @@ -284,10 +287,9 @@ public class CookieCutter { name = hdr.substring(tokenstart, tokenend+1); } - tokenstart = -1; invalue=true; - continue; + break; default: if (quoted) @@ -303,17 +305,30 @@ public class CookieCutter tokenstart=i; tokenend=i; if (i==last) - { - name = hdr.substring(tokenstart, tokenend+1); break; - } continue; } } } + if (invalue && i==last && value==null) + { + if (quoted) + { + value = unquoted.toString(); + unquoted.setLength(0); + quoted = false; + } + else if(tokenstart>=0 && tokenend>=0) + { + value = hdr.substring(tokenstart, tokenend+1); + } + else + value = ""; + } + // If after processing the current character we have a value and a name, then it is a cookie - if (value!=null && name!=null) + if (name!=null && value!=null) { try { diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutter_LenientTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutter_LenientTest.java index 7fab2dc1f72..82d7e1754fa 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutter_LenientTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/CookieCutter_LenientTest.java @@ -65,7 +65,18 @@ public class CookieCutter_LenientTest // quoted-string = ( <"> *(qdtext) <"> ) // qdtext = > + // lenient with spaces and EOF + ret.add(new String[]{"abc=", "abc", ""}); + ret.add(new String[]{"abc = ", "abc", ""}); + ret.add(new String[]{"abc = ;", "abc", ""}); + ret.add(new String[]{"abc = ; ", "abc", ""}); + ret.add(new String[]{"abc = x ", "abc", "x"}); ret.add(new String[]{"abc=\"\"", "abc", ""}); + ret.add(new String[]{"abc= \"\" ", "abc", ""}); + ret.add(new String[]{"abc= \"x\" ", "abc", "x"}); + ret.add(new String[]{"abc= \"x\" ;", "abc", "x"}); + ret.add(new String[]{"abc= \"x\" ; ", "abc", "x"}); + // The backslash character ("\") may be used as a single-character quoting // mechanism only within quoted-string and comment constructs. // quoted-pair = "\" CHAR @@ -80,7 +91,7 @@ public class CookieCutter_LenientTest // See https://tools.ietf.org/html/draft-ietf-httpbis-cookie-prefixes-00#section-5.2 // Cannot pass names through as javax.servlet.http.Cookie class does not allow them ret.add(new String[]{"$foo=bar", null, null}); - + // Tests that conform to RFC6265 ret.add(new String[]{"abc=foobar!", "abc", "foobar!"}); ret.add(new String[]{"abc=\"foobar!\"", "abc", "foobar!"}); diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java index 0165a7692f8..fd693d7738c 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java @@ -1326,7 +1326,7 @@ public class RequestTest "POST / HTTP/1.1\r\n"+ "Host: whatever\r\n"+ "Cookie: name0=value0; name1 = value1 ; name2 = \"\\\"value2\\\"\" \n" + - "Cookie: $Version=2; name3=value3=value3;$path=/path;$domain=acme.com;$port=8080; name4=\"\"; name5 = x ; name6\n" + + "Cookie: $Version=2; name3=value3=value3;$path=/path;$domain=acme.com;$port=8080; name4=\"\"; name5 = ; name6\n" + "Cookie: name7=value7;\n" + "Connection: close\r\n"+ "\r\n"); @@ -1346,7 +1346,7 @@ public class RequestTest assertEquals("name4", cookies.get(4).getName()); assertEquals("", cookies.get(4).getValue()); assertEquals("name5", cookies.get(5).getName()); - assertEquals("x", cookies.get(5).getValue()); + assertEquals("", cookies.get(5).getValue()); // assertEquals("name6", cookies.get(6).getName()); // assertEquals("", cookies.get(6).getValue()); assertEquals("name7", cookies.get(6).getName()); From b1b94d870e0c8ac8d6fb9356ea4c455ddb56c20c Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Tue, 16 May 2017 10:25:34 +0200 Subject: [PATCH 7/8] Code cleanups. --- .../jetty/util/security/Credential.java | 29 ++++--------------- 1 file changed, 6 insertions(+), 23 deletions(-) diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/security/Credential.java b/jetty-util/src/main/java/org/eclipse/jetty/util/security/Credential.java index 54296ecbd4e..9b855faf092 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/security/Credential.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/security/Credential.java @@ -26,7 +26,6 @@ import org.eclipse.jetty.util.TypeUtil; import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Logger; -/* ------------------------------------------------------------ */ /** * Credentials. The Credential class represents an abstract mechanism for * checking authentication credentials. A credential instance either represents @@ -39,15 +38,12 @@ import org.eclipse.jetty.util.log.Logger; * This class includes an implementation for unix Crypt an MD5 digest. * * @see Password - * */ public abstract class Credential implements Serializable { + private static final long serialVersionUID = -7760551052768181572L; private static final Logger LOG = Log.getLogger(Credential.class); - private static final long serialVersionUID = -7760551052768181572L; - - /* ------------------------------------------------------------ */ /** * Check a credential * @@ -59,7 +55,6 @@ public abstract class Credential implements Serializable */ public abstract boolean check(Object credentials); - /* ------------------------------------------------------------ */ /** * Get a credential from a String. If the credential String starts with a * known Credential type (eg "CRYPT:" or "MD5:" ) then a Credential of that @@ -76,15 +71,13 @@ public abstract class Credential implements Serializable return new Password(credential); } - /* ------------------------------------------------------------ */ /** * Unix Crypt Credentials */ public static class Crypt extends Credential { private static final long serialVersionUID = -2027792997664744210L; - - public static final String __TYPE = "CRYPT:"; + private static final String __TYPE = "CRYPT:"; private final String _cooked; @@ -100,58 +93,49 @@ public abstract class Credential implements Serializable credentials=new String((char[])credentials); if (!(credentials instanceof String) && !(credentials instanceof Password)) LOG.warn("Can't check " + credentials.getClass() + " against CRYPT"); - String passwd = credentials.toString(); return _cooked.equals(UnixCrypt.crypt(passwd, _cooked)); } public static String crypt(String user, String pw) { - return "CRYPT:" + UnixCrypt.crypt(pw, user); + return __TYPE + UnixCrypt.crypt(pw, user); } } - /* ------------------------------------------------------------ */ /** * MD5 Credentials */ public static class MD5 extends Credential { private static final long serialVersionUID = 5533846540822684240L; - - public static final String __TYPE = "MD5:"; - - public static final Object __md5Lock = new Object(); - + private static final String __TYPE = "MD5:"; + private static final Object __md5Lock = new Object(); private static MessageDigest __md; private final byte[] _digest; - /* ------------------------------------------------------------ */ MD5(String digest) { digest = digest.startsWith(__TYPE) ? digest.substring(__TYPE.length()) : digest; _digest = TypeUtil.parseBytes(digest, 16); } - /* ------------------------------------------------------------ */ public byte[] getDigest() { return _digest; } - /* ------------------------------------------------------------ */ @Override public boolean check(Object credentials) { try { - byte[] digest = null; - if (credentials instanceof char[]) credentials=new String((char[])credentials); if (credentials instanceof Password || credentials instanceof String) { + byte[] digest; synchronized (__md5Lock) { if (__md == null) __md = MessageDigest.getInstance("MD5"); @@ -193,7 +177,6 @@ public abstract class Credential implements Serializable } } - /* ------------------------------------------------------------ */ public static String digest(String password) { try From 042f325f1cd6e7891d72c7e668f5947b5457dc02 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Tue, 16 May 2017 10:41:08 +0200 Subject: [PATCH 8/8] Fixes #1556 - A timing channel in Password.java. --- .../jaspi/modules/DigestAuthModule.java | 3 +- .../authentication/DigestAuthenticator.java | 2 +- .../jetty/util/security/Credential.java | 57 ++++++++++++++----- .../eclipse/jetty/util/security/Password.java | 25 ++++---- 4 files changed, 57 insertions(+), 30 deletions(-) diff --git a/jetty-jaspi/src/main/java/org/eclipse/jetty/security/jaspi/modules/DigestAuthModule.java b/jetty-jaspi/src/main/java/org/eclipse/jetty/security/jaspi/modules/DigestAuthModule.java index afe3bcf1adc..54ad4f062a1 100644 --- a/jetty-jaspi/src/main/java/org/eclipse/jetty/security/jaspi/modules/DigestAuthModule.java +++ b/jetty-jaspi/src/main/java/org/eclipse/jetty/security/jaspi/modules/DigestAuthModule.java @@ -341,7 +341,7 @@ public class DigestAuthModule extends BaseAuthModule byte[] digest = md.digest(); // check digest - return (TypeUtil.toString(digest, 16).equalsIgnoreCase(response)); + return stringEquals(TypeUtil.toString(digest, 16).toLowerCase(), response == null ? null : response.toLowerCase()); } catch (Exception e) { @@ -356,6 +356,5 @@ public class DigestAuthModule extends BaseAuthModule { return username + "," + response; } - } } diff --git a/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/DigestAuthenticator.java b/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/DigestAuthenticator.java index a982f56aa6b..45055670119 100644 --- a/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/DigestAuthenticator.java +++ b/jetty-security/src/main/java/org/eclipse/jetty/security/authentication/DigestAuthenticator.java @@ -401,7 +401,7 @@ public class DigestAuthenticator extends LoginAuthenticator byte[] digest = md.digest(); // check digest - return (TypeUtil.toString(digest, 16).equalsIgnoreCase(response)); + return stringEquals(TypeUtil.toString(digest, 16).toLowerCase(), response == null ? null : response.toLowerCase()); } catch (Exception e) { diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/security/Credential.java b/jetty-util/src/main/java/org/eclipse/jetty/util/security/Credential.java index 9b855faf092..77692d35c4b 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/security/Credential.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/security/Credential.java @@ -71,6 +71,44 @@ public abstract class Credential implements Serializable return new Password(credential); } + /** + *

Utility method that replaces String.equals() to avoid timing attacks.

+ * + * @param s1 the first string to compare + * @param s2 the second string to compare + * @return whether the two strings are equal + */ + protected static boolean stringEquals(String s1, String s2) + { + if (s1 == s2) + return true; + if (s1 == null || s2 == null || s1.length() != s2.length()) + return false; + boolean result = false; + for (int i = 0; i < s1.length(); i++) + result |= s1.charAt(i) == s2.charAt(i); + return result; + } + + /** + *

Utility method that replaces Arrays.equals() to avoid timing attacks.

+ * + * @param b1 the first byte array to compare + * @param b2 the second byte array to compare + * @return whether the two byte arrays are equal + */ + protected static boolean byteEquals(byte[] b1, byte[] b2) + { + if (b1 == b2) + return true; + if (b1 == null || b2 == null || b1.length != b2.length) + return false; + boolean result = false; + for (int i = 0; i < b1.length; i++) + result |= b1[i] == b2[i]; + return result; + } + /** * Unix Crypt Credentials */ @@ -93,8 +131,7 @@ public abstract class Credential implements Serializable credentials=new String((char[])credentials); if (!(credentials instanceof String) && !(credentials instanceof Password)) LOG.warn("Can't check " + credentials.getClass() + " against CRYPT"); - String passwd = credentials.toString(); - return _cooked.equals(UnixCrypt.crypt(passwd, _cooked)); + return stringEquals(_cooked, UnixCrypt.crypt(credentials.toString(), _cooked)); } public static String crypt(String user, String pw) @@ -143,26 +180,18 @@ public abstract class Credential implements Serializable __md.update(credentials.toString().getBytes(StandardCharsets.ISO_8859_1)); digest = __md.digest(); } - if (digest == null || digest.length != _digest.length) return false; - boolean digestMismatch = false; - for (int i = 0; i < digest.length; i++) - digestMismatch |= (digest[i] != _digest[i]); - return !digestMismatch; + return byteEquals(_digest, digest); } else if (credentials instanceof MD5) { - MD5 md5 = (MD5) credentials; - if (_digest.length != md5._digest.length) return false; - boolean digestMismatch = false; - for (int i = 0; i < _digest.length; i++) - digestMismatch |= (_digest[i] != md5._digest[i]); - return !digestMismatch; + MD5 md5 = (MD5)credentials; + return byteEquals(_digest, md5._digest); } else if (credentials instanceof Credential) { // Allow credential to attempt check - i.e. this'll work // for DigestAuthModule$Digest credentials - return ((Credential) credentials).check(this); + return ((Credential)credentials).check(this); } else { diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/security/Password.java b/jetty-util/src/main/java/org/eclipse/jetty/util/security/Password.java index 81ff1b92bd0..fe7839bf4e0 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/security/Password.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/security/Password.java @@ -20,7 +20,6 @@ package org.eclipse.jetty.util.security; import java.io.IOException; import java.nio.charset.StandardCharsets; -import java.util.Arrays; import java.util.Locale; import org.eclipse.jetty.util.log.Log; @@ -96,15 +95,20 @@ public class Password extends Credential @Override public boolean check(Object credentials) { - if (this == credentials) return true; + if (this == credentials) + return true; - if (credentials instanceof Password) return credentials.equals(_pw); + if (credentials instanceof Password) + return credentials.equals(_pw); - if (credentials instanceof String) return credentials.equals(_pw); + if (credentials instanceof String) + return stringEquals(_pw, (String)credentials); - if (credentials instanceof char[]) return Arrays.equals(_pw.toCharArray(), (char[]) credentials); + if (credentials instanceof char[]) + return stringEquals(_pw, new String((char[])credentials)); - if (credentials instanceof Credential) return ((Credential) credentials).check(_pw); + if (credentials instanceof Credential) + return ((Credential)credentials).check(_pw); return false; } @@ -120,14 +124,10 @@ public class Password extends Credential return false; if (o instanceof Password) - { - Password p = (Password) o; - //noinspection StringEquality - return p._pw == _pw || (null != _pw && _pw.equals(p._pw)); - } + return stringEquals(_pw, ((Password)o)._pw); if (o instanceof String) - return o.equals(_pw); + return stringEquals(_pw, (String)o); return false; } @@ -175,7 +175,6 @@ public class Password extends Credential } return buf.toString(); - } /* ------------------------------------------------------------ */