Repository: accumulo Updated Branches: refs/heads/1.6.0-SNAPSHOT af8588e3b -> 9621701fd refs/heads/master 9ef0fca84 -> 7492d7478
ACCUMULO-2720 Address some HTTP response splitting URLEncode some parameters, and do some validation on redirects in the monitor to mitigate HTTP response splitting vulnerabilities identified by FindBugs. Project: http://git-wip-us.apache.org/repos/asf/accumulo/repo Commit: http://git-wip-us.apache.org/repos/asf/accumulo/commit/9621701f Tree: http://git-wip-us.apache.org/repos/asf/accumulo/tree/9621701f Diff: http://git-wip-us.apache.org/repos/asf/accumulo/diff/9621701f Branch: refs/heads/1.6.0-SNAPSHOT Commit: 9621701fd6d930952f82523b52c428dcf89a18dd Parents: af8588e Author: Christopher Tubbs <ctubb...@apache.org> Authored: Wed Apr 23 16:26:30 2014 -0400 Committer: Christopher Tubbs <ctubb...@apache.org> Committed: Wed Apr 23 16:26:30 2014 -0400 ---------------------------------------------------------------------- pom.xml | 2 +- .../accumulo/monitor/servlets/BasicServlet.java | 26 +++++++-- .../monitor/servlets/OperationServlet.java | 52 +++++++++-------- .../org/apache/accumulo/monitor/util/Table.java | 59 ++++++++++---------- 4 files changed, 78 insertions(+), 61 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/accumulo/blob/9621701f/pom.xml ---------------------------------------------------------------------- diff --git a/pom.xml b/pom.xml index 04b4de0..a6aaccd 100644 --- a/pom.xml +++ b/pom.xml @@ -460,7 +460,7 @@ <effort>Max</effort> <failOnError>true</failOnError> <includeTests>true</includeTests> - <maxRank>4</maxRank> + <maxRank>6</maxRank> </configuration> </plugin> <plugin> http://git-wip-us.apache.org/repos/asf/accumulo/blob/9621701f/server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/BasicServlet.java ---------------------------------------------------------------------- diff --git a/server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/BasicServlet.java b/server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/BasicServlet.java index 22728e2..603f55f 100644 --- a/server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/BasicServlet.java +++ b/server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/BasicServlet.java @@ -20,6 +20,7 @@ import java.io.IOException; import java.io.PrintWriter; import java.io.StringWriter; import java.io.UnsupportedEncodingException; +import java.net.URLDecoder; import java.net.URLEncoder; import java.util.Date; import java.util.List; @@ -83,21 +84,25 @@ abstract public class BasicServlet extends HttpServlet { private static final String DEFAULT_CONTENT_TYPE = "text/html"; - public static final Cookie getCookie(HttpServletRequest req, String name) { + public static final void setCookie(HttpServletResponse resp, String name, String value) { + resp.addCookie(new Cookie(name, value)); + } + + public static final String getCookieValue(HttpServletRequest req, String name) { if (req.getCookies() != null) for (Cookie c : req.getCookies()) if (c.getName().equals(name)) - return c; + return c.getValue(); return null; } protected void pageStart(HttpServletRequest req, HttpServletResponse resp, StringBuilder sb) throws Exception { resp.setContentType(DEFAULT_CONTENT_TYPE); int refresh = -1; - Cookie c = getCookie(req, "page.refresh.rate"); - if (c != null && c.getValue() != null) { + String refreshStr = getCookieValue(req, "page.refresh.rate"); + if (refreshStr != null) { try { - refresh = Integer.parseInt(c.getValue()); + refresh = Integer.parseInt(BasicServlet.decode(refreshStr)); } catch (NumberFormatException e) { // ignore improperly formatted user cookie } @@ -246,7 +251,16 @@ abstract public class BasicServlet extends HttpServlet { return URLEncoder.encode(s, Constants.UTF8.name()); } catch (UnsupportedEncodingException e) { Logger.getLogger(BasicServlet.class).fatal(Constants.UTF8.name() + " is not a recognized encoding", e); - throw new RuntimeException(e); + throw new AssertionError(e); // can't happen with UTF-8 + } + } + + public static String decode(String s) { + try { + return URLDecoder.decode(s, Constants.UTF8.name()); + } catch (UnsupportedEncodingException e) { + Logger.getLogger(BasicServlet.class).fatal(Constants.UTF8.name() + " is not a recognized encoding", e); + throw new AssertionError(e); // can't happen with UTF-8 } } http://git-wip-us.apache.org/repos/asf/accumulo/blob/9621701f/server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/OperationServlet.java ---------------------------------------------------------------------- diff --git a/server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/OperationServlet.java b/server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/OperationServlet.java index e403687..c6c75cd 100644 --- a/server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/OperationServlet.java +++ b/server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/OperationServlet.java @@ -18,7 +18,6 @@ package org.apache.accumulo.monitor.servlets; import java.io.IOException; -import javax.servlet.http.Cookie; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -33,21 +32,21 @@ import org.apache.accumulo.server.problems.ProblemType; import org.apache.log4j.Logger; public class OperationServlet extends BasicServlet { - + private static final long serialVersionUID = 1L; - + @Override protected String getTitle(HttpServletRequest req) { return "Operations"; } - + @Override public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException { String redir = null; try { String operation = req.getParameter("action"); redir = req.getParameter("redir"); - + if (operation != null) { for (Class<?> subclass : OperationServlet.class.getClasses()) { Object t; @@ -69,36 +68,35 @@ public class OperationServlet extends BasicServlet { log.error(t, t); } finally { try { - if (redir != null) - resp.sendRedirect(redir); - else - resp.sendRedirect("/"); + redir = redir == null ? "" : redir; + redir = redir.startsWith("/") ? redir.substring(1) : redir; + resp.sendRedirect("/" + redir); // this makes findbugs happy resp.flushBuffer(); } catch (Throwable t) { log.error(t, t); } } } - + private interface WebOperation { void execute(HttpServletRequest req, HttpServletResponse resp, Logger log) throws Exception; } - + public static class RefreshOperation implements WebOperation { @Override public void execute(HttpServletRequest req, HttpServletResponse resp, Logger log) { String value = req.getParameter("value"); - resp.addCookie(new Cookie("page.refresh.rate", value == null ? "5" : value)); + BasicServlet.setCookie(resp, "page.refresh.rate", value == null ? "5" : BasicServlet.encode(value)); } } - + public static class ClearLogOperation implements WebOperation { @Override public void execute(HttpServletRequest req, HttpServletResponse resp, Logger log) { LogService.getInstance().clear(); } } - + public static class ClearTableProblemsOperation implements WebOperation { @Override public void execute(HttpServletRequest req, HttpServletResponse resp, Logger log) { @@ -110,7 +108,7 @@ public class OperationServlet extends BasicServlet { } } } - + public static class ClearProblemOperation implements WebOperation { @Override public void execute(HttpServletRequest req, HttpServletResponse resp, Logger log) { @@ -124,7 +122,7 @@ public class OperationServlet extends BasicServlet { } } } - + public static class SortTableOperation implements WebOperation { @Override public void execute(HttpServletRequest req, HttpServletResponse resp, Logger log) throws IOException { @@ -134,13 +132,18 @@ public class OperationServlet extends BasicServlet { String col = req.getParameter("col"); if (table == null || page == null || (asc == null && col == null)) return; - if (asc == null) - resp.addCookie(new Cookie("tableSort." + page + "." + table + "." + "sortCol", col)); - else - resp.addCookie(new Cookie("tableSort." + page + "." + table + "." + "sortAsc", asc)); + page = BasicServlet.encode(page); + table = BasicServlet.encode(table); + if (asc == null) { + col = BasicServlet.encode(col); + BasicServlet.setCookie(resp, "tableSort." + page + "." + table + "." + "sortCol", col); + } else { + asc = BasicServlet.encode(asc); + BasicServlet.setCookie(resp, "tableSort." + page + "." + table + "." + "sortAsc", asc); + } } } - + public static class ToggleLegendOperation implements WebOperation { @Override public void execute(HttpServletRequest req, HttpServletResponse resp, Logger log) throws Exception { @@ -149,10 +152,13 @@ public class OperationServlet extends BasicServlet { String show = req.getParameter("show"); if (table == null || page == null || show == null) return; - resp.addCookie(new Cookie("tableLegend." + page + "." + table + "." + "show", show)); + page = BasicServlet.encode(page); + table = BasicServlet.encode(table); + show = BasicServlet.encode(show); + BasicServlet.setCookie(resp, "tableLegend." + page + "." + table + "." + "show", show); } } - + public static class ClearDeadServerOperation implements WebOperation { @Override public void execute(HttpServletRequest req, HttpServletResponse resp, Logger log) { http://git-wip-us.apache.org/repos/asf/accumulo/blob/9621701f/server/monitor/src/main/java/org/apache/accumulo/monitor/util/Table.java ---------------------------------------------------------------------- diff --git a/server/monitor/src/main/java/org/apache/accumulo/monitor/util/Table.java b/server/monitor/src/main/java/org/apache/accumulo/monitor/util/Table.java index e2578c9..b1a4582 100644 --- a/server/monitor/src/main/java/org/apache/accumulo/monitor/util/Table.java +++ b/server/monitor/src/main/java/org/apache/accumulo/monitor/util/Table.java @@ -19,7 +19,6 @@ package org.apache.accumulo.monitor.util; import java.util.ArrayList; import java.util.Collections; -import javax.servlet.http.Cookie; import javax.servlet.http.HttpServletRequest; import org.apache.accumulo.monitor.servlets.BasicServlet; @@ -34,11 +33,11 @@ public class Table { private ArrayList<TableColumn<?>> columns; private ArrayList<TableRow> rows; private boolean hasBegunAddingRows = false; - + public Table(String tableName, String caption) { this(tableName, caption, null); } - + public Table(String tableName, String caption, String captionClass) { this.table = tableName; this.caption = caption; @@ -47,52 +46,52 @@ public class Table { this.columns = new ArrayList<TableColumn<?>>(); this.rows = new ArrayList<TableRow>(); } - + public synchronized void setSubCaption(String subcaption) { this.subcaption = subcaption; } - + public synchronized <T> void addColumn(TableColumn<T> column) { if (hasBegunAddingRows) throw new IllegalStateException("Cannot add more columns newServer rows have been added"); columns.add(column); } - + private synchronized <T> void addColumn(String title, CellType<T> type, String legend, boolean sortable) { if (type == null) type = new StringType<T>(); type.setSortable(sortable); addColumn(new TableColumn<T>(title, type, legend)); } - + public synchronized <T> void addUnsortableColumn(String title, CellType<T> type, String legend) { addColumn(title, type, legend, false); } - + public synchronized <T> void addSortableColumn(String title, CellType<T> type, String legend) { addColumn(title, type, legend, true); } - + public synchronized void addUnsortableColumn(String title) { addUnsortableColumn(title, null, null); } - + public synchronized void addSortableColumn(String title) { addSortableColumn(title, null, null); } - + public synchronized TableRow prepareRow() { hasBegunAddingRows = true; return new TableRow(columns.size()); } - + public synchronized void addRow(TableRow row) { hasBegunAddingRows = true; if (columns.size() != row.size()) throw new IllegalStateException("Row must be the same size as the columns"); rows.add(row); } - + public synchronized void addRow(Object... cells) { TableRow row = prepareRow(); if (cells.length != columns.size()) @@ -101,7 +100,7 @@ public class Table { row.add(cell); addRow(row); } - + public synchronized void generate(HttpServletRequest req, StringBuilder sb) { String page = req.getRequestURI(); if (columns.isEmpty()) @@ -109,12 +108,10 @@ public class Table { for (TableRow row : rows) if (row.size() != columns.size()) throw new RuntimeException("Each row must have the same number of columns"); - - boolean sortAscending = true; - Cookie c = BasicServlet.getCookie(req, "tableSort." + page + "." + table + "." + "sortAsc"); - if (c != null && "false".equals(c.getValue())) - sortAscending = false; - + + boolean sortAscending = !"false".equals(BasicServlet.getCookieValue(req, "tableSort." + BasicServlet.encode(page) + "." + BasicServlet.encode(table) + "." + + "sortAsc")); + int sortCol = -1; // set to first sortable column by default int numLegends = 0; for (int i = 0; i < columns.size(); ++i) { @@ -124,13 +121,13 @@ public class Table { if (col.getLegend() != null && !col.getLegend().isEmpty()) ++numLegends; } - + // only get cookie if there is a possibility that it is sortable if (sortCol >= 0) { - c = BasicServlet.getCookie(req, "tableSort." + page + "." + table + "." + "sortCol"); - if (c != null && c.getValue() != null) { + String sortColStr = BasicServlet.getCookieValue(req, "tableSort." + BasicServlet.encode(page) + "." + BasicServlet.encode(table) + "." + "sortCol"); + if (sortColStr != null) { try { - int col = Integer.parseInt(c.getValue()); + int col = Integer.parseInt(sortColStr); // only bother if specified column is sortable if (!(col < 0 || sortCol >= columns.size()) && columns.get(col).getCellType().isSortable()) sortCol = col; @@ -139,13 +136,13 @@ public class Table { } } } - + boolean showLegend = false; if (numLegends > 0) { - c = BasicServlet.getCookie(req, "tableLegend." + page + "." + table + "." + "show"); - showLegend = c != null && Boolean.parseBoolean(c.getValue()); + String showStr = BasicServlet.getCookieValue(req, "tableLegend." + BasicServlet.encode(page) + "." + BasicServlet.encode(table) + "." + "show"); + showLegend = showStr != null && Boolean.parseBoolean(showStr); } - + sb.append("<div>\n"); sb.append("<a name='").append(table).append("'> </a>\n"); sb.append("<table id='").append(table).append("' class='sortable'>\n"); @@ -157,7 +154,7 @@ public class Table { sb.append("<span class='table-caption'>").append(caption).append("</span><br />\n"); if (subcaption != null && !subcaption.isEmpty()) sb.append("<span class='table-subcaption'>").append(subcaption).append("</span><br />\n"); - + String redir = BasicServlet.currentPage(req); if (numLegends > 0) { String legendUrl = String.format("/op?action=toggleLegend&redir=%s&page=%s&table=%s&show=%s", redir, page, table, !showLegend); @@ -213,7 +210,7 @@ public class Table { sb.append("<tr><td class='center' colspan='").append(columns.size()).append("'><i>Empty</i></td></tr>\n"); sb.append("</table>\n</div>\n\n"); } - + private static void row(StringBuilder sb, boolean highlight, ArrayList<TableColumn<?>> columns, TableRow row) { sb.append(highlight ? "<tr class='highlight'>" : "<tr>"); boolean first = true; @@ -229,5 +226,5 @@ public class Table { } sb.append("</tr>\n"); } - + }
