raphw / byte-buddy

Runtime code generation for the Java virtual machine.
https://bytebuddy.net
Apache License 2.0
6.22k stars 803 forks source link

byte-buddy J9 attachment only checks for self-owned directories #1628

Closed FelixMarxIBM closed 4 months ago

FelixMarxIBM commented 4 months ago

Hi @raphw , we noticed that the attachment on Linux only allows for attachments against directories owned by the current user. So far we used the normal attachment of the J9 or Semeru JDK and on Java 10+ this has always worked with the root user, even though it is mentioned in the openj9 documentation that this should only work with the current user,

Would it be possible to remove the same user check in VirtualMachine.java?

For testing purpose in our smoke-test environment we added this very basic workaround that disables the same-user check: https://github.com/instana/byte-buddy/commit/9c4990e9b6e609b8a0091ecb781d242fc650318d

We are trying to find out in which environments this limitation is actually true.

raphw commented 4 months ago

I remember that this is implemented in HotSpot. I assume that this requirement is taken from there but is not implemented. Disabling it by a property seems like a reasonable option as I think that this might be implemented in the future since it is a security concern. I add it. Did you reach out to the J9 team to see if this is planned for the future?

FelixMarxIBM commented 4 months ago

Milestone should most likely be 1.14.15 as 1.12.14 is already released. I did reach out to the J9 developers and will keep you updated.

FelixMarxIBM commented 4 months ago

I had some discussion with the J9 developers and the handling is comparable to Hotspot based JVMs. On Java versions up to including 1.9, the attacher requires the same user that also started the to-be-attached JVM. On Java 10+, this limitation does not apply and root can also attach to these JVMs. I did test this with Hotspot and Semeru based JVMs.

We have to be a bit smarter here and only omit the file check if we are not root (or not in the root group). Unfortunatly the attachInfo file does not tell us which java version is used here, so even if we could read it, we would not know upfront if the target JVM is java 9 or below and will not allow us to attach.

In our code, we do know if we are root or not and we also know the version of the target as we do a java -version before an attachment, but I guess it will be hard to move any such logic into byte-buddy directly. I guess the easiest solution will be to add a switch to turn this check off and let the outer application handle the errors.

A normal use case is for the same user and there the check does make sense.

raphw commented 4 months ago

I remember implementing this by extracting the logic from a Java 8 version of J9 and I did not revisit thereafter. If I remember correctly, HotSpot checks the chmod of the checked file, but chances are that a root-owned file is today accepted as you say.

As you said, I think it's hard to discover this from the process. I introduced an argument now that allows to controll this per process id. So if the executing process has knowledge of this, it can supply the override as an argument.

FelixMarxIBM commented 4 months ago

Thanks, I will give this a try in our smoke-test environment https://github.com/raphw/byte-buddy/commit/f04d254b61e4f05fd647fbcbcb61de32fea1d652 https://github.com/raphw/byte-buddy/commit/01ed75b4b53bc43b54d8b7814c158cf310393c68

FelixMarxIBM commented 4 months ago

I noticed that you are not forwarding the ignoreUser as part of the public static VirtualMachine attach(String processId, boolean ignoreUser) method https://github.com/raphw/byte-buddy/commit/01ed75b4b53bc43b54d8b7814c158cf310393c68#diff-f796555defc948cb31030ac3933918deee757e89f8788700db985f035dcfa212R1674-R1678

FelixMarxIBM commented 4 months ago

At least for us the current version won't work as we typically just call public static VirtualMachine attach(String processId) via reflection, so the ignoreUser flag would still be false.

FelixMarxIBM commented 4 months ago

When testing this, there is a timeout

Attaching using net.bytebuddy.agent.VirtualMachine$ForOpenJ9
Attach using net.bytebuddy.agent.VirtualMachine$ForOpenJ9
trying to look into /proc/2235675/root/tmp/.com_ibm_tools_attach/2235675
java.net.SocketTimeoutException: Accept timed out
    at java.base/java.net.PlainSocketImpl.socketAccept(Native Method)
    at java.base/java.net.AbstractPlainSocketImpl.accept(AbstractPlainSocketImpl.java:474)
    at java.base/java.net.ServerSocket.implAccept(ServerSocket.java:565)
    at java.base/java.net.ServerSocket.accept(ServerSocket.java:533)
    at net.bytebuddy.agent.VirtualMachine$ForOpenJ9.attach(VirtualMachine.java:1832)
    at net.bytebuddy.agent.VirtualMachine$ForOpenJ9.attach(VirtualMachine.java:1675)
    at net.bytebuddy.agent.VirtualMachine$ForOpenJ9.attach(VirtualMachine.java:1663)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base/java.lang.reflect.Method.invoke(Method.java:572)
    at com.instana.agent.loader.AgentLoaderAttach.attach(AgentLoaderAttach.java:398)
    at com.instana.agent.loader.AgentLoaderAttach.run(AgentLoaderAttach.java:150)
    at com.instana.agent.loader.AgentLoaderAttach.parseArgsAndRun(AgentLoaderAttach.java:100)
    at com.instana.agent.loader.AgentLoaderAttach.main(AgentLoaderAttach.java:83)

I suppose because we need to issue the following command https://github.com/eclipse-openj9/openj9/blob/264f2752c6125e2a6a4d9a4e1a99d3faa4cdc3a6/jcl/src/java.base/share/classes/openj9/internal/tools/attach/target/Reply.java#L88-L90

FelixMarxIBM commented 4 months ago

Okay, on J9, the check for isFileOwnedByUid excludes userid 0 https://github.com/eclipse-openj9/openj9/blob/master/jcl/src/java.base/share/classes/openj9/internal/tools/attach/target/CommonDirectory.java#L424-L426

    /**
     * Check if the file is owned by the UID.  Note that UID 0 "owns" all files.
     * @param f File or directory
     * @param myUid user UID. 
     * @return true if the uid owns the file or uid == 0.
     */
    public static boolean isFileOwnedByUid(File f, long myUid) {
        return (0 == myUid) || (myUid == CommonDirectory.getFileOwner(f.getAbsolutePath()));
    }
raphw commented 4 months ago

Ok, so we need to check if the owners are unequal and change it unless the current id is 0.

I can look into it, but I'd appreciate at PR of course.

FelixMarxIBM commented 4 months ago

I created #1631 to solve the problem. In our smoke test, the attachment from an agent running as root (and Hotspot) against a Semeru Java 11 running as user now works.

FelixMarxIBM commented 4 months ago

@raphw would be great if you can do another release of byte-buddy and then we can close this issue :pleading_face:

raphw commented 4 months ago

Just triggered a release.

FelixMarxIBM commented 4 months ago

We can close this issue. Our smoke tests pass with the 1.14.15 release. Thank you very much for the quick release.