Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

#23900 fixing ajax vulnerabilities reported by sonar #23911

Merged
merged 4 commits into from
Jan 27, 2023
Merged

#23900 fixing ajax vulnerabilities reported by sonar #23911

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d27b002
#23900 fixing ajax vulnerabilities reported by sonar
fabrizzio-dotCMS Jan 26, 2023
8439ab7
#23900 clean up import
fabrizzio-dotCMS Jan 26, 2023
3733f4c
Merge branch 'master' into issue-23900-major-security-fixes
fabrizzio-dotCMS Jan 26, 2023
d07c834
Merge branch 'master' into issue-23900-major-security-fixes
fabrizzio-dotCMS Jan 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions dotCMS/src/main/java/com/dotmarketing/portlets/osgi/AJAX/OSGIAJAX.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import java.io.InputStream;
import java.io.OutputStream;
import java.nio.file.Files;
import java.util.Set;
import javax.servlet.ServletException;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
Expand Down Expand Up @@ -266,4 +267,19 @@ private void remove () {

}

@Override
protected Set<String> getAllowedCommands() {
return Set.of(
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't be a constant static

Copy link
Contributor Author

@fabrizzio-dotCMS fabrizzio-dotCMS Jan 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I had it as a class field. non-static but Mockito wouldnt see the results when calling the real method so ended up inlining the Set instantiation. It isn't a big penalti performance wise @jdotcms

"remove",
"restart",
"modifyExtraPackages",
"getExtraPackages",
"add",
"start",
"stop",
"deploy",
"undeploy",
"action"
);
}
}
48 changes: 40 additions & 8 deletions dotCMS/src/main/java/com/dotmarketing/portlets/osgi/AJAX/OSGIBaseAJAX.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,19 @@
import com.dotcms.api.web.HttpServletRequestThreadLocal;
import com.dotmarketing.business.APILocator;
import com.dotmarketing.cms.factories.PublicCompanyFactory;
import com.dotmarketing.exception.DotDataException;
import com.dotmarketing.exception.DotRuntimeException;
import com.dotmarketing.exception.DotSecurityException;
import com.dotmarketing.servlets.ajax.AjaxAction;
import com.dotmarketing.util.Logger;
import com.dotmarketing.util.UtilMethods;
import com.dotmarketing.util.WebKeys;
import com.google.common.annotations.VisibleForTesting;
import com.liferay.portal.language.LanguageUtil;
import com.liferay.portal.model.User;
import java.io.IOException;
import java.lang.reflect.Method;
import java.util.Set;
import javax.servlet.ServletException;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
Expand All @@ -31,9 +36,9 @@ public boolean validateUser() {
throw new DotRuntimeException (e.getMessage());
}
}


protected abstract Set<String> getAllowedCommands();

