Ubuntu
tomcat8 package

Merge ~kstenerud/ubuntu/+source/tomcat8:xenial-tomcat-resource-names-1606331 into ubuntu/+source/tomcat8:ubuntu/xenial-devel

  1. Git
  2. lp:~kstenerud/ubuntu/+source/tomcat8
  3. xenial-tomcat-resource-names-1606331
  4. Merge into ubuntu/xenial-devel
Proposed by Karl Stenerud
Status: Merged
Approved by: Andreas Hasenack
Approved revision: 4d56628304b1c3a940067debbffa71faa7123324
Merge reported by: Andreas Hasenack
Merged at revision: 624359d557ebbc7ca6d5d8804090ed30ba018b15
Proposed branch: ~kstenerud/ubuntu/+source/tomcat8:xenial-tomcat-resource-names-1606331
Merge into: ubuntu/+source/tomcat8:ubuntu/xenial-devel
Diff against target: 171 lines (+149/-0)
3 files modified
debian/changelog (+7/-0)
debian/patches/fix-class-resource-name-filtering.patch (+141/-0)
debian/patches/series (+1/-0)
Reviewer Review Type Date Requested Status
Andreas Hasenack Approve
Canonical Server Pending
Review via email: mp+359229@code.launchpad.net

Description of the change

Copied upstream fix http://svn.apache.org/viewvc/tomcat/tc8.0.x/trunk/java/org/apache/catalina/loader/WebappClassLoaderBase.java?diff_format=h&revision=1730178&view=markup

PPA: https://launchpad.net/~kstenerud/+archive/ubuntu/xenial-tomcat-resource-names2-1606331

Test case:

The successful test run should throw a ClassNotFoundException instead of a StringIndexOutOfBoundsException.

# lxc launch ubuntu:xenial tester && lxc exec tester bash
# apt update && apt dist-upgrade -y && apt install -y tomcat8 && mkdir -p /var/lib/tomcat8/webapps/test && echo '<%@ page contentType="text/html;charset=UTF-8" language="java" %>
<html>
<head>
    <title>$Title$</title>
</head>
<body>
<%
    Class.forName("org");
%>
</body>
</html>
' >/var/lib/tomcat8/webapps/test/test.jsp
# service tomcat8 restart
# curl localhost:8080/test/test.jsp
...
 An exception occurred processing JSP page /test.jsp at line 8
5: &lt;/head&gt;
6: &lt;body&gt;
7: &lt;%
8: Class.forName(&quot;org&quot;);
9: %&gt;
10: &lt;/body&gt;
11: &lt;/html&gt;
...
</pre><p><b>root cause</b></p><pre>java.lang.StringIndexOutOfBoundsException: String index out of range: 3
...

# add-apt-repository -y ppa:kstenerud/xenial-tomcat-resource-names2-1606331 && apt update && apt dist-upgrade -y
# service tomcat8 restart
# curl localhost:8080/test/test.jsp
...
 An exception occurred processing JSP page /test.jsp at line 8
5: &lt;/head&gt;
6: &lt;body&gt;
7: &lt;%
8: Class.forName(&quot;org&quot;);
9: %&gt;
10: &lt;/body&gt;
11: &lt;/html&gt;
...
</pre><p><b>root cause</b></p><pre>java.lang.ClassNotFoundException: org
...

Package Tests: None

To post a comment you must log in.
Revision history for this message
Andreas Hasenack (ahasenack) wrote :

n/f, comment inline

review: Needs Fixing
Revision history for this message
Andreas Hasenack (ahasenack) wrote :

> # add-apt-repository -y ppa:kstenerud/xenial-tomcat-resource-names2-1606331 && apt update && apt
> dist-upgrade -y
> # service tomcat8 restart
> # curl localhost:8080/test/test.jsp

Why is a manual service restart needed after the update?

Revision history for this message
Karl Stenerud (kstenerud) wrote :

Rebased and pushed.

I'm not 100% sure the manual restart is necessary. I had issues getting it to repro before and was trying things. It's probably not necessary.

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

Thanks for the changes, looks good, +1.

review: Approve
Revision history for this message
Andreas Hasenack (ahasenack) wrote :

Tagged and uploaded.

