authhelper: auth detection rewrite

Signed-off-by: Simon Bennetts <psiinon@gmail.com>

Author: psiinon

addOns/authhelper/CHANGELOG.md

@@ -13,6 +13,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 - Add any session related cookies which are not being tracked.
 - Ignore non proxied requests in auth tester diagnostics.
 - Replace credentials with special tokens.
+- Rewrite of the auth request detection code to handle more cases.
+- Add domain to context if creds posted to it and using using auto-detect for session management.
 
 ### Fixed
 - Bug where some of the data structures were not being reset when the session changed.

addOns/authhelper/src/main/java/org/zaproxy/addon/authhelper/AuthUtils.java

@@ -796,7 +796,9 @@ public static List<Pair<String, String>> getHeaderTokens(
             }
             if (incCookies) {
                 for (HttpCookie cookie : msg.getRequestHeader().getHttpCookies()) {
-                    if (cookie.getValue().contains(token.getValue())) {
+                    if (!(SessionToken.COOKIE_SOURCE.equals(token.getSource())
+                                    && cookie.getName().equals(token.getKey()))
+                            && cookie.getValue().contains(token.getValue())) {
                         String hv =
                                 cookie.getValue()
                                         .replace(token.getValue(), "{%" + token.getToken() + "%}");

addOns/authhelper/src/main/java/org/zaproxy/addon/authhelper/BrowserBasedAuthenticationMethodType.java

@@ -140,10 +140,10 @@ public BrowserBasedAuthenticationMethodType(HttpSender httpSender) {
         this.httpSender = httpSender;
     }
 
-    private Server getProxy(Context context) {
+    private Server getProxy(User user) {
         if (proxy == null) {
             ExtensionNetwork extNet = AuthUtils.getExtension(ExtensionNetwork.class);
-            handler = new ClientSideHandler(context);
+            handler = new ClientSideHandler(user);
             proxy =
                     extNet.createHttpServer(
                             HttpServerConfig.builder()
@@ -314,7 +314,7 @@ private WebSession authenticateImpl(
                             .getExtension(ExtensionSelenium.class);
 
             try {
-                proxyPort = getProxy(user.getContext()).start(proxyHost, 0);
+                proxyPort = getProxy(user).start(proxyHost, 0);
 
                 WebDriver wd = null;
                 try {

addOns/authhelper/src/main/java/org/zaproxy/addon/authhelper/ClientScriptBasedAuthenticationMethodType.java

@@ -88,9 +88,9 @@ public class ClientScriptBasedAuthenticationMethodType extends ScriptBasedAuthen
 
     public ClientScriptBasedAuthenticationMethodType() {}
 
-    private HttpMessageHandler getHandler(Context context) {
+    private HttpMessageHandler getHandler(User user) {
         if (handler == null) {
-            handler = new ClientSideHandler(context);
+            handler = new ClientSideHandler(user);
         }
         return handler;
     }
@@ -334,7 +334,7 @@ public WebSession authenticate(
                 }
 
                 if (authScript instanceof ZestAuthenticationRunner zestRunner) {
-                    zestRunner.registerHandler(getHandler(user.getContext()));
+                    zestRunner.registerHandler(getHandler(user));
                     appendCloseStatements(zestRunner.getScript().getZestScript());
                 } else {
                     LOGGER.warn("Expected authScript to be a Zest script");

addOns/authhelper/src/main/java/org/zaproxy/addon/authhelper/internal/ClientSideHandler.java

@@ -20,38 +20,52 @@
 package org.zaproxy.addon.authhelper.internal;
 
 import java.util.ArrayList;
-import java.util.Map;
+import java.util.Collection;
+import java.util.HashSet;
+import java.util.List;
 import java.util.Set;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import lombok.Getter;
 import lombok.Setter;
-import org.apache.logging.log4j.Level;
+import org.apache.commons.httpclient.URIException;
+import org.apache.commons.lang3.StringUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
-import org.parosproxy.paros.Constant;
-import org.parosproxy.paros.core.scanner.Alert;
+import org.parosproxy.paros.network.HttpHeader;
 import org.parosproxy.paros.network.HttpMessage;
 import org.parosproxy.paros.network.HttpRequestHeader;
-import org.parosproxy.paros.view.View;
+import org.parosproxy.paros.network.HttpStatusCode;
 import org.zaproxy.addon.authhelper.AuthUtils;
+import org.zaproxy.addon.authhelper.HeaderBasedSessionManagementMethodType;
 import org.zaproxy.addon.authhelper.HistoryProvider;
 import org.zaproxy.addon.authhelper.SessionManagementRequestDetails;
 import org.zaproxy.addon.authhelper.SessionToken;
 import org.zaproxy.addon.network.server.HttpMessageHandler;
 import org.zaproxy.addon.network.server.HttpMessageHandlerContext;
-import org.zaproxy.zap.model.Context;
+import org.zaproxy.zap.authentication.UsernamePasswordAuthenticationCredentials;
+import org.zaproxy.zap.model.SessionStructure;
+import org.zaproxy.zap.users.User;
+import org.zaproxy.zap.utils.Pair;
 
 public final class ClientSideHandler implements HttpMessageHandler {
 
     private static final Logger LOGGER = LogManager.getLogger(ClientSideHandler.class);
 
-    private final Context context;
-    private HttpMessage authMsg;
+    private final User user;
+    private UsernamePasswordAuthenticationCredentials authCreds;
+    private AuthRequestDetails authReq;
     private HttpMessage fallbackMsg;
     private int firstHrefId;
 
     @Setter private HistoryProvider historyProvider = new HistoryProvider();
 
-    public ClientSideHandler(Context context) {
-        this.context = context;
+    public ClientSideHandler(User user) {
+        this.user = user;
+        if (user.getAuthenticationCredentials()
+                instanceof UsernamePasswordAuthenticationCredentials authCreds) {
+            this.authCreds = authCreds;
+        }
     }
 
     private boolean isPost(HttpMessage msg) {
@@ -64,81 +78,209 @@ public void handleMessage(HttpMessageHandlerContext ctx, HttpMessage msg) {
         if (ctx.isFromClient()) {
             return;
         }
+        if (firstHrefId == 0 && msg.getHistoryRef() != null) {
+            // Backstop for looping back through the history
+            firstHrefId = msg.getHistoryRef().getHistoryId();
+        }
 
         historyProvider.addAuthMessageToHistory(msg);
 
-        if (!context.isIncluded(msg.getRequestHeader().getURI().toString())) {
-            return;
+        if (!user.getContext().isIncluded(msg.getRequestHeader().getURI().toString())) {
+            String reqBody = msg.getRequestBody().toString();
+            if (isPost(msg)
+                    && authCreds != null
+                    && StringUtils.isNotEmpty(authCreds.getUsername())
+                    && StringUtils.isNotEmpty(authCreds.getPassword())
+                    && reqBody.contains(authCreds.getUsername())
+                    && reqBody.contains(authCreds.getPassword())
+                    && AuthUtils.getSessionManagementDetailsForContext(user.getContext().getId())
+                            != null) {
+                // The app is sending user creds to another site. Assume this is part of the valid
+                // auth flow and add to the context
+                try {
+                    user.getContext()
+                            .addIncludeInContextRegex(SessionStructure.getHostName(msg) + ".*");
+                } catch (URIException e) {
+                    // Very unexpected
+                    LOGGER.error(e.getMessage(), e);
+                    return;
+                }
+            } else {
+                // Not in the context, no creds, not relevant
+                return;
+            }
         }
+        AuthRequestDetails candidate = new AuthRequestDetails(msg);
 
-        if (isPost(msg)) {
-            // Record the last in scope POST as a fallback
-            fallbackMsg = msg;
+        List<Pair<String, String>> headerConfigs = null;
+
+        if (user.getContext().getSessionManagementMethod()
+                instanceof
+                HeaderBasedSessionManagementMethodType.HeaderBasedSessionManagementMethod smgmt) {
+            headerConfigs = smgmt.getHeaderConfigs();
         }
 
-        if (authMsg != null && isPost(authMsg) && !isPost(msg)) {
-            // We have a better candidate
-            return;
+        if (candidate.isBetterThan(authReq, headerConfigs)) {
+            LOGGER.debug(
+                    "Found better auth candidate {} {}",
+                    msg.getRequestHeader().getMethod(),
+                    msg.getRequestHeader().getURI());
+            authReq = candidate;
         }
+
         Set<SessionToken> reqSessionTokens = AuthUtils.getRequestSessionTokens(msg);
+        Set<SessionToken> unkSessionTokens = new HashSet<>();
         for (SessionToken token : reqSessionTokens) {
             if (!SessionToken.COOKIE_SOURCE.equals(token.getSource())) {
-                AuthUtils.recordRequestSessionToken(context, token.getKey(), token.getValue());
+                AuthUtils.recordRequestSessionToken(
+                        user.getContext(), token.getKey(), token.getValue());
+            }
+            if (AuthUtils.containsSessionToken(token.getValue()) == null) {
+                unkSessionTokens.add(token);
             }
         }
-
-        SessionManagementRequestDetails smReqDetails = null;
-        Map<String, SessionToken> sessionTokens = AuthUtils.getResponseSessionTokens(msg);
-        if (!sessionTokens.isEmpty()) {
-            authMsg = msg;
-            LOGGER.debug("Session token found in href {} {}", getHrefId(authMsg), isPost(msg));
-            smReqDetails =
-                    new SessionManagementRequestDetails(
-                            authMsg,
-                            new ArrayList<>(sessionTokens.values()),
-                            Alert.CONFIDENCE_HIGH);
-        } else {
-            if (!reqSessionTokens.isEmpty()) {
-                // The request has at least one auth token we missed - try
-                // to find one of them
-                for (SessionToken st : reqSessionTokens) {
-                    smReqDetails = AuthUtils.findSessionTokenSource(st.getValue(), firstHrefId);
-                    if (smReqDetails != null) {
-                        authMsg = smReqDetails.getMsg();
-                        LOGGER.debug("Session token found in href {}", getHrefId(authMsg));
-                        break;
-                    }
+        for (SessionToken st : unkSessionTokens) {
+            // See if we can find the reqs for the unknown session tokens, then see if they are
+            // better than the current one
+            SessionManagementRequestDetails smReqDetails =
+                    AuthUtils.findSessionTokenSource(st.getValue(), firstHrefId);
+            if (smReqDetails != null) {
+                candidate = new AuthRequestDetails(msg);
+                if (candidate.isBetterThan(authReq, headerConfigs)) {
+                    LOGGER.debug(
+                            "Found better auth candidate {} {}",
+                            msg.getRequestHeader().getMethod(),
+                            msg.getRequestHeader().getURI());
+                    authReq = candidate;
                 }
             }
-
-            if (authMsg != null && View.isInitialised()) {
-                AuthUtils.logUserMessage(
-                        Level.INFO,
-                        Constant.messages.getString(
-                                "authhelper.auth.method.browser.output.sessionid", getHrefId(msg)));
-            }
-        }
-        if (firstHrefId == 0 && msg.getHistoryRef() != null) {
-            firstHrefId = msg.getHistoryRef().getHistoryId();
         }
     }
 
-    private String getHrefId(HttpMessage msg) {
-        if (msg.getHistoryRef() != null) {
-            return Integer.toString(msg.getHistoryRef().getHistoryId());
-        }
-        return "?";
-    }
-
     public HttpMessage getAuthMsg() {
-        return authMsg;
+        return authReq.getMsg();
     }
 
     public void resetAuthMsg() {
-        this.authMsg = null;
+        this.authReq = null;
+    }
+
+    protected static boolean isBetterThan(
+            SessionManagementRequestDetails smrd1, SessionManagementRequestDetails smrd2) {
+        if (smrd2 == null) {
+            return true;
+        }
+        if (smrd1.getConfidence() > smrd2.getConfidence()) {
+            return true;
+        }
+        if (smrd1.getConfidence() < smrd2.getConfidence()) {
+            return false;
+        }
+        return smrd1.getTokens().size() > smrd2.getTokens().size();
     }
 
     public HttpMessage getFallbackMsg() {
         return fallbackMsg;
     }
+
+    protected static List<Pair<String, String>> extractKeyValuePairs(String input) {
+        List<Pair<String, String>> keyValuePairs = new ArrayList<>();
+        Pattern pattern = Pattern.compile("\\{%([^:]+):([^%]+)%}");
+        Matcher matcher = pattern.matcher(input);
+
+        while (matcher.find()) {
+            keyValuePairs.add(new Pair<>(matcher.group(1), matcher.group(2)));
+        }
+
+        return keyValuePairs;
+    }
+
+    protected static int messageTokenCount(HttpMessage msg, List<Pair<String, String>> kvPairs) {
+        int count = 0;
+        Collection<SessionToken> tokens = AuthUtils.getAllTokens(msg, false).values();
+
+        for (Pair<String, String> kvPair : kvPairs) {
+            for (SessionToken token : tokens) {
+                if (token.getSource().equals(kvPair.first)
+                        && token.getKey().equals(kvPair.second)) {
+                    count++;
+                    break;
+                }
+            }
+        }
+        return count;
+    }
+
+    @Getter
+    class AuthRequestDetails {
+        private HttpMessage msg;
+        private List<SessionToken> tokens;
+        private boolean incAllTokens;
+        private boolean incUsername;
+        private boolean incPassword;
+
+        public AuthRequestDetails(HttpMessage msg) {
+            this.msg = msg;
+            String body = msg.getResponseBody().toString();
+            incUsername =
+                    authCreds != null
+                            && StringUtils.isNotBlank(authCreds.getUsername())
+                            && body.contains(authCreds.getUsername());
+            incPassword =
+                    authCreds != null
+                            && StringUtils.isNotBlank(authCreds.getPassword())
+                            && body.contains(authCreds.getPassword());
+        }
+
+        /**
+         * Is this a better candidate for the authentication request than the supplied
+         * AuthRequestDetails.
+         *
+         * @param ard the details to compare with
+         * @param headerConfigs - cannot cache these as they may change when session management
+         *     auto-detect used
+         * @return true if this is a better candidate than the supplied one.
+         */
+        public boolean isBetterThan(
+                AuthRequestDetails ard, List<Pair<String, String>> headerConfigs) {
+            int statusCode = msg.getResponseHeader().getStatusCode();
+            if (HttpStatusCode.isClientError(statusCode)
+                    || HttpStatusCode.isServerError(statusCode)) {
+                // Ignore all error responses
+                return false;
+            }
+            if (ard == null) {
+                return true;
+            }
+            // Including the right tokens is the most important thing, assuming there are any
+            // relevant ones
+            if (headerConfigs != null) {
+                // matching any relevant session tokens is the most important thing
+                List<Pair<String, String>> kvPairs = new ArrayList<>();
+                for (Pair<String, String> pair : headerConfigs) {
+                    if (HttpHeader.COOKIE.equalsIgnoreCase(pair.first)) {
+                        // We track cookies directly
+                        continue;
+                    }
+                    // Most of the time we'd just expect one token, but we need to cope with an
+                    // arbitrary number
+                    kvPairs.addAll(extractKeyValuePairs(pair.second));
+                }
+                if (messageTokenCount(msg, kvPairs) > messageTokenCount(ard.getMsg(), kvPairs)) {
+                    return true;
+                }
+            }
+            if (this.incPassword && !ard.incPassword) {
+                return true;
+            }
+            if (this.incUsername && !ard.incUsername) {
+                return true;
+            }
+            if (isPost(msg) && !isPost(ard.getMsg())) {
+                return true;
+            }
+            // Default to the current one so we always choose the oldest most relevant request
+            return false;
+        }
+    }
 }