Skip to content

Commit eb08b5e

Browse files
NMS-13231: Backport Security Issues from Last Month
1 parent b6a8557 commit eb08b5e

File tree

31 files changed

+233
-83
lines changed

31 files changed

+233
-83
lines changed

Diff for: core/web-assets/src/main/assets/js/apps/onms-requisitions/lib/scripts/controllers/ForeignSource.js

+3-3
Original file line numberDiff line numberDiff line change
@@ -454,7 +454,7 @@ require('../services/Requisitions');
454454
RequisitionsService.startTiming();
455455
RequisitionsService.saveForeignSourceDefinition($scope.foreignSourceDef).then(
456456
function() { // success
457-
growl.success('The definition for the requisition ' + $scope.foreignSource + ' has been saved.');
457+
growl.success('The definition for the requisition ' + _.escape($scope.foreignSource) + ' has been saved.');
458458
form.$dirty = false;
459459
},
460460
$scope.errorHandler
@@ -474,7 +474,7 @@ require('../services/Requisitions');
474474
RequisitionsService.startTiming();
475475
RequisitionsService.deleteForeignSourceDefinition($scope.foreignSource).then(
476476
function() { // success
477-
growl.success('The foreign source definition for ' + $scope.foreignSource + 'has been reseted.');
477+
growl.success('The foreign source definition for ' + _.escape($scope.foreignSource) + 'has been reseted.');
478478
$scope.initialize();
479479
},
480480
$scope.errorHandler
@@ -517,7 +517,7 @@ require('../services/Requisitions');
517517
* @methodOf ForeignSourceController
518518
*/
519519
$scope.initialize = function() {
520-
growl.success('Retrieving definition for requisition ' + $scope.foreignSource + '...');
520+
growl.success('Retrieving definition for requisition ' + _.escape($scope.foreignSource) + '...');
521521
RequisitionsService.getForeignSourceDefinition($scope.foreignSource).then(
522522
function(foreignSourceDef) { // success
523523
$scope.foreignSourceDef = foreignSourceDef;

Diff for: core/web-assets/src/main/assets/js/apps/onms-requisitions/lib/scripts/controllers/Node.js

+2-2
Original file line numberDiff line numberDiff line change
@@ -476,7 +476,7 @@ const RequisitionMetaDataEntry = require('../model/RequisitionMetaDataEntry');
476476
RequisitionsService.startTiming();
477477
RequisitionsService.saveNode($scope.node).then(
478478
function() { // success
479-
growl.success('The node ' + $scope.node.nodeLabel + ' has been saved.');
479+
growl.success('The node ' + _.escape($scope.node.nodeLabel) + ' has been saved.');
480480
$scope.foreignId = $scope.node.foreignId;
481481
form.$dirty = false;
482482
},
@@ -492,7 +492,7 @@ const RequisitionMetaDataEntry = require('../model/RequisitionMetaDataEntry');
492492
* @methodOf NodeController
493493
*/
494494
$scope.refresh = function() {
495-
growl.success('Retrieving node ' + $scope.foreignId + ' from requisition ' + $scope.foreignSource + '...');
495+
growl.success('Retrieving node ' + _.escape($scope.foreignId) + ' from requisition ' + _.escape($scope.foreignSource) + '...');
496496
RequisitionsService.getNode($scope.foreignSource, $scope.foreignId).then(
497497
function(node) { // success
498498
$scope.node = node;

Diff for: core/web-assets/src/main/assets/js/apps/onms-requisitions/lib/scripts/controllers/QuickAddNode.js

+3-3
Original file line numberDiff line numberDiff line change
@@ -111,8 +111,8 @@ const QuickNode = require('../model/QuickNode');
111111
*/
112112
$scope.provision = function() {
113113
$scope.isSaving = true;
114-
growl.info($sanitize('The node ' + $scope.node.nodeLabel + ' is being added to requisition ' + $scope.node.foreignSource + '. Please wait...'));
115-
var successMessage = $sanitize('The node ' + $scope.node.nodeLabel + ' has been added to requisition ' + $scope.node.foreignSource);
114+
growl.info('The node ' + _.escape($scope.node.nodeLabel) + ' is being added to requisition ' + _.escape($scope.node.foreignSource) + '. Please wait...');
115+
var successMessage = 'The node ' + _.escape($scope.node.nodeLabel) + ' has been added to requisition ' + _.escape($scope.node.foreignSource);
116116
RequisitionsService.quickAddNode($scope.node).then(
117117
function() { // success
118118
$scope.reset();
@@ -238,7 +238,7 @@ const QuickNode = require('../model/QuickNode');
238238
function() { // success
239239
RequisitionsService.synchronizeRequisition(foreignSource, false).then(
240240
function() {
241-
growl.success('The requisition ' + foreignSource + ' has been created and synchronized.');
241+
growl.success('The requisition ' + _.escape(foreignSource) + ' has been created and synchronized.');
242242
$scope.foreignSources.push(foreignSource);
243243
},
244244
$scope.errorHandler

Diff for: core/web-assets/src/main/assets/js/apps/onms-requisitions/lib/scripts/controllers/Requisition.js

+3-3
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,7 @@ require('../services/Synchronize');
200200
* @param {object} The node's object to delete
201201
*/
202202
$scope.deleteNode = function(node) {
203-
bootbox.confirm('Are you sure you want to remove the node ' + node.nodeLabel + '?', function(ok) {
203+
bootbox.confirm('Are you sure you want to remove the node ' + _.escape(node.nodeLabel) + '?', function(ok) {
204204
if (ok) {
205205
RequisitionsService.startTiming();
206206
RequisitionsService.deleteNode(node).then(
@@ -214,7 +214,7 @@ require('../services/Synchronize');
214214
if (index > -1) {
215215
$scope.filteredNodes.splice(index,1);
216216
}
217-
growl.success('The node ' + node.nodeLabel + ' has been deleted.');
217+
growl.success('The node ' + _.escape(node.nodeLabel) + ' has been deleted.');
218218
},
219219
$scope.errorHandler
220220
);
@@ -295,7 +295,7 @@ require('../services/Synchronize');
295295
if (value) {
296296
$scope.pageSize = value;
297297
}
298-
growl.success('Retrieving requisition ' + $scope.foreignSource + '...');
298+
growl.success('Retrieving requisition ' + _.escape($scope.foreignSource) + '...');
299299
RequisitionsService.getRequisition($scope.foreignSource).then(
300300
function(requisition) { // success
301301
$scope.requisition = requisition;

Diff for: core/web-assets/src/main/assets/js/apps/onms-requisitions/lib/scripts/controllers/Requisitions.js

+10-10
Original file line numberDiff line numberDiff line change
@@ -172,14 +172,14 @@ require('../services/Synchronize');
172172
}
173173
});
174174
modalInstance.result.then(function(targetForeignSource) {
175-
bootbox.confirm('This action will override the existing foreign source definition for the requisition named ' + targetForeignSource + ', using ' + foreignSource + ' as a template. Are you sure you want to continue ? This cannot be undone.', function(ok) {
175+
bootbox.confirm('This action will override the existing foreign source definition for the requisition named ' + _.escape(targetForeignSource) + ', using ' + _.escape(foreignSource) + ' as a template. Are you sure you want to continue ? This cannot be undone.', function(ok) {
176176
if (!ok) {
177177
return;
178178
}
179179
RequisitionsService.startTiming();
180180
RequisitionsService.cloneForeignSourceDefinition(foreignSource, targetForeignSource).then(
181181
function() { // success
182-
growl.success('The foreign source definition for ' + foreignSource + ' has been cloned to ' + targetForeignSource);
182+
growl.success('The foreign source definition for ' + _.escape(foreignSource) + ' has been cloned to ' + _.escape(targetForeignSource));
183183
},
184184
$scope.errorHandler
185185
);
@@ -201,18 +201,18 @@ require('../services/Synchronize');
201201
if (foreignSource) {
202202
// Validate Requisition
203203
if (foreignSource.match(/[/\\?:&*'"]/)) {
204-
bootbox.alert('Cannot add the requisition ' + foreignSource + ' because the following characters are invalid:<br/>:, /, \\, ?, &, *, \', "');
204+
bootbox.alert('Cannot add the requisition ' + _.escape(foreignSource) + ' because the following characters are invalid:<br/>:, /, \\, ?, &, *, \', "');
205205
return;
206206
}
207207
var r = $scope.requisitionsData.getRequisition(foreignSource);
208208
if (r) {
209-
bootbox.alert('Cannot add the requisition ' + foreignSource+ ' because there is already a requisition with that name');
209+
bootbox.alert('Cannot add the requisition ' + _.escape(foreignSource) + ' because there is already a requisition with that name');
210210
return;
211211
}
212212
// Create Requisition
213213
RequisitionsService.addRequisition(foreignSource).then(
214214
function(r) { // success
215-
growl.success('The requisition ' + r.foreignSource + ' has been created.');
215+
growl.success('The requisition ' + _.escape(r.foreignSource) + ' has been created.');
216216
},
217217
$scope.errorHandler
218218
);
@@ -271,7 +271,7 @@ require('../services/Synchronize');
271271
RequisitionsService.startTiming();
272272
RequisitionsService.updateDeployedStatsForRequisition(requisition).then(
273273
function() { // success
274-
growl.success('The deployed statistics for ' + requisition.foreignSource + ' has been updated.');
274+
growl.success('The deployed statistics for ' + _.escape(requisition.foreignSource) + ' has been updated.');
275275
},
276276
$scope.errorHandler
277277
);
@@ -286,12 +286,12 @@ require('../services/Synchronize');
286286
* @param {string} foreignSource The name of the requisition
287287
*/
288288
$scope.removeAllNodes = function(foreignSource) {
289-
bootbox.confirm('Are you sure you want to remove all the nodes from ' + foreignSource + '?', function(ok) {
289+
bootbox.confirm('Are you sure you want to remove all the nodes from ' + _.escape(foreignSource) + '?', function(ok) {
290290
if (ok) {
291291
RequisitionsService.startTiming();
292292
RequisitionsService.removeAllNodesFromRequisition(foreignSource).then(
293293
function() { // success
294-
growl.success('All the nodes from ' + foreignSource + ' have been removed, and the requisition has been synchronized.');
294+
growl.success('All the nodes from ' + _.escape(foreignSource) + ' have been removed, and the requisition has been synchronized.');
295295
var req = $scope.requisitionsData.getRequisition(foreignSource);
296296
req.reset();
297297
},
@@ -310,12 +310,12 @@ require('../services/Synchronize');
310310
* @param {string} foreignSource The name of the requisition
311311
*/
312312
$scope.delete = function(foreignSource) {
313-
bootbox.confirm('Are you sure you want to remove the requisition ' + foreignSource + '?', function(ok) {
313+
bootbox.confirm('Are you sure you want to remove the requisition ' + _.escape(foreignSource) + '?', function(ok) {
314314
if (ok) {
315315
RequisitionsService.startTiming();
316316
RequisitionsService.deleteRequisition(foreignSource).then(
317317
function() { // success
318-
growl.success('The requisition ' + foreignSource + ' has been deleted.');
318+
growl.success('The requisition ' + _.escape(foreignSource) + ' has been deleted.');
319319
},
320320
$scope.errorHandler
321321
);

Diff for: core/web-assets/src/main/assets/js/apps/onms-requisitions/lib/scripts/services/Synchronize.js

+2-2
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ require('./Requisitions');
4646
RequisitionsService.startTiming();
4747
RequisitionsService.synchronizeRequisition(requisition.foreignSource, rescanExisting).then(
4848
function() { // success
49-
growl.success('The import operation has been started for ' + requisition.foreignSource + ' (rescanExisting? ' + rescanExisting + ')<br/>Use <b>refresh</b> to update the deployed statistics');
49+
growl.success('The import operation has been started for ' + _.escape(requisition.foreignSource) + ' (rescanExisting? ' + rescanExisting + ')<br/>Use <b>refresh</b> to update the deployed statistics');
5050
requisition.setDeployed(true);
5151
},
5252
errorHandler
@@ -58,7 +58,7 @@ require('./Requisitions');
5858
'Choose <b>no</b> to synchronize only the new and deleted nodes with the database executing the scan phase only for new nodes.<br/>' +
5959
'Choose <b>dbonly</b> to synchronize all the nodes with the database skipping the scan phase.<br/>' +
6060
'Choose <b>cancel</b> to abort the request.',
61-
title: 'Synchronize Requisition ' + requisition.foreignSource,
61+
title: 'Synchronize Requisition ' + _.escape(requisition.foreignSource),
6262
buttons: {
6363
fullSync: {
6464
label: 'Yes',

Diff for: core/web-assets/src/test/javascript/ng-requisitions/controllers/ForeignSource.test.js

+1
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
'use strict';
77

88
const angular = require('angular-js');
9+
const _ = require('underscore-js');
910
require('angular-mocks');
1011
require('../../../../../src/main/assets/js/apps/onms-requisitions/requisitions');
1112

Diff for: core/web-assets/src/test/javascript/ng-requisitions/controllers/Node.test.js

+1
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
'use strict';
99

1010
const angular = require('angular-js');
11+
const _ = require('underscore-js');
1112
require('angular-mocks');
1213
require('../../../../../src/main/assets/js/apps/onms-requisitions/requisitions');
1314

Diff for: core/web-assets/src/test/javascript/ng-requisitions/controllers/Requisition.test.js

+1
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
'use strict';
99

1010
const angular = require('angular-js');
11+
const _ = require('underscore-js');
1112
require('angular-mocks');
1213
require('../../../../../src/main/assets/js/apps/onms-requisitions/requisitions');
1314

Diff for: core/web-assets/src/test/javascript/ng-requisitions/controllers/Requisitions.test.js

+1
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
'use strict';
99

1010
const angular = require('angular-js');
11+
const _ = require('underscore-js');
1112
require('angular-mocks');
1213
require('../../../../../src/main/assets/js/apps/onms-requisitions/requisitions');
1314

Diff for: opennms-webapp/src/main/java/org/opennms/web/admin/users/AddNewUserServlet.java

+5
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,11 @@ public void doPost(HttpServletRequest request, HttpServletResponse response) thr
6969
UserManager userFactory = UserFactory.getInstance();
7070

7171
String userID = request.getParameter("userID");
72+
73+
if (userID != null && userID.matches(".*[&<>\"`']+.*")) {
74+
throw new ServletException("User ID must not contain any HTML markup.");
75+
}
76+
7277
String password = request.getParameter("pass1");
7378

7479
boolean hasUser = false;

Diff for: opennms-webapp/src/main/java/org/opennms/web/admin/users/RenameUserServlet.java

+4
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,10 @@ public void doPost(HttpServletRequest request, HttpServletResponse response) thr
6060
String userID = request.getParameter("userID");
6161
String newID = request.getParameter("newID");
6262

63+
if (newID != null && newID.matches(".*[&<>\"`']+.*")) {
64+
throw new ServletException("User ID must not contain any HTML markup.");
65+
}
66+
6367
// now save to the xml file
6468
try {
6569
UserManager userFactory = UserFactory.getInstance();

Diff for: opennms-webapp/src/main/java/org/opennms/web/controller/admin/group/GroupController.java

+13-1
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,11 @@ private ModelAndView renameGroup(HttpServletRequest request, HttpServletResponse
151151

152152
String oldName = request.getParameter("groupName");
153153
String newName = request.getParameter("newName");
154-
154+
155+
if (newName != null && newName.matches(".*[&<>\"`']+.*")) {
156+
throw new ServletException("Group ID must not contain any HTML markup.");
157+
}
158+
155159
if (StringUtils.hasText(oldName) && StringUtils.hasText(newName)) {
156160
m_groupRepository.renameGroup(oldName, newName);
157161
}
@@ -312,6 +316,14 @@ private ModelAndView addGroup(HttpServletRequest request, HttpServletResponse re
312316
groupComment = "";
313317
}
314318

319+
if (groupName != null && groupName.matches(".*[&<>\"`']+.*")) {
320+
throw new ServletException("Group ID must not contain any HTML markup.");
321+
}
322+
323+
if (groupComment != null && groupComment.matches(".*[&<>\"`']+.*")) {
324+
throw new ServletException("Group comment must not contain any HTML markup.");
325+
}
326+
315327
boolean hasGroup = false;
316328
try {
317329
hasGroup = m_groupRepository.groupExists(groupName);

Diff for: opennms-webapp/src/main/webapp/WEB-INF/jsp/admin/notification/noticeWizard/eventNotices.jsp

+10-9
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
%>
3737
<%@ taglib uri="http://java.sun.com/jsp/jstl/core" prefix="c"%>
3838
<%@ taglib uri="http://java.sun.com/jsp/jstl/functions" prefix="fn"%>
39+
<%@ taglib uri="https://www.owasp.org/index.php/OWASP_Java_Encoder_Project" prefix="e"%>
3940

4041
<jsp:include page="/includes/bootstrap.jsp" flush="false" >
4142
<jsp:param name="title" value="Event Notifications" />
@@ -109,31 +110,31 @@
109110
<c:forEach items="${notifications}" var="notification">
110111
<tr>
111112
<td>
112-
<input type="button" class="btn btn-secondary" value="Edit" onclick="javascript:editNotice('${notification.escapedName}')"/>
113+
<input type="button" class="btn btn-secondary" value="Edit" onclick="javascript:editNotice('${e:forJavaScript(notification.escapedName)}')"/>
113114
</td>
114115
<td>
115-
<input type="button" class="btn btn-secondary" value="Delete" onclick="javascript:deleteNotice('${notification.escapedName}')"/>
116+
<input type="button" class="btn btn-secondary" value="Delete" onclick="javascript:deleteNotice('${e:forJavaScript(notification.escapedName)}')"/>
116117
</td>
117118
<td>
118119
<c:choose>
119120
<c:when test="${notification.isOn}">
120-
<input type="radio" value="Off" onclick="javascript:setStatus('${notification.escapedName}','off')"/>Off
121-
<input type="radio" value="On" CHECKED onclick="javascript:setStatus('${notification.escapedName}','on')"/>On
121+
<input type="radio" value="Off" onclick="javascript:setStatus('${e:forJavaScript(notification.escapedName)}','off')"/>Off
122+
<input type="radio" value="On" CHECKED onclick="javascript:setStatus('${e:forJavaScript(notification.escapedName)}','on')"/>On
122123
</c:when>
123124
<c:otherwise>
124-
<input type="radio" value="Off" CHECKED onclick="javascript:setStatus('${notification.escapedName}','off')"/>Off
125-
<input type="radio" value="On" onclick="javascript:setStatus('${notification.escapedName}','on')"/>On
125+
<input type="radio" value="Off" CHECKED onclick="javascript:setStatus('${e:forJavaScript(notification.escapedName)}','off')"/>Off
126+
<input type="radio" value="On" onclick="javascript:setStatus('${e:forJavaScript(notification.escapedName)}','on')"/>On
126127
</c:otherwise>
127128
</c:choose>
128129
</td>
129130
<td>
130-
${notification.name}
131+
${fn:escapeXml(notification.name)}
131132
</td>
132133
<td>
133-
${notification.eventLabel}
134+
${fn:escapeXml(notification.eventLabel)}
134135
</td>
135136
<td>
136-
${notification.displayUei}
137+
${fn:escapeXml(notification.displayUei)}
137138
</td>
138139
</tr>
139140
</c:forEach>

Diff for: opennms-webapp/src/main/webapp/WEB-INF/jsp/admin/userGroupView/groups/groupDetail.jsp

+4-3
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
%>
4141

4242
<%@page import="org.opennms.web.group.WebGroup"%>
43+
<%@ page import="org.opennms.core.utils.WebSecurityUtils" %>
4344

4445
<%
4546
WebGroup group = (WebGroup)request.getAttribute("group");
@@ -60,13 +61,13 @@
6061
<div class="col-md-6">
6162
<div class="card">
6263
<div class="card-header">
63-
<span>Details for Group: <%=group.getName()%></span>
64+
<span>Details for Group: <%=WebSecurityUtils.sanitizeString(group.getName())%></span>
6465
</div>
6566
<table class="table table-sm">
6667
<tr>
6768
<th>Comments:</th>
6869
<td width="75%">
69-
<%=group.getComments()%>
70+
<%=WebSecurityUtils.sanitizeString(group.getComments())%>
7071
</td>
7172
</tr>
7273
<tr>
@@ -79,7 +80,7 @@
7980
<% } else { %>
8081
<ul class="list-unstyled">
8182
<% for (String user : users) { %>
82-
<li> <%=user%> </li>
83+
<li> <%=WebSecurityUtils.sanitizeString(user)%> </li>
8384
<% } %>
8485
</ul>
8586
<% } %>

0 commit comments

Comments
 (0)