From 05691e16463e02b8376d05c8416722e437e86470 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Thu, 17 Mar 2016 12:00:18 -0700 Subject: [PATCH 1/4] Issue #438 - File and Path Resources with control characters should be rejected + Adding testcases + Cleaning up unit tests, adding more + Fixing one testcase related to FileResource.addPath() + Adding validation of filesystem paths Signed-off-by: Joakim Erdfelt --- .../jetty/util/resource/FileResource.java | 22 ++++- .../jetty/util/resource/PathResource.java | 13 +++ .../util/resource/AbstractFSResourceTest.java | 95 ++++++++++++++++++- .../jetty/util/resource/FileResourceTest.java | 15 --- 4 files changed, 123 insertions(+), 22 deletions(-) diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/FileResource.java b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/FileResource.java index d2f07c93a17..5ecc6c5e121 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/FileResource.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/FileResource.java @@ -29,8 +29,11 @@ import java.net.URL; import java.net.URLConnection; import java.nio.channels.FileChannel; import java.nio.channels.ReadableByteChannel; +import java.nio.file.InvalidPathException; import java.nio.file.StandardOpenOption; import java.security.Permission; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import org.eclipse.jetty.util.IO; import org.eclipse.jetty.util.URIUtil; @@ -50,6 +53,7 @@ import org.eclipse.jetty.util.log.Logger; public class FileResource extends Resource { private static final Logger LOG = Log.getLogger(FileResource.class); + private static final Pattern CNTRL_PATTERN = Pattern.compile("\\p{Cntrl}"); /* ------------------------------------------------------------ */ private final File _file; @@ -65,6 +69,7 @@ public class FileResource extends Resource { // Try standard API to convert URL to file. file =new File(url.toURI()); + assertValidPath(file.toString()); } catch (URISyntaxException e) { @@ -108,6 +113,7 @@ public class FileResource extends Resource _file=file; URI file_uri=_file.toURI(); _uri=normalizeURI(_file,uri); + assertValidPath(file.toString()); // Is it a URI alias? if (!URIUtil.equalsIgnoreEncodings(_uri,file_uri.toString())) @@ -119,6 +125,7 @@ public class FileResource extends Resource /* -------------------------------------------------------- */ FileResource(File file) { + assertValidPath(file.toString()); _file=file; _uri=normalizeURI(_file,_file.toURI()); _alias=checkFileAlias(_file); @@ -179,6 +186,7 @@ public class FileResource extends Resource public Resource addPath(String path) throws IOException,MalformedURLException { + assertValidPath(path); path = org.eclipse.jetty.util.URIUtil.canonicalPath(path); if (path==null) @@ -204,13 +212,21 @@ public class FileResource extends Resource } catch(final URISyntaxException e) { - throw new MalformedURLException(){{initCause(e);}}; + throw new InvalidPathException(path, e.getMessage()); } return new FileResource(uri); } - - + + private void assertValidPath(String path) + { + Matcher mat = CNTRL_PATTERN.matcher(path); + if(mat.find()) + { + throw new InvalidPathException(path, "Invalid Character at index " + mat.start()); + } + } + /* ------------------------------------------------------------ */ @Override public URI getAlias() diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java index 5d0c7bc4abe..68ee7628e09 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java @@ -38,6 +38,8 @@ import java.nio.file.StandardOpenOption; import java.nio.file.attribute.FileTime; import java.util.ArrayList; import java.util.List; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Logger; @@ -48,6 +50,7 @@ import org.eclipse.jetty.util.log.Logger; public class PathResource extends Resource { private static final Logger LOG = Log.getLogger(PathResource.class); + private static final Pattern CNTRL_PATTERN = Pattern.compile("\\p{Cntrl}"); private final Path path; private final URI uri; @@ -61,6 +64,7 @@ public class PathResource extends Resource public PathResource(Path path) { this.path = path; + assertValidPath(path); this.uri = this.path.toUri(); } @@ -110,6 +114,15 @@ public class PathResource extends Resource return new PathResource(this.path.getFileSystem().getPath(path.toString(), apath)); } + private void assertValidPath(Path path) + { + Matcher mat = CNTRL_PATTERN.matcher(path.toString()); + if(mat.find()) + { + throw new InvalidPathException(path.toString(), "Invalid Character at index " + mat.start()); + } + } + @Override public void close() { diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/resource/AbstractFSResourceTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/resource/AbstractFSResourceTest.java index deaff22dc11..3edda218110 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/resource/AbstractFSResourceTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/resource/AbstractFSResourceTest.java @@ -18,10 +18,6 @@ package org.eclipse.jetty.util.resource; -import static org.hamcrest.Matchers.*; -import static org.junit.Assert.*; -import static org.junit.Assume.*; - import java.io.File; import java.io.FileWriter; import java.io.IOException; @@ -51,6 +47,14 @@ import org.junit.Assert; import org.junit.Rule; import org.junit.Test; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.notNullValue; +import static org.hamcrest.Matchers.nullValue; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.fail; +import static org.junit.Assume.assumeFalse; +import static org.junit.Assume.assumeNoException; + public abstract class AbstractFSResourceTest { @Rule @@ -517,6 +521,89 @@ public abstract class AbstractFSResourceTest } } + @Test + public void testExist_BadControlChars_Encoded() throws Exception + { + createEmptyFile("a.jsp"); + + try + { + // request with control characters + URI ref = testdir.getDir().toURI().resolve("a.jsp%1F%10"); + assertThat("ControlCharacters URI",ref,notNullValue()); + + Resource fileref = newResource(ref); + assertThat("File Resource should not exists", fileref.exists(), is(false)); + } + catch (InvalidPathException e) + { + // Expected path + } + } + + @Test + public void testExist_BadControlChars_Decoded() throws Exception + { + createEmptyFile("a.jsp"); + + try + { + // request with control characters + File badFile = new File(testdir.getDir(), "a.jsp\014\010"); + newResource(badFile); + fail("Should have thrown " + InvalidPathException.class); + } + catch (InvalidPathException e) + { + // Expected path + } + } + + @Test + public void testExist_AddPath_BadControlChars_Decoded() throws Exception + { + createEmptyFile("a.jsp"); + + try + { + // base resource + URI ref = testdir.getDir().toURI(); + Resource base = newResource(ref); + assertThat("Base Resource URI",ref,notNullValue()); + + // add path with control characters (raw/decoded control characters) + // This MUST fail + base.addPath("/a.jsp\014\010"); + fail("Should have thrown " + InvalidPathException.class); + } + catch (InvalidPathException e) + { + // Expected path + } + } + + @Test + public void testExist_AddPath_BadControlChars_Encoded() throws Exception + { + createEmptyFile("a.jsp"); + + try + { + // base resource + URI ref = testdir.getDir().toURI(); + Resource base = newResource(ref); + assertThat("Base Resource URI",ref,notNullValue()); + + // add path with control characters + Resource fileref = base.addPath("/a.jsp%14%10"); + assertThat("File Resource should not exists", fileref.exists(), is(false)); + } + catch (InvalidPathException e) + { + // Expected path + } + } + @Test public void testUtf8Dir() throws Exception { diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/resource/FileResourceTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/resource/FileResourceTest.java index ff63159f911..afa16e57266 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/resource/FileResourceTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/resource/FileResourceTest.java @@ -22,9 +22,6 @@ import java.io.File; import java.io.IOException; import java.net.URI; -import org.junit.Ignore; -import org.junit.Test; - public class FileResourceTest extends AbstractFSResourceTest { @Override @@ -38,16 +35,4 @@ public class FileResourceTest extends AbstractFSResourceTest { return new FileResource(file); } - - @Ignore("Cannot get null to be seen by FileResource") - @Test - public void testExist_BadNull() throws Exception - { - } - - @Ignore("Validation shouldn't be done in FileResource") - @Test - public void testExist_BadNullX() throws Exception - { - } } From 4822bea0b189ae157082504521c507d86c5861d9 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Fri, 18 Mar 2016 11:51:32 -0700 Subject: [PATCH 2/4] Issue #438 - File and Path Resources with control characters should be rejected + Removing regex + Adding StringUtil.indexOfControlChars() Signed-off-by: Joakim Erdfelt --- .../org/eclipse/jetty/util/StringUtil.java | 47 +++++++++++++++++++ .../jetty/util/resource/FileResource.java | 10 ++-- .../jetty/util/resource/PathResource.java | 11 ++--- .../eclipse/jetty/util/StringUtilTest.java | 31 +++++++++--- 4 files changed, 81 insertions(+), 18 deletions(-) diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/StringUtil.java b/jetty-util/src/main/java/org/eclipse/jetty/util/StringUtil.java index c1b1d4319a5..9dbd9e49b73 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/StringUtil.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/StringUtil.java @@ -374,6 +374,53 @@ public class StringUtil } } + /** + * Find the index of a control characters in String + *

