Replacing SecurityManager with a tracking agent #3386

jlahoda · 2021-12-26T08:56:14Z

This is a prototype of an attempt to replace the deprecated SecurityManager with a custom agent-based hooks.

What functionality is kept:
-preventing inadvertent System.exit
-tracking file I/O (probably the most complex part, might be missing some hooks)
-checking for deprecated system property access
-checking calls to setAccessible to open Unsafe (this didn't work on JDK 8+, but can be fixed)
-checking/preventing calls to System.setSecurityManager. Possibly no longer that critical, as it is not possible to disable the hooks using this method.
-tracking new Window creation (used by internal execution)

What functionality is not kept:
-checkConnect for connecting over network. This didn't seem to be actually used, as the original code was:

        try {
            checkPermission(allPermission);
            return;
        } catch (SecurityException e) {
        }
//proceed with the actual check

But checkPermission seems to always(?) pass, so the check was never performed
-checkLogger, which was disabled anyway
-tweaks related to installation of a custom Swing clipboard - these appear to exists for JDK-4818143, which is marked as fixed, so presumably not needed anymore.

When looking at the patch, TrackingAgent is the agent class that does various transformations to install hooks, TrackingHooks are the callback, TopSecurityManager (o.n.bootstrap), FileChangedManager (masterfs), SecMan (core.execution) are the actual uses of the hooks.

mbien · 2021-12-27T09:02:11Z

this is pretty cool!

…ually used.

JaroslavTulach

Magnificent work.

I'd prefer if the code was packaged as NetBeans module for ease of tooling as well as packaging and deploying. With proper Javadoc it could even be used as a great standalone library.

Inviting @sdedic as a reviewer. I'd say Sváťa is long time NetBeans expert when it comes to bytecode patching.

JaroslavTulach · 2022-02-02T04:39:26Z

platform/o.n.bootstrap/launcher/unix/nbexec

@@ -192,7 +192,7 @@ fi
 # rename old heap dump to .old
 mv "${userdir}/var/log/heapdump.hprof" "${userdir}/var/log/heapdump.hprof.old" > /dev/null 2>&1

-jargs_without_clusters="$jargs -Djava.security.manager=allow"
+jargs_without_clusters="$jargs -Djava.security.manager=allow -javaagent:${plathome}/core/tracking-agent.jar -Xbootclasspath/a:${plathome}/core/tracking-hooks.jar"


By default all JARs in core directory are added to fixed modules classpath. Does this mean we have to tracking-agent.jar used three times?

put on bootclasspath

used as javaagent

added on fixed module classpath

JaroslavTulach · 2022-02-02T04:41:46Z

platform/o.n.bootstrap/nbproject/org-netbeans-bootstrap.sig

@@ -558,27 +499,17 @@ meth public abstract void flushCaches(java.io.DataOutputStream) throws java.io.I

 CLSS public org.netbeans.TopSecurityManager


Keeping the TopSecurityManager class and replacing its superclass is one option. I'd like to ask @sdedic for his opinion: there is a support for patching classes and even for changing superclass - possibly we could use it here? If we want to be really compatible? Do we want to?

Given this is a friend API, I don't see such a strong need to keep compatibility, esp. in an aspect that will cease to work at some point.

JaroslavTulach · 2022-02-02T04:45:47Z

ide/mercurial/nbproject/project.properties

@@ -26,3 +26,5 @@ test.qa-functional.cp.extra=${openide.nodes.dir}/modules/org-openide-nodes.jar:\
 test.config.stable.includes=**/hgStableTest.class
 # #178009
 disable.qa-functional.tests=true
+
+test.unit.cp.extra=${nb_all}/platform/o.n.bootstrap/netbeans-tracking-agent/tracking-hooks/dist/tracking-hooks.jar


Rather than using test.unit.cp.extra I'd prefer making tracing-hooks real module of the platform. Then we can:

have test/regular dependencies when using it

packaging into NBM & Maven repository is straighforward

Is there any downside of having the functionality as a NetBeans module?

JaroslavTulach · 2022-02-02T04:48:12Z

platform/core.execution/src/org/netbeans/core/execution/Install.java

@@ -88,7 +89,7 @@
    private static final Logger LOG = Logger.getLogger(Install.class.getName());

    public @Override void run() {
-        TopSecurityManager.register(SecMan.DEFAULT);
+        TrackingHooks.register(SecMan.DEFAULT, 100, TrackingHooks.HOOK_EXIT, TrackingHooks.HOOK_NEW_AWT_WINDOW);


Would it be possible to have declarative registration of the TracingHooks? E.g. Lookup.getDefault() or ServiceLoader.load instead of register method?

Seems fairly complex, as there are uses that actively use the pre-existing register/unregister mechanism in TopSecurityManager:

netbeans/platform/core.execution/src/org/netbeans/core/execution/Install.java

Line 91 in ebb8086

TopSecurityManager.register(SecMan.DEFAULT);

netbeans/platform/core.execution/src/org/netbeans/core/execution/Install.java

Line 100 in ebb8086

TopSecurityManager.unregister(SecMan.DEFAULT);

It is not clear to me how to replace that with a Lookup-based registration. But if you can provide a patch to resolve all existing uses of TopSecurityManager.register/unregister, then we can switch to a Lookup-based registration.

JaroslavTulach · 2022-02-02T04:49:14Z

platform/core.execution/test/unit/src/org/netbeans/core/execution/CloseTest.java

+        File wd = getWorkDir();
+        File classes = new File(wd, "classes");
+        classes.mkdirs();
+        CompilationTask compilation = ToolProvider.getSystemJavaCompiler().getTask(null, null, null, Arrays.asList("-classpath", CloseTest.class.getProtectionDomain().getCodeSource().getLocation().getPath(), "-d", classes.getAbsolutePath()), null, Arrays.asList(new SimpleJavaFileObject(new URI("mem://Test.java"), Kind.SOURCE) {


What a test!

JaroslavTulach · 2022-02-02T04:52:25Z

platform/o.n.bootstrap/build.xml

@@ -38,4 +40,13 @@
            <compilerarg line="${javac.compilerargs}"/>
        </javac>
    </target>
+    <target name="-build-hooks">


I'd prefer to put the tracing agent/hooks into its own module.

JaroslavTulach · 2022-02-02T04:53:41Z

platform/o.n.bootstrap/netbeans-tracking-agent/tracking-agent/nbproject/build-impl.xml

@@ -0,0 +1,1789 @@
+<?xml version="1.0" encoding="UTF-8"?>


Avoiding old Ant projects in NetBeans code base would make me happier.

JaroslavTulach · 2022-02-02T04:56:36Z

...n.bootstrap/netbeans-tracking-agent/tracking-agent/src/org/netbeans/agent/TrackingAgent.java

+                            additionalConstantPool.write(data);
+                        }
+
+                        int maxStack = readShort(classfileBuffer, pp); pp += 2;


Patching classfile manually by hand! @sdedic has already introduced asm.jar into the platform. It'd be good to consider some synergy among this and his code.

On one hand, it would be easier to use ASM. On the other hand, ASM cannot handle new classfiles - we could workaround that by downgrading the versions of the classfiles sent to ASM; it would also mean to place ASM into the agent path, or doing some trampoline from the agent to a normal module. I guess I'd try with the handcoded bytecode patcher, and we'll see what are the experiences with that.

JaroslavTulach · 2022-02-02T04:59:33Z

...strap/netbeans-tracking-agent/tracking-hooks/src/org/netbeans/agent/hooks/TrackingHooks.java

+
+    protected void checkFileWrite(String path) {}
+    protected void checkFileRead(String path) {}
+    protected void checkDelete(String path) {}


This class is mixing API and SPI. Wouldn't it be cleaner to separate the two concepts into own files?

JaroslavTulach · 2022-02-02T05:05:08Z

nbbuild/templates/common.xml

+            <available file="${nbplatform.active.dir}/platform/core" type="dir" />
+        </condition>
+        <property name="test.tracking.agent.dir" location="${o.n.bootstrap.dir}/core/" />
+        <property name="test.tracking.agent.args" value="-javaagent:${test.tracking.agent.dir}/tracking-agent.jar -Xbootclasspath/a:${test.tracking.agent.dir}/tracking-hooks.jar"/>


Having to manually modify -Xbootclasspath is annoying. Can't agent modify classpath programatically? There are methods like appendToBootstrap....

jlahoda · 2022-08-17T05:53:30Z

I wonder if there are any further comments on this. It might be reasonable to try to avoid too much dependency on the SecurityManager.

matthiasblaesing · 2022-08-18T17:40:58Z

Given that we are early in the NB16 cycle, now would be a good time to merge this if ready.

sdedic · 2022-08-18T19:14:02Z

harness/nbjunit/test/unit/src/org/netbeans/junit/NbModuleSuiteTest.java

@@ -232,25 +232,27 @@ public void testAccessClassPathDefinedAutoload() {
    public void testModulesForCL() throws Exception {
        Set<String> s = NbModuleSuite.S.findEnabledModules(ClassLoader.getSystemClassLoader());
        s.remove("org.netbeans.modules.nbjunit");
-        assertEquals("Four modules left: " + s, 5, s.size());
+        assertEquals("Four modules left: " + s, 6, s.size());


four > six ?

sdedic · 2022-08-18T19:17:56Z

platform/core.execution/src/org/netbeans/core/execution/Install.java

@@ -88,7 +88,7 @@ public class Install implements Runnable {
    private static final Logger LOG = Logger.getLogger(Install.class.getName());

    public @Override void run() {
-        TopSecurityManager.register(SecMan.DEFAULT);
+        TrackingHooks.register(SecMan.DEFAULT, 100, TrackingHooks.Hooks.EXIT, Hooks.NEW_AWT_WINDOW);


Nitpick: TrackingHooks. redundant.

sdedic · 2022-08-18T19:27:07Z

platform/o.n.agent.hooks/nbproject/project.properties

+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+javac.source=1.8


sdedic · 2022-08-18T19:29:39Z

platform/o.n.agent.hooks/src/org/netbeans/agent/hooks/TrackingHooksCallback.java

+            for (Iterator<HookDescription> it = delegates.iterator(); it.hasNext();) {
+                HookDescription hd = it.next();
+                if (hd.hooks == toRemove) {
+                    it.remove();


stop after first removed ?

sdedic · 2022-08-19T06:38:39Z

platform/o.n.agent/src/org/netbeans/agent/TrackingAgent.java

+                return classfileBuffer;
+            }
+            try {
+                List<MethodEnhancement> thisClassEnhancements = toInject.stream().filter(me -> {/*System.err.println("className=" + className); */return className.equals(me.className);}).collect(Collectors.toList());


Wouldn't it be better to organize toInject as a Map keyed by FQN ? It would allow faster lookup here. Also not sure about effectivness of the stream filter approach for that small number of items.

sdedic · 2022-08-19T06:40:04Z

platform/o.n.agent/src/org/netbeans/agent/TrackingAgent.java

+                        constantPool.add(null);
+                        break;
+                    default:
+                        System.err.println("unknown constant pool tag: " + tag);


Use Logger.

sdedic · 2022-08-19T06:47:22Z

platform/o.n.agent/src/org/netbeans/agent/TrackingAgent.java

+            System.arraycopy(classfileBuffer, lastCopySource, newBuffer, lastCopyDest, len); lastCopySource += len; lastCopyDest += len;
+            return newBuffer;
+            } catch (Throwable t) {
+                t.printStackTrace();


Use logger; it is really useful to catch all Throwables ?

sdedic · 2022-08-19T06:57:30Z

platform/o.n.bootstrap/src/org/netbeans/TopSecurityManager.java

+            Class<?> stackWalker = Class.forName("java.lang.StackWalker");
+            Class<?> stackWalkerOption = Class.forName("java.lang.StackWalker$Option");
+            Class<?> stackWalkerStackFrame = Class.forName("java.lang.StackWalker$StackFrame");
+            Method getInstance = stackWalker.getDeclaredMethod("getInstance", stackWalkerOption);


Perhaps this reflective lookups could be done just once ?

sdedic · 2022-08-19T11:51:15Z

platform/o.n.core/src/org/netbeans/core/NbLifecycleManager.java

@@ -67,9 +67,6 @@ public static void advancePolicy() {
        if (!Boolean.getBoolean("TopSecurityManager.disable")) {
            // set security manager
            TopSecurityManager.install();
-            if (CLIOptions.isGui()) {
-                TopSecurityManager.makeSwingUseSpecialClipboard(Lookup.getDefault().lookup(ExClipboard.class));


What was this supposed to do ?

I believe this was part of a workaround for JDK-4818143. That bug is marked as fixed in JDK 6. So, it seems that keeping the workaround is probably not useful anymore (and, in case the problem would not be actually fixed, this is a way to find out).

Doesn't that also stop the system clipboard being an instance of ExClipboard? If so, seem a bunch of places that expect it to be one.

.travis.yml

mbien · 2022-10-28T19:38:51Z

all green. excellent!

mbien · 2023-05-09T04:31:31Z

JEP 451: Prepare to Disallow the Dynamic Loading of Agents
will require the -XX:+EnableDynamicAgentLoading flag (sooner or later, will print warnings first).

It might be better to load the agent using JVM flags on startup (instead of dynamically), since there are probably good reasons JEPs like that exist.

neilcsmith-net · 2023-05-09T09:21:08Z

@mbien isn't it already doing this - https://github.com/apache/netbeans/pull/3386/files#diff-d4b605d5fdbc82d1afa51dc1d6c3ce44516cf46c5e2ac0c79a98118bdcefb896R195

Or are there other uses in there? I assume we'll need a Windows launcher update too? Wish that could be a script too!

jlahoda added 14 commits June 14, 2021 07:06

An idea on a possible SecurityManager replacement.

bb66484

Merge branch 'master' into tracking-agent

61720cc

Some progress on the tracking agent.

8ec4e04

Improvements.

fbbfb33

Cleanup, build fixes, test fixes.

b7d2eb6

Cleanup

35e9ca1

More test cleanup.

4d211fa

Merge.

21a33cf

More test fixing.

bc490ff

Test fixing.

b50fdcb

Cleanup.

198dc11

Cleanup.

d92fb78

More test fixing.

360dd51

More fixes.

efb9465

jlahoda added 7 commits December 28, 2021 14:43

Merge branch 'master' into tracking-agent

a082fee

Cleanup

e7e2024

The Logger may be GCed after the logging is set, but before it is act…

43b6c0b

…ually used.

Attempting to compile the agent early enough.

35731ec

Avoiding setSecurityManager in FileSystem checks.

7db6043

Adding forgotten files.

9a0db22

Fixing cp.extra.

9240bd7

neilcsmith-net mentioned this pull request Jan 27, 2022

Startup Warnings #3519

Closed

3 tasks

JaroslavTulach requested review from sdedic, neilcsmith-net and JaroslavTulach February 2, 2022 04:42

JaroslavTulach reviewed Feb 2, 2022

View reviewed changes

jlahoda added 3 commits February 12, 2022 20:20

Moving agent and hooks into real modules.

21a5141

Cleanup.

b5d63df

Cleanup - separating callbacks from the SPI, adding friends.

55ffef1

jlahoda added 3 commits August 7, 2022 12:31

Cleanup.

4e8106c

Fixing test.

a2b474f

Fixing test.

feb8735

sdedic reviewed Aug 19, 2022

View reviewed changes

jlahoda added 2 commits August 23, 2022 09:03

Reflecting review feedback.

0fc0808

Using NetBeans clipboard for Swing transfer actions.

8114562

eirikbakke mentioned this pull request Sep 20, 2022

This is just a naive attempt to fix Windows Clipboard #4572

Closed

jlahoda added 5 commits September 24, 2022 07:27

Reinstanting NbClipboardIsUsedBySwingComponentsTest.java.

f2dd8e3

Resolving conflicts.

3ed9e31

Cleanup

b4e5237

Cleanup.

6ebd546

Fixing test.

777f51e

mbien added Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) Platform [ci] enable platform tests (platform/*) ci:all-tests [ci] enable all tests labels Oct 26, 2022

mbien reviewed Oct 26, 2022

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

jlahoda added 2 commits October 28, 2022 11:33

Merging master into tracking-agent

c30c07c

Testing the agent on various JDK versions.

d667ee6

Whitelisting setAccessible class

888df0b

neilcsmith-net mentioned this pull request Apr 27, 2023

prepare NB 19 release cycle for JDK 11. #5852

Merged

mbien mentioned this pull request Feb 8, 2024

File Chooser dialog opens with long delay. #3919

Open

mbien mentioned this pull request Oct 31, 2024

Remove SecurityManager layer #7928

Merged

neilcsmith-net mentioned this pull request Nov 29, 2024

netbeans 24 will not start on java 24-ea due to security manager permanently disabled. #8008

Closed

mbien mentioned this pull request Jan 17, 2025

NB config: disable TopSecurityManager and enable exit logging #8169

Merged

neilcsmith-net mentioned this pull request Feb 24, 2025

Attempted clipboard issue workaround on Windows #8277

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replacing SecurityManager with a tracking agent #3386

Replacing SecurityManager with a tracking agent #3386

jlahoda commented Dec 26, 2021 •

edited

Loading

mbien commented Dec 27, 2021

JaroslavTulach left a comment

JaroslavTulach Feb 2, 2022

JaroslavTulach Feb 2, 2022

jlahoda Mar 26, 2022

JaroslavTulach Feb 2, 2022

JaroslavTulach Feb 2, 2022

jlahoda Mar 26, 2022

JaroslavTulach Feb 2, 2022

JaroslavTulach Feb 2, 2022

JaroslavTulach Feb 2, 2022

JaroslavTulach Feb 2, 2022

jlahoda Mar 26, 2022

JaroslavTulach Feb 2, 2022

JaroslavTulach Feb 2, 2022

jlahoda commented Aug 17, 2022

matthiasblaesing commented Aug 18, 2022

sdedic Aug 18, 2022

sdedic Aug 18, 2022

sdedic Aug 18, 2022

sdedic Aug 18, 2022

sdedic Aug 19, 2022

sdedic Aug 19, 2022

sdedic Aug 19, 2022

sdedic Aug 19, 2022

sdedic Aug 19, 2022

jlahoda Aug 21, 2022

neilcsmith-net Sep 1, 2022

mbien commented Oct 28, 2022

mbien commented May 9, 2023

neilcsmith-net commented May 9, 2023

		@@ -558,27 +499,17 @@ meth public abstract void flushCaches(java.io.DataOutputStream) throws java.io.I

		CLSS public org.netbeans.TopSecurityManager

Replacing SecurityManager with a tracking agent #3386

Are you sure you want to change the base?

Replacing SecurityManager with a tracking agent #3386

Conversation

jlahoda commented Dec 26, 2021 • edited Loading

mbien commented Dec 27, 2021

JaroslavTulach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlahoda commented Aug 17, 2022

matthiasblaesing commented Aug 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbien commented Oct 28, 2022

mbien commented May 9, 2023

neilcsmith-net commented May 9, 2023

jlahoda commented Dec 26, 2021 •

edited

Loading