Fix plugin CSRF protection (igniterealtime#1566)

GTMYang · Jan 3, 2020 · cf4e40f · cf4e40f
1 parent 6c58e53
commit cf4e40f
Show file tree

Hide file tree

Showing 2 changed files with 15 additions and 11 deletions.
diff --git a/documentation/plugin-dev-guide.html b/documentation/plugin-dev-guide.html
@@ -159,7 +159,7 @@ <h2>Structure of a Plugin</h2>
                     plugin's Readme.</li>
             </ul>
             If the license type is not set, it is assumed to be other.</li>
-</ul></p>
+</ul>
 
 Several additional files can be present in the plugin to provide additional information to
 end-users (all placed in the main plugin directory):
@@ -342,7 +342,6 @@ <h3>Writing Pages for the Admin Console</h3>
 </ul>
 
 The following HTML snippet demonstrates a valid page:
-</p>
 
 <fieldset>
     <legend>Sample HTML</legend>
@@ -388,7 +387,7 @@ <h4>Using i18n in your Plugins</h4>
         <tt>&lt;description&gt;${plugin.description}&lt;/description&gt;</tt>
     </li>
 </ul>
-</p>
+
 <h2>Using the Openfire Build Script</h2>
 
 <p>
@@ -474,8 +473,6 @@ <h2>Implementing Your Plugin</h2>
 
 </ol>
 
-</p>
-
 <h2>Openfire admin tags</h2>
         Openfire provides useful JSP tags that can be used. To enable them on a JSP page, simply add:<br>
         <code>&lt;%@ taglib uri="admin" prefix="admin" %&gt;</code>
@@ -503,16 +500,19 @@ <h2>CSRF protection</h2>
                     <li>Set a servlet request attribute with key "csrf"</li>
                 </ul>
             </li>
-            <li>Ensure that GET requests do not modify any settings or change any data as this protections is not
+            <li>Ensure that GET requests do not modify any settings or change any data as this protection is not
                 enabled for GET requests</li>
             <li>Ensure that any form submitted in the admin page has a field called <code>csrf</code> whose value is that
             defined by the request attribute "csrf" - for example:<br>
                 <code>&lt;input name="csrf" value="&lt;c:out value="${csrf}"/&gt;" type="hidden"&gt;</code></li>
         </ol>
-        That's it! If a CSRF attack is detected, the admin page will be reloaded (with a simple HTTP GET request) with
+        If a CSRF attack is detected, the admin page will be reloaded (with a simple HTTP GET request) with
         the session attribute <code>FlashMessageTag.ERROR_MESSAGE_KEY</code> set to indicate the problem - it's
         therefore advised to include the <code>&lt;admin:FlashMessage/&gt;</code> at the top of your JSP page.
 
+        <p><strong>NOTE</strong>: It is still important to ensure that all your output is properly escaped using
+        <code>&lt;c:out&gt;</code> tags or the equivalent.</p>
+
 <h2>Plugin FAQ</h2>
 
 <b>Can I deploy a plugin as a directory instead of a JAR?</b>

diff --git a/xmppserver/src/main/java/org/jivesoftware/openfire/container/PluginServlet.java b/xmppserver/src/main/java/org/jivesoftware/openfire/container/PluginServlet.java
@@ -119,20 +119,18 @@ public void service(HttpServletRequest request, HttpServletResponse response) {
                         response.sendRedirect(request.getRequestURI());
                         return;
                     }
-                    // Set a new CSRF
-                    final String csrf = StringUtils.randomString(32);
-                    request.getSession().setAttribute(CSRF_ATTRIBUTE, csrf);
-                    request.setAttribute(CSRF_ATTRIBUTE, csrf);
                 }
                 // Handle JSP requests.
                 if (pathInfo.endsWith(".jsp")) {
+                    setCSRF(request);
                     if (handleDevJSP(pathInfo, request, response)) {
                         return;
                     }
                     handleJSP(pathInfo, request, response);
                 }
                 // Handle servlet requests.
                 else if (getServlet(pathInfo) != null) {
+                    setCSRF(request);
                     handleServlet(pathInfo, request, response);
                 }
                 // Handle image/other requests.
@@ -147,6 +145,12 @@ else if (getServlet(pathInfo) != null) {
         }
     }
 
+    private static void setCSRF(final HttpServletRequest request) {
+        final String csrf = StringUtils.randomString(32);
+        request.getSession().setAttribute(CSRF_ATTRIBUTE, csrf);
+        request.setAttribute(CSRF_ATTRIBUTE, csrf);
+    }
+
     private boolean passesCsrf(final HttpServletRequest request) {
         if (request.getMethod().equals("GET")) {
             // No CSRF's for GET requests