azkaban-aplcache

Disallowing users from logging in via URI QueryString (#1252) Related

6/27/2017 7:04:20 PM

Details

diff --git a/azkaban-web-server/src/main/java/azkaban/webapp/servlet/LoginAbstractAzkabanServlet.java b/azkaban-web-server/src/main/java/azkaban/webapp/servlet/LoginAbstractAzkabanServlet.java
index 3e8d743..8f07fab 100644
--- a/azkaban-web-server/src/main/java/azkaban/webapp/servlet/LoginAbstractAzkabanServlet.java
+++ b/azkaban-web-server/src/main/java/azkaban/webapp/servlet/LoginAbstractAzkabanServlet.java
@@ -148,7 +148,7 @@ public abstract class LoginAbstractAzkabanServlet extends
     buf.append("\"");
     buf.append(req.getMethod()).append(" ");
     buf.append(req.getRequestURI()).append(" ");
-    if (req.getQueryString() != null) {
+    if (req.getQueryString() != null && !isIllegalPostRequest(req)) {
       buf.append(req.getQueryString()).append(" ");
     } else {
       buf.append("-").append(" ");
@@ -277,6 +277,10 @@ public abstract class LoginAbstractAzkabanServlet extends
     Session session = getSessionFromRequest(req);
     this.webMetrics.markWebPostCall();
     logRequest(req, session);
+    if (isIllegalPostRequest(req)) {
+      writeResponse(resp, "Login error. Must pass username and password in request body");
+      return;
+    }
 
     // Handle Multipart differently from other post messages
     if (ServletFileUpload.isMultipartContent(req)) {
@@ -345,6 +349,25 @@ public abstract class LoginAbstractAzkabanServlet extends
     }
   }
 
+  /**
+   * Disallows users from logging in by passing their username and password via the request header
+   * where it'd be logged.
+   *
+   * Example of illegal post request:
+   * curl -X POST http://localhost:8081/?action=login\&username=azkaban\&password=azkaban
+   *
+   * req.getParameterMap() or req.getParameterNames() cannot be used because they draw no
+   * distinction between the illegal request above and the following valid request:
+   * curl -X POST -d "action=login&username=azkaban&password=azkaban" http://localhost:8081/
+   *
+   * "password=" is searched for because it leverages the query syntax to determine that the user is
+   * passing the password as a parameter name. There is no other ajax call that has a parameter
+   * that includes the string "password" at the end which could throw false positives.
+   */
+  private boolean isIllegalPostRequest(final HttpServletRequest req) {
+    return (req.getQueryString() != null && req.getQueryString().contains("password="));
+  }
+
   private Session createSession(final HttpServletRequest req)
       throws UserManagerException, ServletException {
     final String username = getParam(req, "username");
diff --git a/azkaban-web-server/src/test/java/azkaban/webapp/servlet/LoginAbstractAzkabanServletTest.java b/azkaban-web-server/src/test/java/azkaban/webapp/servlet/LoginAbstractAzkabanServletTest.java
index 57ec350..d50de73 100644
--- a/azkaban-web-server/src/test/java/azkaban/webapp/servlet/LoginAbstractAzkabanServletTest.java
+++ b/azkaban-web-server/src/test/java/azkaban/webapp/servlet/LoginAbstractAzkabanServletTest.java
@@ -27,6 +27,7 @@ import azkaban.fixture.MockLoginAzkabanServlet;
 import java.io.IOException;
 import java.io.PrintWriter;
 import java.io.StringWriter;
+import java.util.HashMap;
 import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
@@ -182,4 +183,39 @@ public class LoginAbstractAzkabanServletTest {
     // Assert that our response was written (we have a valid session)
     assertEquals("SUCCESS_MOCK_LOGIN_SERVLET", writer.toString());
   }
+
+  /**
+   * Simulates users passing username/password via URI
+   * where it would be logged by Azkaban Web Server
+   */
+  @Test
+  public void testLoginRevealingCredentialsShouldThrowFailure() throws Exception {
+
+    final String clientIp = "127.0.0.1:10000";
+    final String sessionId = "111";
+    final String queryString = "action=login&username=azkaban&password=azkaban";
+    final String[] mockCredentials = {"azkaban"};
+    final HashMap<String, String[]> mockParameterMap = new HashMap<String, String[]>() {
+      {
+        put("username", mockCredentials);
+        put("password", mockCredentials);
+      }
+    };
+
+    final HttpServletRequest req = MockLoginAzkabanServlet
+        .getRequestWithNoUpstream(clientIp, sessionId, "POST");
+    when(req.getParameterMap()).thenReturn(mockParameterMap);
+    when(req.getQueryString()).thenReturn(queryString);
+    final StringWriter writer = new StringWriter();
+    final HttpServletResponse resp = getResponse(writer);
+
+    final MockLoginAzkabanServlet servlet = MockLoginAzkabanServlet.getServletWithSession(sessionId,
+        "user", "127.0.0.1");
+
+    servlet.doPost(req, resp);
+
+    // Assert that expected error message is returned
+    assertEquals("Login error. Must pass username and password in request body", writer.toString());
+
+  }
 }