I am getting alert in Checkmarx scan saying Unsafe object binding in the saveAll() call. The exact words in checkmarx are -
The columnConfigSet at src\main\java\com\ge\digital\oa\moa\controller\ConfigController.java in line 45 may unintentionally allow setting the value of saveAll in setColumnsConfig, in the object src\main\java\com\ge\digital\oa\moa\service\ConfigService.java at line 170.
Any idea how to rewrite the code , so that the checkmarx stops complaining.
My code:
@PutMapping("/columns")
@ResponseStatus(OK)
public void setColumnsConfig(@RequestBody(required=true) ColumnConfigSetDto columnConfigSet) {
service.setColumnsConfig(columnConfigSet);
}
public void setColumnsConfig(ColumnConfigSetDto columnConfigSet) {
String userId = columnConfigSet.getUserId();
String viewName = columnConfigSet.getViewName();
List<ColumnConfig> configs = new ArrayList<>();
for (ColumnConfigDto colConfig : columnConfigSet.getColumns()) {
// build a db config row only for the visibility property for now
ColumnConfigId confId = new ColumnConfigId();
confId.setUserId(userId);
confId.setViewName(viewName);
confId.setKey(colConfig.getKey());
confId.setProperty("visible");
ColumnConfig conf = new ColumnConfig();
conf.setColumnConfigId(confId);
conf.setValue(colConfig.getIsVisible() ? "true" : "false" );
configs.add(conf);
}
if (!configs.isEmpty()) {
configRepo.saveAll(configs);
}
}
Below are my DTO Objects which is used in this code :
@Getter
@Setter
public class ColumnConfigSetDto {
@JsonProperty("userId")
private String userId;
@JsonProperty("viewName")
private String viewName;
@JsonProperty("columns")
private List<ColumnConfigDto> columns;
}
Below are my DTO code which is used in this
@Getter
@Setter
public class ColumnConfigDto {
@JsonProperty("key")
private String key;
@JsonProperty("label")
private String label;
@JsonProperty("isVisible")
private Boolean isVisible;
@JsonProperty("position")
private Integer position;
@JsonProperty("isSortable")
private Boolean isSortable;
@JsonProperty("isHideable")
private Boolean isHideable;
}
3 Answers
Here is my solution for Unsafe object binding reported by cherkmarx in Java. It's not a graceful approach and only fix this vulnerability.
Remove all setter methods for boxed fields in each requestbody bean.
Since @JsonProperty could support deserialization capbility, no need to add setter manually.
If you need setter for request body bean indeed, you can use reflaction way instead.
FieldUtils.writeField(columnConfigDto , "isVisible", true, true);
public class ColumnConfigDto {
// Ensure @JsonProperty existed on each field
@JsonProperty("key")
private String key;
@JsonProperty("isVisible")
private Boolean isVisible;
@JsonProperty("list")
private List list;
public String getKey() {
return key;
}
public void setKey(String key) {
this.key = key;
}
public Boolean getVisible() {
return isVisible;
}
// Remove boxed type field
// public void setVisible(Boolean visible) {
// isVisible = visible;
// }
public List getList() {
return list;
}
// Remove boxed type field
// public void setList(List list) {
// this.list = list;
// }
}
this issue occurs due to @RequestBoby as per spring documentation but there is no issue for @RequestParam. if we bind request body to object without @RequestBody, this issue is not occurred.
HttpServletRequest request;
mapper.readValue(request.getInputStream(), Product.class);
The error is also thrown if data is set to an object annotated with @RequestBody.
requestBodyVariable.setAdditionalValue(valueFromRequestParamOrPathVariable);
// This setter call should not be used
Instead, use a user-defined variable for storing the value from request param, header or path variable in its place:
service.callServiceMethod(requestBodyVariable, valueFromRequestParamOrPathVariable);