public void service(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {

Expand All @@ -46,20 +51,23 @@ public void service(HttpServletRequest request, HttpServletResponse response) th
Class partypes[] = new Class[] { HttpServletRequest.class, HttpServletResponse.class };
Object arglist[] = new Object[] { request, response };
try {
if(getUser()==null || !APILocator.getLayoutAPI().doesUserHaveAccessToPortlet("dynamic-plugins", getUser())){
if(getUser()==null || !doesUserHaveAccessToPortlet(getUser())){
response.sendError(401);
return;
}



meth = this.getClass().getMethod(cmd, partypes);
if(getAllowedCommands().contains(cmd)) {
meth = getMethod(cmd, partypes);
} else if (UtilMethods.isSet(cmd)){
Logger.error(this.getClass(), String.format("Attempt to run Invalid command %s :",cmd));
return;
}

} catch (Exception e) {

try {
cmd = "action";
meth = this.getClass().getMethod(cmd, partypes);
meth = getMethod(cmd, partypes);
} catch (Exception ex) {
Logger.error(this.getClass(), "Trying to run method:" + cmd);
Logger.error(this.getClass(), e.getMessage(), e.getCause());
Expand All @@ -77,6 +85,30 @@ public void service(HttpServletRequest request, HttpServletResponse response) th

}

/**
* Extracted so it can be mocked
* @param user
* @return
* @throws DotDataException
*/
@VisibleForTesting
boolean doesUserHaveAccessToPortlet(final User user) throws DotDataException {
return APILocator.getLayoutAPI().doesUserHaveAccessToPortlet("dynamic-plugins", user);
}

/**
* extracted as a separated method, so it can be verified when called
* @param method
* @param parameterTypes
* @return
* @throws NoSuchMethodException
*/
@VisibleForTesting
Method getMethod(String method, Class<?>... parameterTypes ) throws NoSuchMethodException {
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a ReflectionUtils, may be better there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First this is a deprecated class Im not spending too much effort on it.. and the only reason I extracted that logic into a separate method was to verify that the method gets called using mokito @jdotcms

return this.getClass().getMethod(method, parameterTypes);
}


public void writeError(HttpServletResponse response, String error) throws IOException {
String ret = null;

Expand Down
12 changes: 12 additions & 0 deletions dotCMS/src/main/java/com/dotmarketing/portlets/workflows/ajax/WfActionAjax.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import java.io.IOException;
import java.util.Arrays;
import java.util.List;
import java.util.Set;


@Deprecated
Expand Down Expand Up @@ -137,4 +138,15 @@ public void save(final HttpServletRequest request,
writeError(response, e.getMessage());
}
} // save.

/**
* Security check demanded by Sonar
* We register all the allowed methods down here
*
* @return allowed method names
*/
@Override
protected Set<String> getAllowedCommands() {
return Set.of( "action", "reorder", "delete", "add", "save", "deleteActionForStep" );
Copy link
Contributor

Choose a reason for hiding this comment

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

same constant

}
}
29 changes: 20 additions & 9 deletions dotCMS/src/main/java/com/dotmarketing/portlets/workflows/ajax/WfActionClassAjax.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,5 @@
package com.dotmarketing.portlets.workflows.ajax;

import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;

import javax.servlet.ServletException;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import com.dotmarketing.business.APILocator;
import com.dotmarketing.business.web.UserWebAPI;
import com.dotmarketing.business.web.WebAPILocator;
Expand All @@ -21,6 +12,15 @@
import com.dotmarketing.util.Logger;
import com.liferay.portal.model.User;

import javax.servlet.ServletException;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.Set;

@Deprecated
public class WfActionClassAjax extends WfBaseAction {

Expand Down Expand Up @@ -135,5 +135,16 @@ public void save(final HttpServletRequest request, final HttpServletResponse res
writeError(response, e.getMessage());
}
}

/**
* Security check demanded by Sonar
* We register all the allowed methods down here
*
* @return allowed method names
*/
@Override
protected Set<String> getAllowedCommands() {
return Set.of("save","add","delete","reorder","action");
}
}

27 changes: 25 additions & 2 deletions dotCMS/src/main/java/com/dotmarketing/portlets/workflows/ajax/WfBaseAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package com.dotmarketing.portlets.workflows.ajax;

import java.io.IOException;
import java.lang.reflect.Method;
import java.util.Set;

import javax.servlet.ServletException;
import javax.servlet.http.HttpServletRequest;
Expand All @@ -9,11 +11,15 @@
import com.dotmarketing.cms.factories.PublicCompanyFactory;
import com.dotmarketing.servlets.ajax.AjaxAction;
import com.dotmarketing.util.Logger;
import com.dotmarketing.util.UtilMethods;
import com.google.common.annotations.VisibleForTesting;
import com.liferay.portal.language.LanguageUtil;

@Deprecated
abstract class WfBaseAction extends AjaxAction {

protected abstract Set<String> getAllowedCommands();

public void service(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
String cmd = request.getParameter("cmd");
java.lang.reflect.Method meth = null;
Expand All @@ -25,13 +31,18 @@ public void service(HttpServletRequest request, HttpServletResponse response) th
return;
}

meth = this.getClass().getMethod(cmd, partypes);
if(getAllowedCommands().contains(cmd)) {
meth = getMethod(cmd, partypes);
} else if (UtilMethods.isSet(cmd)){
Logger.error(this.getClass(), String.format("Attempt to run Invalid command %s :",cmd));
return;
}

} catch (Exception e) {

try {
cmd = "action";
meth = this.getClass().getMethod(cmd, partypes);
meth = getMethod(cmd, partypes);
} catch (Exception ex) {
Logger.error(this.getClass(), "Trying to run method:" + cmd);
Logger.error(this.getClass(), e.getMessage(), e.getCause());
Expand All @@ -49,6 +60,18 @@ public void service(HttpServletRequest request, HttpServletResponse response) th

}

/**
* extracted as a separated method, so it can be verified when called
* @param method
* @param parameterTypes
* @return
* @throws NoSuchMethodException
*/
@VisibleForTesting
Method getMethod(String method, Class<?>... parameterTypes ) throws NoSuchMethodException {
return this.getClass().getMethod(method, parameterTypes);
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems the method is repeat

}

public void writeError(HttpServletResponse response, String error) throws IOException {
String ret = null;

Expand Down
26 changes: 17 additions & 9 deletions dotCMS/src/main/java/com/dotmarketing/portlets/workflows/ajax/WfRoleStoreAjax.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
package com.dotmarketing.portlets.workflows.ajax;

import com.fasterxml.jackson.databind.DeserializationFeature;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.dotmarketing.business.APILocator;
import com.dotmarketing.business.Role;
import com.dotmarketing.business.RoleAPI;
Expand All @@ -10,20 +8,19 @@
import com.dotmarketing.portlets.workflows.model.WorkflowAction;
import com.dotmarketing.util.Logger;
import com.dotmarketing.util.UtilMethods;
import com.fasterxml.jackson.databind.DeserializationFeature;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.liferay.portal.language.LanguageException;
import com.liferay.portal.language.LanguageUtil;
import com.liferay.portal.model.User;
import com.liferay.util.StringPool;
import java.io.IOException;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import org.apache.commons.beanutils.BeanUtils;

import javax.servlet.ServletException;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import org.apache.commons.beanutils.BeanUtils;
import java.io.IOException;
import java.util.*;

@Deprecated
public class WfRoleStoreAjax extends WfBaseAction {
Expand Down Expand Up @@ -247,4 +244,15 @@ private String rolesToJson ( List<Role> roles, boolean includeFake, boolean incl
return mapper.writerWithDefaultPrettyPrinter().writeValueAsString( m );
}

/**
* Security check demanded by Sonar
* We register all the allowed methods down here
*
* @return allowed method names
*/
@Override
protected Set<String> getAllowedCommands() {
return Set.of( "rolesToJson", "assignable", "action" );
}

}
12 changes: 12 additions & 0 deletions dotCMS/src/main/java/com/dotmarketing/portlets/workflows/ajax/WfSchemeAjax.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import java.io.IOException;
import java.util.Set;

@Deprecated
public class WfSchemeAjax extends WfBaseAction {
Expand Down Expand Up @@ -58,4 +59,15 @@ private String saveScheme(String schemeId, WorkflowSchemeForm schemeForm, User u
return "SUCCESS";
}

/**
* Security check demanded by Sonar
* We register all the allowed methods down here
*
* @return allowed method names
*/
@Override
protected Set<String> getAllowedCommands() {
return Set.of( "save", "action" );
}

}
17 changes: 13 additions & 4 deletions dotCMS/src/main/java/com/dotmarketing/portlets/workflows/ajax/WfStepAjax.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
package com.dotmarketing.portlets.workflows.ajax;

import com.dotcms.exception.ExceptionUtil;
import com.fasterxml.jackson.databind.DeserializationFeature;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.dotcms.workflow.form.WorkflowActionStepBean;
import com.dotcms.workflow.form.WorkflowStepAddForm;
import com.dotcms.workflow.form.WorkflowStepUpdateForm;
Expand All @@ -15,6 +13,8 @@
import com.dotmarketing.portlets.workflows.model.WorkflowStep;
import com.dotmarketing.util.Logger;
import com.dotmarketing.util.StringUtils;
import com.fasterxml.jackson.databind.DeserializationFeature;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.liferay.portal.model.User;

import javax.servlet.ServletException;
Expand Down Expand Up @@ -200,7 +200,16 @@ private String stepsToJson(List<WorkflowStep> steps) throws IOException{
m.put("items", list);
return mapper.writerWithDefaultPrettyPrinter().writeValueAsString(m);
}



/**
* Security check demanded by Sonar
* We register all the allowed methods down here
*
* @return allowed method names
*/
@Override
protected Set<String> getAllowedCommands() {
return Set.of( "listByScheme", "add", "addActionToStep", "delete", "reorder", "action" );
}

}
Loading