CVE-2018-1000129

@@ -264,20 +264,28 @@ protected void doOptions(HttpServletRequest req, HttpServletResponse resp) throw
     @SuppressWarnings({ "PMD.AvoidCatchingThrowable", "PMD.AvoidInstanceofChecksInCatchClause" })
     private void handle(ServletRequestHandler pReqHandler,HttpServletRequest pReq, HttpServletResponse pResp) throws IOException {
         JSONAware json = null;
+
         try {
             // Check access policy
             requestHandler.checkAccess(allowDnsReverseLookup ? pReq.getRemoteHost() : null,
                                        pReq.getRemoteAddr(),
                                        getOriginOrReferer(pReq));
 
+            // If a callback is given, check this is a valid javascript function name
+            validateCallbackIfGiven(pReq);
+
             // Remember the agent URL upon the first request. Needed for discovery
             updateAgentDetailsIfNeeded(pReq);
 
             // Dispatch for the proper HTTP request method
             json = handleSecurely(pReqHandler, pReq, pResp);
         } catch (Throwable exp) {
-            json = requestHandler.handleThrowable(
+            try {
+                json = requestHandler.handleThrowable(
                     exp instanceof RuntimeMBeanException ? ((RuntimeMBeanException) exp).getTargetException() : exp);
+            } catch (Throwable exp2) {
+                exp2.printStackTrace();
+            }
         } finally {
             setCorsHeader(pReq, pResp);
 
@@ -289,6 +297,7 @@ private void handle(ServletRequestHandler pReqHandler,HttpServletRequest pReq, H
         }
     }
 
+
     private JSONAware handleSecurely(final ServletRequestHandler pReqHandler, final HttpServletRequest pReq, final HttpServletResponse pResp) throws IOException, PrivilegedActionException {
         Subject subject = (Subject) pReq.getAttribute(ConfigKey.JAAS_SUBJECT_REQUEST_ATTRIBUTE);
         if (subject != null) {
@@ -380,15 +389,6 @@ private void setCorsHeader(HttpServletRequest pReq, HttpServletResponse pResp) {
         }
     }
 
-    // Extract mime type for response (if not JSONP)
-    private String getMimeType(HttpServletRequest pReq) {
-        String requestMimeType = pReq.getParameter(ConfigKey.MIME_TYPE.getKeyValue());
-        if (requestMimeType != null) {
-            return requestMimeType;
-        }
-        return configMimeType;
-    }
-
     private boolean isStreamingEnabled(HttpServletRequest pReq) {
         String streamingFromReq = pReq.getParameter(ConfigKey.STREAMING.getKeyValue());
         if (streamingFromReq != null) {
@@ -470,8 +470,12 @@ Configuration initConfig(ServletConfig pConfig) {
 
     private void sendResponse(HttpServletResponse pResp, HttpServletRequest pReq, JSONAware pJson) throws IOException {
         String callback = pReq.getParameter(ConfigKey.CALLBACK.getKeyValue());
-        setContentType(pResp, callback != null ? "text/javascript" : getMimeType(pReq));
 
+        setContentType(pResp,
+                       MimeTypeUtil.getResponseMimeType(
+                           pReq.getParameter(ConfigKey.MIME_TYPE.getKeyValue()),
+                           configMimeType, callback
+                                                       ));
         pResp.setStatus(HttpServletResponse.SC_OK);
         setNoCacheHeaders(pResp);
         if (pJson == null) {
@@ -487,6 +491,12 @@ private void sendResponse(HttpServletResponse pResp, HttpServletRequest pReq, JS
         }
     }
 
+    private void validateCallbackIfGiven(HttpServletRequest pReq) {
+        String callback = pReq.getParameter(ConfigKey.CALLBACK.getKeyValue());
+        if (callback != null && !MimeTypeUtil.isValidCallback(callback)) {
+            throw new IllegalArgumentException("Invalid callback name given, which must be a valid javascript function name");
+        }
+    }
     private void sendStreamingResponse(HttpServletResponse pResp, String pCallback, JSONStreamAware pJson) throws IOException {
         Writer writer = new OutputStreamWriter(pResp.getOutputStream(), "UTF-8");
         IoUtil.streamResponseAndClose(writer, pJson, pCallback);

@@ -0,0 +1,74 @@
+package org.jolokia.util;
+
+import java.util.regex.Pattern;
+
+/**
+ * Helper class for handling proper response mime types
+ *
+ * @author roland
+ * @since 24.01.18
+ */
+public class MimeTypeUtil {
+
+
+    /**
+     * Extract the response mime type. This value is calculated for different situations:
+     * <p>
+     * <ul>
+     * <li>If a callback is given and its valid, the mime type is "text/javascript"</li>
+     * <li>Otherwise:
+     * <ul>
+     * <li>If a valid mimeType is given in the request ("text/plain", "application/json"), then this
+     * mimet type is returned</li>
+     * <li>If another mimeType is given, then "text/plain" is used</li>
+     * <li>If no mimeType is given then a given default mime type is used, but also sanitized
+     * as described above</li>
+     * </ul>
+     * </li>
+     * </ul>
+     *
+     * @param pRequestMimeType the mimetype given in the request
+     * @param defaultMimeType  the default mime type to use if none is given in the request
+     * @param pCallback        a callback given (can be null)
+     */
+    public static String getResponseMimeType(String pRequestMimeType, String defaultMimeType, String pCallback) {
+
+        // For a valid given callback, return "text/javascript" for proper inclusion
+        if (pCallback != null && isValidCallback(pCallback)) {
+            return "text/javascript";
+        }
+
+        // Pick up mime time from request, but sanitize
+        if (pRequestMimeType != null) {
+            return sanitize(pRequestMimeType);
+        }
+
+        // Use the given default mime type (possibly picked up from a configuration)
+        return sanitize(defaultMimeType);
+    }
+
+    private static String sanitize(String mimeType) {
+        for (String accepted : new String[]{
+            "application/json",
+            "text/plain"
+        }) {
+            if (accepted.equalsIgnoreCase(mimeType)) {
+                return accepted;
+            }
+        }
+        return "text/plain";
+    }
+
+    /**
+     * Check that a callback matches a javascript function name. The argument must be not null
+     *
+     * @param pCallback callback to verify
+     * @return true if valud, false otherwise
+     */
+    public static boolean isValidCallback(String pCallback) {
+        Pattern validJavaScriptFunctionNamePattern =
+            Pattern.compile("^[$A-Z_][0-9A-Z_$]*$", Pattern.CASE_INSENSITIVE);
+        return validJavaScriptFunctionNamePattern.matcher(pCallback).matches();
+    }
+
+}
\ No newline at end of file

@@ -113,7 +113,7 @@ public void configWithOverWrite() throws ServletException {
     }
 
     @Test
-    public void initWithcustomAccessRestrictor() throws ServletException {
+    public void initWithCustomAccessRestrictor() throws ServletException {
         prepareStandardInitialisation();
         servlet.destroy();
     }
@@ -251,6 +251,44 @@ public void simpleGet() throws ServletException, IOException {
         servlet.destroy();
     }
 
+    @Test
+    public void simpleGetWithWrongMimeType() throws ServletException, IOException {
+        checkMimeTypes("text/html", "text/plain");
+    }
+
+    @Test
+    public void simpleGetWithTextPlainMimeType() throws ServletException, IOException {
+        checkMimeTypes("text/plain", "text/plain");
+    }
+
+    @Test
+    public void simpleGetWithApplicationJsonMimeType() throws ServletException, IOException {
+        checkMimeTypes("application/json", "application/json");
+    }
+
+    private void checkMimeTypes(String given, final String expected) throws ServletException, IOException {
+        prepareStandardInitialisation();
+
+        initRequestResponseMocks(
+            getStandardRequestSetup(),
+            new Runnable() {
+                public void run() {
+                    response.setCharacterEncoding("utf-8");
+                    // The default content type
+                    response.setContentType(expected);
+                    response.setStatus(200);
+                }
+            });
+        expect(request.getPathInfo()).andReturn(HttpTestUtil.HEAP_MEMORY_GET_REQUEST);
+        expect(request.getParameter(ConfigKey.MIME_TYPE.getKeyValue())).andReturn(given);
+        replay(request, response);
+
+        servlet.doGet(request, response);
+
+        verifyMocks();
+        servlet.destroy();
+    }
+
     @Test
     public void simpleGetWithNoReverseDnsLookupFalse() throws ServletException, IOException {
         checkNoReverseDns(false,"127.0.0.1");
@@ -484,6 +522,7 @@ public void run() {
                 });
         expect(request.getPathInfo()).andReturn(HttpTestUtil.HEAP_MEMORY_GET_REQUEST);
         expect(request.getAttribute("subject")).andReturn(null);
+        expect(request.getParameter(ConfigKey.MIME_TYPE.getKeyValue())).andReturn(null);
 
         replay(request, response);
 
@@ -493,6 +532,30 @@ public void run() {
         servlet.destroy();
     }
 
+    @Test
+    public void withInvalidCallback() throws IOException, ServletException {
+        servlet = new AgentServlet(new AllowAllRestrictor());
+        initConfigMocks(null, null,"Error 400", IllegalArgumentException.class);
+        replay(config, context);
+        servlet.init(config);
+        ByteArrayOutputStream sw = initRequestResponseMocks(
+            "doSomethingEvil(); myCallback",
+            getStandardRequestSetup(),
+            getStandardResponseSetup());
+        expect(request.getPathInfo()).andReturn(HttpTestUtil.HEAP_MEMORY_GET_REQUEST);
+        expect(request.getAttribute("subject")).andReturn(null);
+        expect(request.getParameter(ConfigKey.MIME_TYPE.getKeyValue())).andReturn(null);
+
+        replay(request, response);
+
+        servlet.doGet(request, response);
+        String resp = sw.toString();
+        assertTrue(resp.contains("error_type"));
+        assertTrue(resp.contains("IllegalArgumentException"));
+        assertTrue(resp.matches(".*status.*400.*"));
+        servlet.destroy();
+    }
+
     @Test
     public void withException() throws ServletException, IOException {
         servlet = new AgentServlet(new AllowAllRestrictor());
@@ -606,7 +669,7 @@ private ByteArrayOutputStream initRequestResponseMocks(String callback,Runnable
         response = createMock(HttpServletResponse.class);
         setNoCacheHeaders(response);
 
-        expect(request.getParameter(ConfigKey.CALLBACK.getKeyValue())).andReturn(callback);
+        expect(request.getParameter(ConfigKey.CALLBACK.getKeyValue())).andReturn(callback).anyTimes();
         requestSetup.run();
         responseSetup.run();
 
@@ -648,6 +711,7 @@ private Runnable getStandardResponseSetup() {
         return new Runnable() {
             public void run() {
                 response.setCharacterEncoding("utf-8");
+                // The default content type
                 response.setContentType("text/plain");
                 response.setStatus(200);
             }
@@ -690,7 +754,7 @@ public void run() {
                 expect(request.getAttribute(ConfigKey.JAAS_SUBJECT_REQUEST_ATTRIBUTE)).andReturn(null);
 
                 expect(request.getPathInfo()).andReturn(HttpTestUtil.HEAP_MEMORY_GET_REQUEST);
-                expect(request.getParameter(ConfigKey.MIME_TYPE.getKeyValue())).andReturn("text/plain");
+                expect(request.getParameter(ConfigKey.MIME_TYPE.getKeyValue())).andReturn("text/plain").anyTimes();
                 StringBuffer buf = new StringBuffer();
                 buf.append(url).append(HttpTestUtil.HEAP_MEMORY_GET_REQUEST);
                 expect(request.getRequestURL()).andReturn(buf);

@@ -224,6 +224,9 @@ public void doHandle(HttpExchange pExchange) throws IOException {
                                        extractOriginOrReferer(pExchange));
             String method = pExchange.getRequestMethod();
 
+            // If a callback is given, check this is a valid javascript function name
+            validateCallbackIfGiven(parsedUri);
+
             // Dispatch for the proper HTTP request method
             if ("GET".equalsIgnoreCase(method)) {
                 setHeaders(pExchange);
@@ -244,6 +247,14 @@ public void doHandle(HttpExchange pExchange) throws IOException {
         }
     }
 
+
+    private void validateCallbackIfGiven(ParsedUri pUri) {
+        String callback = pUri.getParameter(ConfigKey.CALLBACK.getKeyValue());
+        if (callback != null && !MimeTypeUtil.isValidCallback(callback)) {
+            throw new IllegalArgumentException("Invalid callback name given, which must be a valid javascript function name");
+        }
+    }
+
     // ========================================================================
 
     // Used for checking origin or referer is an origin policy is enabled
@@ -342,10 +353,11 @@ private void sendStreamingResponse(HttpExchange pExchange, ParsedUri pParsedUri,
         Headers headers = pExchange.getResponseHeaders();
         if (pJson != null) {
             headers.set("Content-Type", getMimeType(pParsedUri) + "; charset=utf-8");
-            String callback = pParsedUri.getParameter(ConfigKey.CALLBACK.getKeyValue());
             pExchange.sendResponseHeaders(200, 0);
             Writer writer = new OutputStreamWriter(pExchange.getResponseBody(), "UTF-8");
-            IoUtil.streamResponseAndClose(writer, pJson, callback);
+
+            String callback = pParsedUri.getParameter(ConfigKey.CALLBACK.getKeyValue());
+            IoUtil.streamResponseAndClose(writer, pJson, callback != null && MimeTypeUtil.isValidCallback(callback) ? callback : null);
         } else {
             headers.set("Content-Type", "text/plain");
             pExchange.sendResponseHeaders(200,-1);
@@ -360,7 +372,7 @@ private void sendAllJSON(HttpExchange pExchange, ParsedUri pParsedUri, JSONAware
                 headers.set("Content-Type", getMimeType(pParsedUri) + "; charset=utf-8");
                 String json = pJson.toJSONString();
                 String callback = pParsedUri.getParameter(ConfigKey.CALLBACK.getKeyValue());
-                String content = callback == null ? json : callback + "(" + json + ");";
+                String content = callback != null && MimeTypeUtil.isValidCallback(callback) ? callback + "(" + json + ");" : json;
                 byte[] response = content.getBytes("UTF8");
                 pExchange.sendResponseHeaders(200,response.length);
                 out = pExchange.getResponseBody();
@@ -380,16 +392,10 @@ private void sendAllJSON(HttpExchange pExchange, ParsedUri pParsedUri, JSONAware
 
     // Get the proper mime type according to configuration
     private String getMimeType(ParsedUri pParsedUri) {
-        if (pParsedUri.getParameter(ConfigKey.CALLBACK.getKeyValue()) != null) {
-            return "text/javascript";
-        } else {
-            String mimeType = pParsedUri.getParameter(ConfigKey.MIME_TYPE.getKeyValue());
-            if (mimeType != null) {
-                return mimeType;
-            }
-            mimeType = configuration.get(ConfigKey.MIME_TYPE);
-            return mimeType != null ? mimeType : ConfigKey.MIME_TYPE.getDefaultValue();
-        }
+        return MimeTypeUtil.getResponseMimeType(
+            pParsedUri.getParameter(ConfigKey.MIME_TYPE.getKeyValue()),
+            configuration.get(ConfigKey.MIME_TYPE),
+            pParsedUri.getParameter(ConfigKey.CALLBACK.getKeyValue()));
     }
 
     // Creat a log handler from either the given class or by creating a default log handler printing

@@ -74,14 +74,74 @@ public void testCallbackGet() throws IOException, URISyntaxException {
         assertTrue(result.startsWith("data({"));
     }
 
+
+    @Test
+    public void testInvalidMimeType() throws IOException, URISyntaxException {
+        checkMimeType("text/html", "text/plain");
+    }
+
+    @Test
+    public void testMimeTypeApplicationJson() throws IOException, URISyntaxException {
+        checkMimeType("application/json", "application/json");
+    }
+
+    private void checkMimeType(String given, String expected) throws IOException, URISyntaxException {
+        HttpExchange exchange = prepareExchange("http://localhost:8080/jolokia/read/java.lang:type=Memory/HeapMemoryUsage?mimeType=" + given);
+
+        // Simple GET method
+        expect(exchange.getRequestMethod()).andReturn("GET");
+
+        Headers header = new Headers();
+        ByteArrayOutputStream out = prepareResponse(handler, exchange, header);
+
+        handler.doHandle(exchange);
+
+        assertEquals(header.getFirst("content-type"),expected + "; charset=utf-8");
+    }
+
+    @Test
+    public void testInvalidCallbackGetStreaming() throws IOException, URISyntaxException, ParseException {
+        checkInvalidCallback(true);
+    }
+
+    @Test
+    public void testInvalidCallbackGetNonStreaming() throws IOException, URISyntaxException, ParseException {
+        checkInvalidCallback(false);
+    }
+
+    private void checkInvalidCallback(boolean streaming) throws URISyntaxException, IOException, ParseException {
+        JolokiaHttpHandler handler = new JolokiaHttpHandler(getConfig(ConfigKey.SERIALIZE_EXCEPTION, Boolean.toString(streaming)));
+        handler.start(false);
+
+        HttpExchange exchange = prepareExchange("http://localhost:8080/jolokia/read/java.lang:type=Memory/HeapMemoryUsage?callback=evilCallback();data");
+
+        // Simple GET method
+        expect(exchange.getRequestMethod()).andReturn("GET");
+
+        Headers header = new Headers();
+        ByteArrayOutputStream out = prepareResponse(handler, exchange, header);
+
+        handler.doHandle(exchange);
+
+        assertEquals(header.getFirst("content-type"),"text/plain; charset=utf-8");
+        String result = out.toString("utf-8");
+        JSONObject resp = (JSONObject) new JSONParser().parse(result);
+        assertTrue(resp.containsKey("error"));
+        assertEquals(resp.get("error_type"), IllegalArgumentException.class.getName());
+        assertTrue(((String) resp.get("error")).contains("callback"));
+        assertFalse(((String) resp.get("error")).contains("evilCallback"));
+
+        handler.stop();
+    }
+
+
     @Test
     public void testCallbackPost() throws URISyntaxException, IOException, java.text.ParseException {
         HttpExchange exchange = prepareExchange("http://localhost:8080/jolokia?callback=data",
                                                 "Content-Type","text/plain; charset=UTF-8",
                                                 "Origin",null
                                                );
 
-        // Simple GET method
         prepareMemoryPostReadRequest(exchange);
         Headers header = new Headers();
         ByteArrayOutputStream out = prepareResponse(handler, exchange, header);