Stop using ValidationSecurityManager as SecurityManager #1983
Conversation
There was a problem hiding this comment.
Is
spotbugs/spotbugs/src/main/java/edu/umd/cs/findbugs/PluginLoader.java
Lines 1421 to 1432 in 6a0dc03
| System.out.println("Checking for " + perm + " permission in thread " + Thread.currentThread().getName()); | ||
| } | ||
| if (performingValidation.get() && inValidation()) { | ||
| SecurityException e = new SecurityException("No permissions granted while performing JSR-305 validation"); |
There was a problem hiding this comment.
Do I understand correctly that under some conditions, analyzing bytecode could result in some application classes being loaded in the SpotBugs process and potentially running arbitrary code?
There was a problem hiding this comment.
It looks like it sandboxes the validation to prevent either the validator or the validated code to do unexpected things.
Yes and it might also still be useful for those using SpotBugs with Java 8. |
wborn
force-pushed
the
remove-securitymanager
branch
from
c1f35d5
to
0ab9419
Compare
| @@ -148,15 +148,6 @@ private TypeQualifierValue(ClassDescriptor typeQualifier, @CheckForNull Object v | |||
| try { | |||
| Global.getAnalysisCache().getClassAnalysis(ClassData.class, checkerName); | |||
|
|
|||
| // found it. | |||
There was a problem hiding this comment.
Isn't it the better approach, to use env variable to on/off it,? This bug is just for "recent" jdk's only.
There was a problem hiding this comment.
Maybe the maintainers can tell if we can part with the functionality provided by the ValidationSecurityManager. Does it solve an issue you have?
There was a problem hiding this comment.
🤔 I'm not so sure about why we need to set a security manager here. The comment on 4a9eaf4 gave no hint.
At least existing test cases got no effect from this deletion. It also has already been disabled when we run SpotBugs as an IDE plugin. So I think it's OK to remove it.
😊 @spotbugs/core-devs please share your thought if you have.
BTW SpotBugs usually uses system property instead of environment variables, to switch behavior. Just like the DEBUG_CLASSLOADING variable in this class. So if we need a feature toggle, better to use a system property.
There was a problem hiding this comment.
Would it make sense to check the JVM version before calling, and only call on older platforms where it makes sense (like Java 8 as mentioned above)?
The Security Manager is deprecated and subject to removal in a future release. There is no replacement for the Security Manager. See also spotbugs#1579
wborn
force-pushed
the
remove-securitymanager
branch
from
0ab9419
to
efee878
Compare
|
Merged, thanks for your fix! |
…ity with JDK 17
Log contains messages such as
(file:/home/runner/.m2/repository/com/github/spotbugs/spotbugs/4.2.2/spotbugs-4.2.2.jar)
[java] WARNING: Please consider reporting this to the maintainers of edu.umd.cs.findbugs.ba.jsr305.TypeQualifierValue
[java] WARNING: System::setSecurityManager will be removed in a future release
[java] WARNING: A terminally deprecated method in java.lang.System has been called
[java] WARNING: System::setSecurityManager has been called by edu.umd.cs.findbugs.ba.jsr305.TypeQualifierValue
This is fixed by spotbugs/spotbugs#1983 in 4.7.0
Spotbugs 4.7.3 release notes:
https://github.com/spotbugs/spotbugs/releases/tag/4.7.3
The Security Manager is deprecated and subject to removal in a future release. There is no replacement for the Security Manager.
See also #1579