accumulo-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ctubb...@apache.org
Subject [2/3] git commit: ACCUMULO-2720 Address some HTTP response splitting
Date Wed, 23 Apr 2014 21:00:12 GMT
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/master
Commit: 9621701fd6d930952f82523b52c428dcf89a18dd
Parents: af8588e
Author: Christopher Tubbs <ctubbsii@apache.org>
Authored: Wed Apr 23 16:26:30 2014 -0400
Committer: Christopher Tubbs <ctubbsii@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("'>&nbsp;</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");
   }
-  
+
 }


Mime
View raw message