Skip to content

Commit be38831

Browse files
mmoquimmoqui
committed
Bug #14836
The pb is the query part of the URI of HTTP GET requests isn't taken when checking for attempt to perform a CRUD operation on a resource in Silverpeas. Now all parts of the query is checked. Ensure also to avoid regexp flooding.
1 parent b753316 commit be38831

File tree

4 files changed

+41
-37
lines changed

4 files changed

+41
-37
lines changed

‎core-rs/src/main/java/org/silverpeas/core/web/token/SynchronizerTokenService.java

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@
4646
import java.time.OffsetDateTime;
4747
import java.util.Arrays;
4848
import java.util.List;
49+
import java.util.Optional;
4950

5051
/**
5152
* A service to manage the synchronizer tokens used in Silverpeas to protect the user sessions or
@@ -65,12 +66,14 @@ public class SynchronizerTokenService {
6566
public static final String SESSION_TOKEN_KEY = "X-STKN";
6667
public static final String NAVIGATION_TOKEN_KEY = "X-NTKN";
6768
private static final String UNPROTECTED_URI_RULE =
68-
"(?i)(?!.*(/icons/|/images/|/qaptcha|rpdcsearch/|rclipboard/|rselectionpeaswrapper/|rusernotification/|services/usernotifications/|blockingNews|services/password/)).*";
69+
"(?i)(?![\\w/]{0,500}(/icons/|/images/|/qaptcha|rpdcsearch/|rclipboard" +
70+
"/|rselectionpeaswrapper/|rusernotification/|services/usernotifications/|blockingNews" +
71+
"|services/password/)).{0,500}";
6972
private static final String DEFAULT_GET_RULE_KEYWORDS = "(delete|update|creat|save|block)";
7073
private static final String DEFAULT_GET_RULE_ON_KEYWORD =
71-
"(?i)^.*" + DEFAULT_GET_RULE_KEYWORDS + ".*$";
74+
"(?i)^.{0,500}" + DEFAULT_GET_RULE_KEYWORDS + ".{0,500}$";
7275
private static final String DEFAULT_GET_RULE =
73-
"(?i)^/\\w+[\\w/]*/jsp/.*" + DEFAULT_GET_RULE_KEYWORDS + ".*$";
76+
"(?i)^/\\w+[\\w/]{0,500}/jsp/.{0,500}" + DEFAULT_GET_RULE_KEYWORDS + ".{0,500}$";
7477
private static final SilverLogger logger = SilverLogger.getLogger("silverpeas.core.security");
7578
private static final List<String> DEFAULT_PROTECTED_METHODS = Arrays.asList("POST", "PUT",
7679
"DELETE");
@@ -184,14 +187,21 @@ public void validate(HttpServletRequest request, final boolean onKeywordsOnly)
184187
* validated.
185188
*/
186189
public boolean isAProtectedResource(HttpServletRequest request, final boolean onKeywordsOnly) {
190+
// attempt to bypass the protection by overflowing the regexp matching
191+
if (request.getRequestURI().length() > 500) {
192+
return true;
193+
}
187194
boolean isProtected = false;
188195
if (request.getRequestURI().matches(UNPROTECTED_URI_RULE)) {
189196
isProtected = DEFAULT_PROTECTED_METHODS.contains(request.getMethod());
190197
if (!isProtected && "GET".equals(request.getMethod())) {
191198
String path = getRequestPath(request);
199+
String query = Optional.ofNullable(request.getQueryString())
200+
.map(q -> "?" + q)
201+
.orElse("");
192202
isProtected = onKeywordsOnly ?
193-
path.matches(DEFAULT_GET_RULE_ON_KEYWORD) :
194-
path.matches(DEFAULT_GET_RULE);
203+
(path + query).matches(DEFAULT_GET_RULE_ON_KEYWORD) :
204+
(path + query).matches(DEFAULT_GET_RULE);
195205
}
196206
}
197207
return isProtected;

‎core-war/src/main/java/org/silverpeas/web/todo/servlets/TodoRequestRouter.java

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,6 @@
3535

3636
import java.util.Collection;
3737

38-
/**
39-
* Class declaration
40-
* @author
41-
*/
4238
public class TodoRequestRouter extends ComponentRequestRouter<ToDoSessionController> {
4339

4440
private static final long serialVersionUID = 6455939825707914384L;
@@ -74,17 +70,15 @@ public String getSessionControlBeanName() {
7470
*/
7571
public String getDestination(String function, ToDoSessionController scc,
7672
HttpRequest request) {
77-
78-
String destination = "";
79-
73+
String destination;
8074
try {
8175

8276
final String defaultDestination = "/todo/jsp/todo.jsp";
8377
if (function.startsWith("Main")) {
8478
scc.getSelectedTodoIds().clear();
8579
destination = defaultDestination;
8680
} else if (function.startsWith("searchResult")) {
87-
destination = "/todo/jsp/todoEdit.jsp?Action=Update&ToDoId="
81+
destination = "/todo/jsp/todoEdit.jsp?Action=Edit&ToDoId="
8882
+ request.getParameter("Id");
8983
} else if (function.startsWith("diffusion")) {
9084
// initialisation du userPanel avec les participants
@@ -97,7 +91,7 @@ public String getDestination(String function, ToDoSessionController scc,
9791
} else if ("DeleteTodo".equals(function)) {
9892
request.mergeSelectedItemsInto(scc.getSelectedTodoIds());
9993
if (!scc.getSelectedTodoIds().isEmpty()) {
100-
scc.removeTabToDo(scc.getSelectedTodoIds().toArray(new String[scc.getSelectedTodoIds().size()]));
94+
scc.removeTabToDo(scc.getSelectedTodoIds().toArray(new String[0]));
10195
}
10296
scc.getSelectedTodoIds().clear();
10397
destination = defaultDestination;

‎core-war/src/main/webapp/todo/jsp/todoEdit.jsp

Lines changed: 22 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -75,8 +75,8 @@ function gotoToDo()
7575
}
7676
7777
function ifCorrectFormExecute(callback) {
78-
var errorMsg = "";
79-
var errorNb = 0;
78+
let errorMsg = "";
79+
let errorNb = 0;
8080
8181
if (isWhitespace(document.todoEditForm.Name.value)) {
8282
errorMsg += " - '<%=todo.getString("nomToDo")%>' <%=todo.getString("MustContainsText")%>\n";
@@ -89,7 +89,7 @@ function ifCorrectFormExecute(callback) {
8989
errorNb++;
9090
}
9191
92-
var dateErrors = isPeriodValid('StartDate', 'EndDate');
92+
const dateErrors = isPeriodValid('StartDate', 'EndDate');
9393
$(dateErrors).each(function(index, error) {
9494
errorMsg += " - " + error.message + "\n";
9595
errorNb++;
@@ -112,25 +112,25 @@ function ifCorrectFormExecute(callback) {
112112
}
113113
114114
115-
function reallyAdd()
115+
function save()
116116
{
117117
ifCorrectFormExecute(function() {
118-
document.todoEditForm.Action.value = "ReallyAdd";
118+
document.todoEditForm.Action.value = "Save";
119119
document.todoEditForm.submit();
120120
});
121121
}
122122
123-
function reallyUpdate()
123+
function update()
124124
{
125-
document.todoEditForm.Name.disabled = false;
125+
document.todoEditForm.Name.disabled = false;
126126
document.todoEditForm.PercentCompleted.disabled = false;
127127
document.todoEditForm.Description.disabled = false;
128128
document.todoEditForm.StartDate.disabled = false;
129129
document.todoEditForm.EndDate.disabled = false;
130130
document.todoEditForm.Classification.disabled = false;
131131
document.todoEditForm.Priority.disabled = false;
132132
ifCorrectFormExecute(function() {
133-
document.todoEditForm.Action.value = "ReallyUpdate";
133+
document.todoEditForm.Action.value = "Update";
134134
document.todoEditForm.submit();
135135
});
136136
}
@@ -164,7 +164,7 @@ function test(){
164164
String toPrint = null;
165165
166166
167-
if (action.equals("Add") || action.equals("Update")) {
167+
if (action.equals("Add") || action.equals("Edit")) {
168168
todo.setCurrentToDoHeader(null);
169169
todo.setCurrentAttendees(null);
170170
}
@@ -177,12 +177,12 @@ function test(){
177177
String toDoId = request.getParameter("ToDoId");
178178
179179
if (toDoId != null) {
180-
if (toDoId.length() == 0) {
180+
if (toDoId.isEmpty()) {
181181
toDoId = null;
182182
}
183183
}
184184
185-
/* Update et premier acces a la page */
185+
/* Edit et premier acces a la page */
186186
if (toDoId != null) {
187187
todoHeader = todo.getToDoHeader(toDoId);
188188
attendees = todo.getToDoAttendees(toDoId);
@@ -264,8 +264,8 @@ function test(){
264264
</head>
265265
266266
<%
267-
/* ReallyAdd || ReallyUpdate */
268-
if (action.equals("ReallyAdd") || action.equals("ReallyUpdate")) {
267+
/* Save || Update */
268+
if (action.equals("Save") || action.equals("Update")) {
269269
String name = request.getParameter("Name");
270270
String description = request.getParameter("Description");
271271
String priority = request.getParameter("Priority");
@@ -290,11 +290,11 @@ function test(){
290290
Date date1 = null;
291291
if (name == null)
292292
throw new Exception("nameErreur");
293-
else if (name.length() == 0)
293+
else if (name.isEmpty())
294294
throw new Exception("nameErreur");
295295
296296
try {
297-
if (startDate.trim().length() > 0)
297+
if (!startDate.trim().isEmpty())
298298
date1 = DateUtil.stringToDate(startDate, todo.getLanguage());
299299
}
300300
catch (java.text.ParseException e) {
@@ -304,7 +304,7 @@ function test(){
304304
Date date2 = null;
305305
306306
try {
307-
if (endDate.trim().length() == 0)
307+
if (endDate.trim().isEmpty())
308308
date2 = date1;
309309
else
310310
date2 = DateUtil.stringToDate(endDate, todo.getLanguage());
@@ -325,7 +325,7 @@ function test(){
325325
j++;
326326
}
327327
328-
/* ReallyAdd */
328+
/* Save */
329329
// now diffusion list can be empty
330330
if (todoHeader.getId() == null) {
331331
String id = todo.addToDo(name, description, priority, classification, date1, startHour, date2, endHour, percent);
@@ -336,7 +336,7 @@ function test(){
336336
return;
337337
}
338338
339-
/* ReallyUpdate */
339+
/* Update */
340340
else {
341341
if (todo.getUserId().equals(todoHeader.getDelegatorId())) {
342342
todo.updateToDo(todoHeader.getId(), name, description, priority, classification, date1, startHour, date2, endHour, percent);
@@ -416,8 +416,8 @@ function test(){
416416
out.println("</div>");
417417
}
418418
419-
/* Add || Update || View || DiffusionListOK */
420-
else if (action.equals("Update") || action.equals("Add") || action.equals("View") || action.equals("DiffusionListOK")) {
419+
/* Add || Edit || View || DiffusionListOK */
420+
else if (action.equals("Edit") || action.equals("Add") || action.equals("View") || action.equals("DiffusionListOK")) {
421421
%>
422422
<center>
423423
<view:board>
@@ -617,9 +617,9 @@ out.println(frame.printMiddle());
617617
ButtonPane pane = graphicFactory.getButtonPane();
618618
Button button = null;
619619
if (todoHeader.getId() != null)
620-
button = graphicFactory.getFormButton(todo.getString("mettreAJour"), "javascript:onClick=reallyUpdate()", false);
620+
button = graphicFactory.getFormButton(todo.getString("mettreAJour"), "javascript:onClick=update()", false);
621621
else
622-
button = graphicFactory.getFormButton(todo.getString("ajouter"), "javascript:onClick=reallyAdd()", false);
622+
button = graphicFactory.getFormButton(todo.getString("ajouter"), "javascript:onClick=save()", false);
623623
624624
pane.addButton(button);
625625

‎core-web/src/main/java/org/silverpeas/core/web/filter/exception/WebSecurityException.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ public abstract class WebSecurityException extends SilverpeasException {
3636

3737
/**
3838
* Default constructor.
39-
* @param message
39+
* @param message the reason of the exception
4040
*/
4141
protected WebSecurityException(final String message) {
4242
super(message + LocalDateTime.now().format(DateTimeFormatter.ISO_DATE_TIME));

0 commit comments

Comments
 (0)