WW-5288 Make excluded package exemption logic more strict #664
Conversation
| for (Pattern pattern : excludedPackageNamePatterns) { | ||
| if (pattern.matcher(targetPackageName).matches() || pattern.matcher(memberPackageName).matches()) { | ||
| for (Class<?> clazz : classesToCheck) { | ||
| if (!isExcludedPackageExempt(clazz) && (isExcludedPackageNamePatterns(clazz) || isExcludedPackageNames(clazz))) { |
There was a problem hiding this comment.
Exemption must now exist for both classes (target and member) if they both match a banned package
| } | ||
|
|
||
| protected boolean isExcludedPackageExempt(Class<?> clazz) { | ||
| return excludedPackageExemptClasses.stream().anyMatch(clazz::equals); |
There was a problem hiding this comment.
Do not exempt superclasses (use #equals instead of #isAssignableFrom)
| final Class<?> memberClass = member.getDeclaringClass(); | ||
| // target can be null in case of accessing static fields, since OGNL 3.2.8 | ||
| final Class<?> targetClass = Modifier.isStatic(memberModifiers) ? memberClass : target.getClass(); | ||
| if (!memberClass.isAssignableFrom(targetClass)) { |
There was a problem hiding this comment.
Added some validation here and in #isPackageExcluded to ensure unit tests made sense
| @@ -413,10 +413,13 @@ public final class StrutsConstants { | |||
| /** Allows override default DispatcherErrorHandler */ | |||
| public static final String STRUTS_DISPATCHER_ERROR_HANDLER = "struts.dispatcher.errorHandler"; | |||
|
|
|||
| /** Comma delimited set of excluded classes and package names which cannot be accessed via expressions */ | |||
| /** Comma delimited set of excluded classes which cannot be accessed via OGNL expressions. Matching is done on both target and member classes of OGNL expression. Note that superclasses of listed classes are also used for matching. */ | |||
There was a problem hiding this comment.
Tried my best to describe the code logic in words for all these options.
|
|
||
| @Override | ||
| protected void assignNewSma(boolean allowStaticFieldAccess) { | ||
| sma = new ExternalSecurityMemberAccess(allowStaticFieldAccess); |
There was a problem hiding this comment.
We have two SecurityMemberAccessTest in two different packages to test two different behaviours. One test is in the package com.opensymphony.xwork2.ognl which is excluded.
@lukaszlenart Do you think this is an okay solution?
There was a problem hiding this comment.
Nope :) Yet testing the MemberAccess behaviour for different settings makes sense, this is the last line of defence :)
|
@yasserzamani @JCgH4164838Gh792C124B5 any thoughts? |
|
Do you plan to document this option? |
I'll create a PR :) |
WW-5288
See Jira card description then PR comments below.