From b1c50187d4eef764ade2cc03eac2f638bc2d910c Mon Sep 17 00:00:00 2001 From: "burke.davey" Date: Mon, 19 Jul 2010 00:15:37 +0000 Subject: [PATCH] XSS protection in signin / signout. XSRF protection for register / unregister. Support only v3 and upwards. Rev to version 4. --- .../chrometophone/server/AuthServlet.java | 60 +++++++------------ .../chrometophone/server/RegisterServlet.java | 21 +++++-- .../chrometophone/server/SendServlet.java | 59 ++++++++---------- .../server/UnregisterServlet.java | 11 +++- appengine/war/WEB-INF/appengine-web.xml | 2 +- 5 files changed, 73 insertions(+), 80 deletions(-) diff --git a/appengine/src/com/google/android/chrometophone/server/AuthServlet.java b/appengine/src/com/google/android/chrometophone/server/AuthServlet.java index 5fe35de..f27d230 100644 --- a/appengine/src/com/google/android/chrometophone/server/AuthServlet.java +++ b/appengine/src/com/google/android/chrometophone/server/AuthServlet.java @@ -18,6 +18,7 @@ package com.google.android.chrometophone.server; import java.io.IOException; import java.net.URLEncoder; +import java.util.logging.Logger; import javax.servlet.http.HttpServlet; import javax.servlet.http.HttpServletRequest; @@ -28,61 +29,46 @@ import com.google.appengine.api.users.UserServiceFactory; @SuppressWarnings("serial") public class AuthServlet extends HttpServlet { + private static final Logger log = + Logger.getLogger(SendServlet.class.getName()); private static final String ERROR_STATUS = "ERROR"; + @Override public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException { - if (req.getRequestURI().startsWith("/signin")) { - doSignIn(req, resp); - } else if (req.getRequestURI().startsWith("/signout")) { - doSignOut(req, resp); - } - } + resp.setContentType("text/html"); + boolean signIn = req.getRequestURI().startsWith("/signin"); - private void doSignIn(HttpServletRequest req, HttpServletResponse resp) throws IOException { // Get the extension return URL String extRet = req.getParameter("extret"); if (extRet == null) { - resp.setContentType("text/plain"); + resp.setStatus(400); resp.getWriter().println(ERROR_STATUS + " (extret parameter missing)"); return; } - // If login is complete, redirect to the extension page. Otherwise, send user to login, - // setting the continue page back to this servlet (since UserService does not understand - // chrome-extension:// URLs + // If login/logout is complete, redirect to the extension page. Otherwise, send user to + // login/logout, setting the continue page back to this servlet (since UserService does + // not understand chrome-extension:// URLs) if (req.getParameter("completed") != null) { // Server-side redirects don't work for chrome-extension:// URLs so we do a client- // side redirect instead - resp.getWriter().println(""); + + // Sanitize the extRet URL for XSS protection + String regEx = "chrome-extension://[a-z]+" + + (signIn ? "/signed_in\\.html" : "/signed_out\\.html"); + if (extRet.matches(regEx)) { + resp.getWriter().println(""); + } else { + resp.setStatus(400); + resp.getWriter().println(ERROR_STATUS + " (invalid redirect)"); + log.warning("Invalid redirect " + extRet); + } } else { String followOnURL = req.getRequestURI() + "?completed=true&extret=" + URLEncoder.encode(extRet, "UTF-8"); UserService userService = UserServiceFactory.getUserService(); - resp.sendRedirect(userService.createLoginURL(followOnURL)); - } - } - - private void doSignOut(HttpServletRequest req, HttpServletResponse resp) throws IOException { - // Get the extension return URL - String extRet = req.getParameter("extret"); - if (extRet == null) { - resp.setContentType("text/plain"); - resp.getWriter().println(ERROR_STATUS + " (extret parameter missing)"); - return; - } - - // If logout is complete, redirect to the extension page. Otherwise, send user to login, - // setting the continue page back to this servlet (since UserService does not understand - // chrome-extension:// URLs - if (req.getParameter("completed") != null) { - // Server-side redirects don't work for chrome-extension:// URLs so we do a client- - // side redirect instead - resp.getWriter().println(""); - } else { - String followOnURL = req.getRequestURI() + "?completed=true&extret=" + - URLEncoder.encode(extRet, "UTF-8"); - UserService userService = UserServiceFactory.getUserService(); - resp.sendRedirect(userService.createLogoutURL(followOnURL)); + resp.sendRedirect(signIn ? userService.createLoginURL(followOnURL) : + userService.createLogoutURL(followOnURL)); } } } diff --git a/appengine/src/com/google/android/chrometophone/server/RegisterServlet.java b/appengine/src/com/google/android/chrometophone/server/RegisterServlet.java index 5491368..1406e02 100644 --- a/appengine/src/com/google/android/chrometophone/server/RegisterServlet.java +++ b/appengine/src/com/google/android/chrometophone/server/RegisterServlet.java @@ -42,11 +42,11 @@ public class RegisterServlet extends HttpServlet { /** * Get the user using the UserService. - * + * * If not logged in, return an error message. - * - * @return user, or null if not logged in. - * @throws IOException + * + * @return user, or null if not logged in. + * @throws IOException */ static User checkUser(HttpServletRequest req, HttpServletResponse resp, boolean errorIfNotLoggedIn) throws IOException { @@ -62,7 +62,7 @@ public class RegisterServlet extends HttpServlet { } catch (Throwable t) { user = null; } - + UserService userService = UserServiceFactory.getUserService(); user = userService.getCurrentUser(); if (user == null && errorIfNotLoggedIn) { @@ -73,10 +73,11 @@ public class RegisterServlet extends HttpServlet { } return user; } - + /** * @deprecated will be removed in next rel. */ + @Deprecated @Override public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException { doPost(req, resp); @@ -86,6 +87,14 @@ public class RegisterServlet extends HttpServlet { public void doPost(HttpServletRequest req, HttpServletResponse resp) throws IOException { resp.setContentType("text/plain"); + // Basic XSRF protection + if (req.getHeader("X-Same-Domain") == null) { + // TODO: Enable at consumer launch + //resp.setStatus(400); + //resp.getWriter().println(ERROR_STATUS + " (Missing X-Same-Domain header)"); + //return; + } + String deviceRegistrationID = req.getParameter("devregid"); if (deviceRegistrationID == null) { resp.setStatus(400); diff --git a/appengine/src/com/google/android/chrometophone/server/SendServlet.java b/appengine/src/com/google/android/chrometophone/server/SendServlet.java index b9b35b8..6ea9d9c 100644 --- a/appengine/src/com/google/android/chrometophone/server/SendServlet.java +++ b/appengine/src/com/google/android/chrometophone/server/SendServlet.java @@ -17,7 +17,6 @@ package com.google.android.chrometophone.server; import java.io.IOException; -import java.net.URLEncoder; import java.util.logging.Logger; import javax.jdo.JDOObjectNotFoundException; @@ -30,8 +29,6 @@ import com.google.android.c2dm.server.C2DMessaging; import com.google.appengine.api.datastore.Key; import com.google.appengine.api.datastore.KeyFactory; import com.google.appengine.api.users.User; -import com.google.appengine.api.users.UserService; -import com.google.appengine.api.users.UserServiceFactory; @SuppressWarnings("serial") public class SendServlet extends HttpServlet { @@ -51,19 +48,25 @@ public class SendServlet extends HttpServlet { public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException { resp.setContentType("text/plain"); - // Basic XSRF protection - if (req.getHeader("X-Extension") == null) { - resp.setStatus(400); - resp.getWriter().println(ERROR_STATUS + " - Please upgrade your extension"); - log.warning("Missing X-Extension header"); - resp.setStatus(400); - return; - } - + // Check API version String apiVersionString = req.getParameter("ver"); if (apiVersionString == null) apiVersionString = "1"; int apiVersion = Integer.parseInt(apiVersionString); log.info("Extension version: " + apiVersion); + if (apiVersion < 3) { + resp.setStatus(400); + resp.getWriter().println(ERROR_STATUS + + " (Please remove old Chrome extension and install latest)"); + return; + } + + // Basic XSRF protection (TODO: remove X-Extension in a future release for consistency) + if (req.getHeader("X-Same-Domain") == null && req.getHeader("X-Extension") == null) { + resp.setStatus(400); + resp.getWriter().println(ERROR_STATUS + " (Missing header)"); + log.warning("Missing header"); + return; + } String sel = req.getParameter("sel"); if (sel == null) sel = ""; // optional @@ -78,23 +81,14 @@ public class SendServlet extends HttpServlet { User user = RegisterServlet.checkUser(req, resp, false); if (user != null) { - doSendToPhone(url, title, sel, user.getEmail(), apiVersion, resp); + doSendToPhone(url, title, sel, user.getEmail(), resp); } else { - if (apiVersion >= 2) { // TODO: Make this default code path on launch - resp.getWriter().println(LOGIN_REQUIRED_STATUS); - } else { // TODO: DEPRECATED code path. Delete on launch - String followOnURL = req.getRequestURI() + "?title=" + - URLEncoder.encode(title, "UTF-8") + - "&url=" + URLEncoder.encode(url, "UTF-8") + - "&sel=" + URLEncoder.encode(sel, "UTF-8"); - UserService userService = UserServiceFactory.getUserService(); - resp.sendRedirect(userService.createLoginURL(followOnURL)); - } + resp.getWriter().println(LOGIN_REQUIRED_STATUS); } } private boolean doSendToPhone(String url, String title, String sel, - String userAccount, int apiVersion, HttpServletResponse resp) throws IOException { + String userAccount, HttpServletResponse resp) throws IOException { // Get device info DeviceInfo deviceInfo = null; // Shared PMF @@ -106,12 +100,7 @@ public class SendServlet extends HttpServlet { deviceInfo = pm.getObjectById(DeviceInfo.class, key); } catch (JDOObjectNotFoundException e) { log.warning("Device not registered"); - if (apiVersion >= 3) { // TODO: Make this default code path on launch - resp.getWriter().println(DEVICE_NOT_REGISTERED_STATUS); - } else { // TODO: DEPRECATED code path. Delete on launch - resp.setStatus(400); - resp.getWriter().println(ERROR_STATUS + " (Device not registered)"); - } + resp.getWriter().println(DEVICE_NOT_REGISTERED_STATUS); return false; } } finally { @@ -124,16 +113,16 @@ public class SendServlet extends HttpServlet { String collapseKey = "" + url.hashCode(); if (deviceInfo.getDebug()) { res = push.sendNoRetry(deviceInfo.getDeviceRegistrationID(), - collapseKey, - "url", url, + collapseKey, + "url", url, "title", title, "sel", sel, "debug", "1"); - + } else { res = push.sendNoRetry(deviceInfo.getDeviceRegistrationID(), - collapseKey, - "url", url, + collapseKey, + "url", url, "title", title, "sel", sel); } diff --git a/appengine/src/com/google/android/chrometophone/server/UnregisterServlet.java b/appengine/src/com/google/android/chrometophone/server/UnregisterServlet.java index 39b7907..770f5c3 100644 --- a/appengine/src/com/google/android/chrometophone/server/UnregisterServlet.java +++ b/appengine/src/com/google/android/chrometophone/server/UnregisterServlet.java @@ -42,15 +42,24 @@ public class UnregisterServlet extends HttpServlet { /** * @deprecated Will be removed in next rel cycle. */ + @Deprecated @Override public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException { doPost(req, resp); } - + @Override public void doPost(HttpServletRequest req, HttpServletResponse resp) throws IOException { resp.setContentType("text/plain"); + // Basic XSRF protection + if (req.getHeader("X-Same-Domain") == null) { + // TODO: Enable at consumer launch + //resp.setStatus(400); + //resp.getWriter().println(ERROR_STATUS + " (Missing X-Same-Domain header)"); + //return; + } + String deviceRegistrationID = req.getParameter("devregid"); if (deviceRegistrationID == null) { resp.setStatus(400); diff --git a/appengine/war/WEB-INF/appengine-web.xml b/appengine/war/WEB-INF/appengine-web.xml index b5cf2cd..5489cba 100644 --- a/appengine/war/WEB-INF/appengine-web.xml +++ b/appengine/war/WEB-INF/appengine-web.xml @@ -16,7 +16,7 @@ --> chrometophone - 3 + 4