~kstenerud/ubuntu/+source/tomcat8:xenial-tomcat-resource-names-1606331 updated
0cc4932... by Karl Stenerud

  * d/p/fix-class-resource-name-filtering.patch: Fix class and resource name
    filtering in WebappClassLoader (LP: #1606331).

624359d... by Karl Stenerud

changelog

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

FYI this is in xenial-unapproved since andreas sponsored it.
=> https://launchpadlibrarian.net/399412813/tomcat8_8.0.32-1ubuntu1.9_source.changes
on
=> https://launchpad.net/ubuntu/xenial/+queue?queue_state=1

Not sure why I see updates 19 hours ago?

@Karl do we want/need the SRU team to reject it from unapproved to re-upload something new?
Or is the "19 hours ago" here on LP a red herring and we just wait for the SRU team to accept it?

Revision history for this message
Karl Stenerud (kstenerud) wrote :

rbasak asked to add the test included in the cherry-picked commit. I had the original upload cancelled from Xenial-unapproved, please tag and sponsor again.

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

Tag updated and package uploaded.

Revision history for this message
Andreas Hasenack (ahasenack) wrote :

This landed in xenial-proposed, and is thus merged:
$ rmadison tomcat8|grep xenial
 tomcat8 | 8.0.32-1ubuntu1 | xenial | source, all
 tomcat8 | 8.0.32-1ubuntu1.8 | xenial-security | source, all
 tomcat8 | 8.0.32-1ubuntu1.8 | xenial-updates | source, all
 tomcat8 | 8.0.32-1ubuntu1.9 | xenial-proposed | source, all

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/debian/changelog b/debian/changelog
2index af3ab55..652d5dd 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,10 @@
6+tomcat8 (8.0.32-1ubuntu1.9) xenial; urgency=medium
7+
8+ * d/p/fix-class-resource-name-filtering.patch: Fix class and resource name
9+ filtering in WebappClassLoader (LP: #1606331).
10+
11+ -- Karl Stenerud <karl.stenerud@canonical.com> Mon, 10 Dec 2018 15:08:07 +0100
12+
13 tomcat8 (8.0.32-1ubuntu1.8) xenial-security; urgency=medium
14
15 * SECURITY UPDATE: arbitrary redirect issue
16diff --git a/debian/patches/fix-class-resource-name-filtering.patch b/debian/patches/fix-class-resource-name-filtering.patch
17new file mode 100644
18index 0000000..8bd2744
19--- /dev/null
20+++ b/debian/patches/fix-class-resource-name-filtering.patch
21@@ -0,0 +1,141 @@
22+Description: Fix class and resource name filtering in WebappClassLoader.
23+Origin: upstream, http://svn.apache.org/viewvc?diff_format=h&view=revision&revision=1730178
24+Bug: https://bz.apache.org/bugzilla/show_bug.cgi?id=58999
25+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/tomcat8/+bug/1606331
26+Last-Update: 2018-12-10
27+---
28+This patch header follows DEP-3: http://dep.debian.net/deps/dep3/
29+diff --git a/java/org/apache/catalina/loader/WebappClassLoaderBase.java b/java/org/apache/catalina/loader/WebappClassLoaderBase.java
30+index 06186982..a2952157 100644
31+--- a/java/org/apache/catalina/loader/WebappClassLoaderBase.java
32++++ b/java/org/apache/catalina/loader/WebappClassLoaderBase.java
33+@@ -2753,6 +2753,9 @@ public abstract class WebappClassLoaderBase extends URLClassLoader
34+ char ch;
35+ if (name.startsWith("javax")) {
36+ /* 5 == length("javax") */
37++ if (name.length() == 5) {
38++ return false;
39++ }
40+ ch = name.charAt(5);
41+ if (isClassName && ch == '.') {
42+ /* 6 == length("javax.") */
43+@@ -2777,6 +2780,9 @@ public abstract class WebappClassLoaderBase extends URLClassLoader
44+ }
45+ } else if (name.startsWith("org")) {
46+ /* 3 == length("org") */
47++ if (name.length() == 3) {
48++ return false;
49++ }
50+ ch = name.charAt(3);
51+ if (isClassName && ch == '.') {
52+ /* 4 == length("org.") */
53+diff --git a/test/org/apache/catalina/loader/TestWebappClassLoader.java b/test/org/apache/catalina/loader/TestWebappClassLoader.java
54+index 30160e79..c85f90a8 100644
55+--- a/test/org/apache/catalina/loader/TestWebappClassLoader.java
56++++ b/test/org/apache/catalina/loader/TestWebappClassLoader.java
57+@@ -65,15 +65,17 @@ public class TestWebappClassLoader extends TomcatBaseTest {
58+ public void testFilter() throws IOException {
59+
60+ String[] classSuffixes = new String[]{
61++ "",
62+ "some.package.Example"
63+ };
64+
65+ String[] resourceSuffixes = new String[]{
66++ "",
67+ "some/path/test.properties",
68+ "some/path/test"
69+ };
70+
71+- String[] prefixesPermit = new String[]{
72++ String[] prefixes = new String[]{
73+ "",
74+ "resources",
75+ "WEB-INF",
76+@@ -81,12 +83,15 @@ public class TestWebappClassLoader extends TomcatBaseTest {
77+ "WEB-INF.lib",
78+ "org",
79+ "org.apache",
80+- "org.apache.tomcat.jdbc",
81+ "javax",
82+- "javax.jsp.jstl",
83+ "com.mycorp"
84+ };
85+
86++ String[] prefixesPermit = new String[]{
87++ "org.apache.tomcat.jdbc",
88++ "javax.servlet.jsp.jstl",
89++ };
90++
91+ String[] prefixesDeny = new String[]{
92+ "org.apache.catalina",
93+ "org.apache.coyote",
94+@@ -103,7 +108,7 @@ public class TestWebappClassLoader extends TomcatBaseTest {
95+ try (WebappClassLoader loader = new WebappClassLoader()) {
96+ String name;
97+
98+- for (String prefix : prefixesPermit) {
99++ for (String prefix : prefixes) {
100+ for (String suffix : classSuffixes) {
101+ name = prefix + "." + suffix;
102+ Assert.assertTrue("Class '" + name + "' failed permit filter",
103+@@ -113,6 +118,11 @@ public class TestWebappClassLoader extends TomcatBaseTest {
104+ Assert.assertTrue("Class '" + name + "' failed permit filter",
105+ !loader.filter(name, true));
106+ }
107++ if (suffix.equals("")) {
108++ name = prefix;
109++ Assert.assertTrue("Class '" + name + "' failed permit filter",
110++ !loader.filter(name, true));
111++ }
112+ }
113+ prefix = prefix.replace('.', '/');
114+ for (String suffix : resourceSuffixes) {
115+@@ -124,26 +134,37 @@ public class TestWebappClassLoader extends TomcatBaseTest {
116+ Assert.assertTrue("Resource '" + name + "' failed permit filter",
117+ !loader.filter(name, false));
118+ }
119++ if (suffix.equals("")) {
120++ name = prefix;
121++ Assert.assertTrue("Resource '" + name + "' failed permit filter",
122++ !loader.filter(name, false));
123++ }
124++ }
125++ }
126++
127++ for (String prefix : prefixesPermit) {
128++ for (String suffix : classSuffixes) {
129++ name = prefix + "." + suffix;
130++ Assert.assertTrue("Class '" + name + "' failed permit filter",
131++ !loader.filter(name, true));
132++ }
133++ prefix = prefix.replace('.', '/');
134++ for (String suffix : resourceSuffixes) {
135++ name = prefix + "/" + suffix;
136++ Assert.assertTrue("Resource '" + name + "' failed permit filter",
137++ !loader.filter(name, false));
138+ }
139+ }
140+
141+ for (String prefix : prefixesDeny) {
142+ for (String suffix : classSuffixes) {
143+- if (prefix.equals("")) {
144+- name = suffix;
145+- } else {
146+- name = prefix + "." + suffix;
147+- }
148++ name = prefix + "." + suffix;
149+ Assert.assertTrue("Class '" + name + "' failed deny filter",
150+ loader.filter(name, true));
151+ }
152+ prefix = prefix.replace('.', '/');
153+ for (String suffix : resourceSuffixes) {
154+- if (prefix.equals("")) {
155+- name = suffix;
156+- } else {
157+- name = prefix + "/" + suffix;
158+- }
159++ name = prefix + "/" + suffix;
160+ Assert.assertTrue("Resource '" + name + "' failed deny filter",
161+ loader.filter(name, false));
162+ }
163diff --git a/debian/patches/series b/debian/patches/series
164index de1043b..4fb70b2 100644
165--- a/debian/patches/series
166+++ b/debian/patches/series
167@@ -34,3 +34,4 @@ CVE-2018-8014.patch
168 CVE-2018-1336.patch
169 CVE-2018-8034.patch
170 CVE-2018-11784.patch
171+fix-class-resource-name-filtering.patch

Subscribers

People subscribed via source and target branches