Skip to content

Commit

Permalink
Fix several security issues in 4.2 (#929)
Browse files Browse the repository at this point in the history
* OF-1417 CVE-2017-15911 Fix XSS issues in host setup

* OF-1329 Prevent session fixation attack

* OF-1403 Escape group name in MUC admin

* OF-1393 Make randomString more random

* OF-1400 Escape servername field

* OF-1401 Validate SMS host and escape error message
  • Loading branch information
dwd authored and akrherz committed Nov 16, 2017
1 parent 56ac521 commit 7ff1f73
Show file tree
Hide file tree
Showing 10 changed files with 54 additions and 9 deletions.
2 changes: 2 additions & 0 deletions src/i18n/openfire_i18n_en.properties
Original file line number Diff line number Diff line change
Expand Up @@ -1069,6 +1069,8 @@ index.home=Server Directory:
index.certificate-warning=Found RSA certificate that is not valid for the server domain.
index.dns-warning={0}DNS configuration appears to be missing or incorrect.{1}
index.certificate-error=Unable to access certificate store. The keystore may be corrupt.
index.domain-stringprep-error=This domain contains illegal characters.
index.hostname-stringprep-error=This hostname contains illegal characters.
index.server_name=XMPP Domain Name:
index.host_name=Server Host Name (FQDN):
index.server_port=Server Ports
Expand Down
4 changes: 2 additions & 2 deletions src/java/org/jivesoftware/util/StringUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -524,7 +524,7 @@ public static String[] toLowerCaseWordArray(String text) {
* array index.
*/
private static char[] numbersAndLetters = ("0123456789abcdefghijklmnopqrstuvwxyz" +
"0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ").toCharArray();
"ABCDEFGHIJKLMNOPQRSTUVWXYZ").toCharArray();

/**
* Returns a random String of numbers and letters (lower and upper case)
Expand All @@ -544,7 +544,7 @@ public static String randomString(int length) {
// Create a char buffer to put random letters and numbers in.
char[] randBuffer = new char[length];
for (int i = 0; i < randBuffer.length; i++) {
randBuffer[i] = numbersAndLetters[randGen.nextInt(71)];
randBuffer[i] = numbersAndLetters[randGen.nextInt(numbersAndLetters.length - 1)];

This comment has been minimized.

Copy link
@Flowdalic

Flowdalic Nov 25, 2017

Member

The - 1 shouldn't be necessary, or in other words: Why don't you like the capital letter 'Z'? ;)

}
return new String(randBuffer);
}
Expand Down
11 changes: 11 additions & 0 deletions src/java/org/jivesoftware/util/WebManager.java
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import javax.servlet.http.HttpSession;

/**
* A utility bean for Openfire admin console pages.
*/
Expand All @@ -59,6 +61,15 @@ public class WebManager extends WebBean {
public WebManager() {
}

/**
* Invalidates and recreates session (do this on login/logout).
*/
public HttpSession invalidateSession() {
session.invalidate();
session = request.getSession(true);
return session;
}

/**
* Returns the auth token redirects to the login page if an auth token is not found.
*/
Expand Down
11 changes: 9 additions & 2 deletions src/web/index.jsp
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
<%@ page import="java.util.Arrays" %>
<%@ page import="java.util.List" %>
<%@ page import="org.jivesoftware.openfire.net.DNSUtil" %>
<%@ page import="org.xmpp.packet.JID" %>

<%@ taglib uri="http://java.sun.com/jsp/jstl/core" prefix="c" %>
<%@ taglib uri="http://java.sun.com/jsp/jstl/fmt" prefix="fmt" %>
Expand Down Expand Up @@ -247,7 +248,10 @@
<% } catch (Exception e) { %>
<img src="images/error-16x16.gif" width="12" height="12" border="0" alt="<fmt:message key="index.certificate-error" />" title="<fmt:message key="index.certificate-error" />">&nbsp;
<% } %>
${webManager.serverInfo.XMPPDomain}
<c:out value="${webManager.serverInfo.XMPPDomain}"/>
<% try { String whatevs = JID.domainprep(webManager.getXMPPServer().getServerInfo().getXMPPDomain()); } catch (Exception e) { %>
<img src="images/error-16x16.gif" width="12" height="12" border="0" alt="<fmt:message key="index.domain-stringprep-error" />" title="<fmt:message key="index.domain-stringprep-error" />">&nbsp;
<% } %>
</td>
</tr>
<tr><td>&nbsp;</td></tr>
Expand Down Expand Up @@ -284,7 +288,10 @@
<fmt:message key="index.host_name" />
</td>
<td class="c2">
${webManager.serverInfo.hostname}
<c:out value="${webManager.serverInfo.hostname}"/>
<% try { String whatevs = JID.domainprep(webManager.getXMPPServer().getServerInfo().getHostname()); } catch (Exception e) { %>
<img src="images/error-16x16.gif" width="12" height="12" border="0" alt="<fmt:message key="index.hostname-stringprep-error" />" title="<fmt:message key="index.hostname-stringprep-error" />">&nbsp;
<% } %>
<% // Determine if the DNS configuration for this XMPP domain needs to be evaluated.
final String xmppDomain = XMPPServer.getInstance().getServerInfo().getXMPPDomain();
final String hostname = XMPPServer.getInstance().getServerInfo().getHostname();
Expand Down
1 change: 1 addition & 0 deletions src/web/login.jsp
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@
throw new UnauthorizedException("User '" + loginUsername + "' not allowed to login.");
}
authToken = AuthFactory.authenticate(loginUsername, password);
session = admin.invalidateSession();
}
else {
errors.put("unauthorized", LocaleUtils.getLocalizedString("login.failed.unauthorized"));
Expand Down
2 changes: 1 addition & 1 deletion src/web/muc-sysadmins.jsp
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@
<img src="images/user.gif" width="16" height="16" align="top" title="<fmt:message key="groupchat.admins.user" />" alt="<fmt:message key="groupchat.admins.user" />"/>
<% } %>
<a href="<%= isGroup ? "group-edit.jsp?group=" + URLEncoder.encode(jidDisplay) : "user-properties.jsp?username=" + URLEncoder.encode(jid.getNode()) %>">
<%= jidDisplay %></a>
<c:out value="${jidDisplay}"/></a>
</td>
</td>
<td width="1%" align="center">
Expand Down
8 changes: 7 additions & 1 deletion src/web/server-props.jsp
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
java.util.HashMap"
%>
<%@ page import="java.util.Map" %>
<%@ page import="org.xmpp.packet.JID" %>