+ * This will return a result on the first occurrence of a control character, regardless if + * there are more than one. + *

+ *

+ * Note: uses codepoint version of {@link Character#isISOControl(int)} to support Unicode better. + *

+ * + *
+     *   indexOfControlChars(null)      == -1
+     *   indexOfControlChars("")        == -1
+     *   indexOfControlChars("\r\n")    == 0
+     *   indexOfControlChars("\t")      == 0
+     *   indexOfControlChars("   ")     == -1
+     *   indexOfControlChars("a")       == -1
+     *   indexOfControlChars(".")       == -1
+     *   indexOfControlChars(";\n")     == 1
+     *   indexOfControlChars("abc\f")   == 3
+     *   indexOfControlChars("z\010")   == 1
+     *   indexOfControlChars(":\u001c") == 1
+     * 
+ * + * @param str + * the string to test. + * @return the index of first control character in string, -1 if no control characters encountered + */ + public static int indexOfControlChars(String str) + { + if (str == null) + { + return -1; + } + int len = str.length(); + for (int i = 0; i < len; i++) + { + if (Character.isISOControl(str.codePointAt(i))) + { + // found a control character, we can stop searching now + return i; + } + } + // no control characters + return -1; + } + /* ------------------------------------------------------------ */ /** * Test if a string is null or only has whitespace characters in it. diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/FileResource.java b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/FileResource.java index 5ecc6c5e121..dbc4670a727 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/FileResource.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/FileResource.java @@ -32,10 +32,9 @@ import java.nio.channels.ReadableByteChannel; import java.nio.file.InvalidPathException; import java.nio.file.StandardOpenOption; import java.security.Permission; -import java.util.regex.Matcher; -import java.util.regex.Pattern; import org.eclipse.jetty.util.IO; +import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.URIUtil; import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Logger; @@ -53,7 +52,6 @@ import org.eclipse.jetty.util.log.Logger; public class FileResource extends Resource { private static final Logger LOG = Log.getLogger(FileResource.class); - private static final Pattern CNTRL_PATTERN = Pattern.compile("\\p{Cntrl}"); /* ------------------------------------------------------------ */ private final File _file; @@ -220,10 +218,10 @@ public class FileResource extends Resource private void assertValidPath(String path) { - Matcher mat = CNTRL_PATTERN.matcher(path); - if(mat.find()) + int idx = StringUtil.indexOfControlChars(path); + if (idx >= 0) { - throw new InvalidPathException(path, "Invalid Character at index " + mat.start()); + throw new InvalidPathException(path, "Invalid Character at index " + idx); } } diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java index 68ee7628e09..8e773676ea7 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java @@ -38,9 +38,8 @@ import java.nio.file.StandardOpenOption; import java.nio.file.attribute.FileTime; import java.util.ArrayList; import java.util.List; -import java.util.regex.Matcher; -import java.util.regex.Pattern; +import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Logger; @@ -50,7 +49,6 @@ import org.eclipse.jetty.util.log.Logger; public class PathResource extends Resource { private static final Logger LOG = Log.getLogger(PathResource.class); - private static final Pattern CNTRL_PATTERN = Pattern.compile("\\p{Cntrl}"); private final Path path; private final URI uri; @@ -116,10 +114,11 @@ public class PathResource extends Resource private void assertValidPath(Path path) { - Matcher mat = CNTRL_PATTERN.matcher(path.toString()); - if(mat.find()) + String str = path.toString(); + int idx = StringUtil.indexOfControlChars(str); + if(idx >= 0) { - throw new InvalidPathException(path.toString(), "Invalid Character at index " + mat.start()); + throw new InvalidPathException(str, "Invalid Character at index " + idx); } } diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/StringUtilTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/StringUtilTest.java index a33170a629c..09889fa891b 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/StringUtilTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/StringUtilTest.java @@ -18,20 +18,20 @@ package org.eclipse.jetty.util; +import java.nio.charset.StandardCharsets; + +import org.junit.Assert; +import org.junit.Test; + import static org.hamcrest.Matchers.arrayContaining; import static org.hamcrest.Matchers.emptyArray; +import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.nullValue; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; -import java.nio.charset.StandardCharsets; - -import org.hamcrest.Matchers; -import org.junit.Assert; -import org.junit.Test; - public class StringUtilTest { @Test @@ -205,6 +205,25 @@ public class StringUtilTest System.err.println(calc); } + @Test + public void testHasControlCharacter() + { + assertThat(StringUtil.indexOfControlChars("\r\n"), is(0)); + assertThat(StringUtil.indexOfControlChars("\t"), is(0)); + assertThat(StringUtil.indexOfControlChars(";\n"), is(1)); + assertThat(StringUtil.indexOfControlChars("abc\fz"), is(3)); + assertThat(StringUtil.indexOfControlChars("z\010"), is(1)); + assertThat(StringUtil.indexOfControlChars(":\u001c"), is(1)); + + assertThat(StringUtil.indexOfControlChars(null), is(-1)); + assertThat(StringUtil.indexOfControlChars(""), is(-1)); + assertThat(StringUtil.indexOfControlChars(" "), is(-1)); + assertThat(StringUtil.indexOfControlChars("a"), is(-1)); + assertThat(StringUtil.indexOfControlChars("."), is(-1)); + assertThat(StringUtil.indexOfControlChars(";"), is(-1)); + assertThat(StringUtil.indexOfControlChars("Euro is \u20ac"), is(-1)); + } + @Test public void testIsBlank() { From 60bccc178e5939a529004be2e4ec0ed882383bcf Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Thu, 31 Mar 2016 18:41:03 +1100 Subject: [PATCH 3/4] Issue #469 update to apache jasper 8.0.33 --- jetty-osgi/test-jetty-osgi/pom.xml | 2 +- pom.xml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/jetty-osgi/test-jetty-osgi/pom.xml b/jetty-osgi/test-jetty-osgi/pom.xml index 09b442b5172..21310a545a3 100644 --- a/jetty-osgi/test-jetty-osgi/pom.xml +++ b/jetty-osgi/test-jetty-osgi/pom.xml @@ -166,7 +166,7 @@ org.mortbay.jasper apache-el - 8.0.9.M3 + 8.0.33 test diff --git a/pom.xml b/pom.xml index db82c5b26e8..a07adcabfcf 100644 --- a/pom.xml +++ b/pom.xml @@ -549,7 +549,7 @@ org.mortbay.jasper apache-jsp - 8.0.27 + 8.0.33 From 3e6bc4d17a239eaedbd2341dfb7982c84e3ab317 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Mon, 4 Apr 2016 10:57:50 +0200 Subject: [PATCH 4/4] Updated ALPN version for JDK 8u77. --- .../config/modules/protonego-impl/alpn-1.8.0_77.mod | 8 ++++++++ .../modules/protonego-impl/alpn-1.8.0_77.mod | 8 ++++++++ pom.xml | 12 ++++++++++++ 3 files changed, 28 insertions(+) create mode 100644 jetty-alpn/jetty-alpn-server/src/main/config/modules/protonego-impl/alpn-1.8.0_77.mod create mode 100644 jetty-start/src/test/resources/dist-home/modules/protonego-impl/alpn-1.8.0_77.mod diff --git a/jetty-alpn/jetty-alpn-server/src/main/config/modules/protonego-impl/alpn-1.8.0_77.mod b/jetty-alpn/jetty-alpn-server/src/main/config/modules/protonego-impl/alpn-1.8.0_77.mod new file mode 100644 index 00000000000..3628757cbfd --- /dev/null +++ b/jetty-alpn/jetty-alpn-server/src/main/config/modules/protonego-impl/alpn-1.8.0_77.mod @@ -0,0 +1,8 @@ +[name] +protonego-boot + +[files] +http://central.maven.org/maven2/org/mortbay/jetty/alpn/alpn-boot/8.1.7.v20160121/alpn-boot-8.1.7.v20160121.jar|lib/alpn/alpn-boot-8.1.7.v20160121.jar + +[exec] +-Xbootclasspath/p:lib/alpn/alpn-boot-8.1.7.v20160121.jar diff --git a/jetty-start/src/test/resources/dist-home/modules/protonego-impl/alpn-1.8.0_77.mod b/jetty-start/src/test/resources/dist-home/modules/protonego-impl/alpn-1.8.0_77.mod new file mode 100644 index 00000000000..3628757cbfd --- /dev/null +++ b/jetty-start/src/test/resources/dist-home/modules/protonego-impl/alpn-1.8.0_77.mod @@ -0,0 +1,8 @@ +[name] +protonego-boot + +[files] +http://central.maven.org/maven2/org/mortbay/jetty/alpn/alpn-boot/8.1.7.v20160121/alpn-boot-8.1.7.v20160121.jar|lib/alpn/alpn-boot-8.1.7.v20160121.jar + +[exec] +-Xbootclasspath/p:lib/alpn/alpn-boot-8.1.7.v20160121.jar diff --git a/pom.xml b/pom.xml index a07adcabfcf..a93de594f94 100644 --- a/pom.xml +++ b/pom.xml @@ -1204,5 +1204,17 @@ 8.1.7.v20160121 + + 8u77 + + + java.version + 1.8.0_77 + + + + 8.1.7.v20160121 + +