<%@ taglib uri="http://java.sun.com/jsp/jstl/core" prefix="c" %>
<%@ taglib uri="http://java.sun.com/jsp/jstl/fmt" prefix="fmt" %>
Expand Down Expand Up @@ -92,6 +93,11 @@
if (serverName == null) {
errors.put("serverName", "");
}
try {
JID.domainprep(serverName);
} catch (Exception e) {
errors.put("serverName", "");
}
if (port < 1) {
errors.put("port", "");
}
Expand Down Expand Up @@ -235,7 +241,7 @@
<fmt:message key="server.props.name" />
</td>
<td class="c2">
<input type="text" name="serverName" value="<%= (serverName != null) ? serverName : "" %>"
<input type="text" name="serverName" value="<%= (serverName != null) ? StringUtils.escapeForXML(serverName) : "" %>"
size="30" maxlength="150">
<% if (errors.containsKey("serverName")) { %>
<br>
Expand Down
16 changes: 14 additions & 2 deletions src/web/setup/setup-host-settings.jsp
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
org.jivesoftware.openfire.XMPPServer"
%>
<%@ page import="java.net.UnknownHostException" %>
<%@ page import="org.xmpp.packet.JID" %>
<%@ page import="org.jivesoftware.util.StringUtils" %>

<%@ taglib uri="http://java.sun.com/jsp/jstl/core" prefix="c" %>
<%@ taglib uri="http://java.sun.com/jsp/jstl/fmt" prefix="fmt" %>
Expand Down Expand Up @@ -43,6 +45,16 @@
if (fqdn == null || fqdn.isEmpty()) {
errors.put("fqdn", "fqdn");
}
try {
fqdn = JID.domainprep(fqdn);
} catch (IllegalArgumentException e) {
errors.put("fqdn", "fqdn");
}
try {
domain = JID.domainprep(domain);
} catch (IllegalArgumentException e) {
errors.put("domain", "domain");
}
if (XMPPServer.getInstance().isStandAlone()) {
if (embeddedPort == Integer.MIN_VALUE) {
errors.put("embeddedPort", "embeddedPort");
Expand Down Expand Up @@ -153,7 +165,7 @@
</td>
<td width="99%">
<input type="text" size="30" maxlength="150" name="domain"
value="<%= ((domain != null) ? domain : "") %>">
value="<%= ((domain != null) ? StringUtils.escapeForXML(domain) : "") %>">
<span class="jive-setup-helpicon" onmouseover="domTT_activate(this, event, 'content', '<fmt:message key="setup.host.settings.domain.help" />', 'styleClass', 'jiveTooltip', 'trail', true, 'delay', 300, 'lifetime', 8000);"></span>
<% if (errors.get("domain") != null) { %>
<span class="jive-error-text">
Expand All @@ -168,7 +180,7 @@
</td>
<td width="99%">
<input type="text" size="30" maxlength="150" name="fqdn"
value="<%= ((fqdn != null) ? fqdn : "") %>">
value="<%= ((fqdn != null) ? StringUtils.escapeForXML(fqdn) : "") %>">
<span class="jive-setup-helpicon" onmouseover="domTT_activate(this, event, 'content', '<fmt:message key="setup.host.settings.fqdn.help" />', 'styleClass', 'jiveTooltip', 'trail', true, 'delay', 300, 'lifetime', 8000);"></span>
<% if (errors.get("fqdn") != null) { %>
<span class="jive-error-text">
Expand Down
6 changes: 6 additions & 0 deletions src/web/system-sms.jsp
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
org.jivesoftware.util.*"
errorPage="error.jsp"
%>
<%@ page import="org.xmpp.packet.JID" %>

<%@ taglib uri="admin" prefix="admin" %>
<%@ taglib uri="http://java.sun.com/jsp/jstl/core" prefix="c" %>
Expand Down Expand Up @@ -54,6 +55,11 @@
{
errors.put( "host", "cannot be missing or empty." );
}
try {
JID.domainprep(host);
} catch (Exception e) {
errors.put("host", "Invalid hostname");
}
if ( port < 0 || port > 65535 )
{
errors.put( "port", "must be a number between 0 and 65535." );
Expand Down
2 changes: 1 addition & 1 deletion src/web/system-smstest.jsp
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@
}
catch ( Exception e )
{
errors.put( "sendfailed", SmsService.getDescriptiveMessage( e ) );
errors.put( "sendfailed", StringUtils.escapeHTMLTags(SmsService.getDescriptiveMessage( e ), true) );
}
}
}
Expand Down

2 comments on commit 7ff1f73

@dwd
Copy link
Member Author

@dwd dwd commented on 7ff1f73 Nov 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I was thinking that it'd be inclusive, but it probably isn't. I'll fix.

@guusdk
Copy link
Member

@guusdk guusdk commented on 7ff1f73 Nov 25, 2017 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.