Re: [External] : Re: [openjdk/jdk] 8327114: Attach in Linux may have wrong behaviour when pid == ns_pid (Kubernetes debug container) (PR #19055)

2024-05-04 Thread Laurence Cable 

so I think to summarize the logic we require:

1) if we can determine that the attacher and attachee occupy the same 
mnt ns (/proc//ns/mnt == /proc//ns/mnt), return "/tmp"

2) if they are not in the same mnt ns:
    - test the /proc//root/tmp path for readability, if it 
is, return that
    - if it is not readable (its either secured with elevated privs and 
it exists) return "/tmp" which may still fail because they are in fact 
not in the same mnt ns


what about /proc//cwd?

- Larry


On 5/3/24 9:43 AM, Sebastian Lövdahl wrote:


Thanks for the patch @larry-cable 
, 
much appreciated! I really like this idea.


I tried it out a bit locally. These cases seem to work:

  * attaching to a process running on the same host (PID and mount
namespace the same)
  * attaching as root from the host to a JVM inside a container
  * attaching from a sidecar container to a JVM in another container

Unfortunately, attaching to a JVM process running as the same user in 
the same PID and mount namespace but one that has elevated privileges 
no longer works (the case that JDK-8226919 fixed). That case ends up 
failing like this with the patch:


|slovdahl@ubuntu2204:~/reproducer$ 
/home/slovdahl/dev/external/jdk/build/linux-x86_64-server-release/images/jdk/bin/jcmd 
1751545 VM.version 1751545: java.io.IOException: Unable to access root 
directory /proc/1751545/root of target process 1751545 at 
jdk.attach/sun.tools.attach.VirtualMachineImpl.findTargetProcessTmpDirectory(VirtualMachineImpl.java:284) 
at 
jdk.attach/sun.tools.attach.VirtualMachineImpl.findSocketFile(VirtualMachineImpl.java:229) 
at 
jdk.attach/sun.tools.attach.VirtualMachineImpl.(VirtualMachineImpl.java:86) 
at 
jdk.attach/sun.tools.attach.AttachProviderImpl.attachVirtualMachine(AttachProviderImpl.java:58) 
at 
jdk.attach/com.sun.tools.attach.VirtualMachine.attach(VirtualMachine.java:207) 
at jdk.jcmd/sun.tools.jcmd.JCmd.executeCommandForPid(JCmd.java:113) at 
jdk.jcmd/sun.tools.jcmd.JCmd.main(JCmd.java:97) |


I /think/ it boils down to the same reason as why the fix for 
JDK-8226919 was needed in the first place - a non-root user cannot 
read the symlinks in |/proc//ns| for a process running with more 
privileges even though it's run by the same non-root user.


|slovdahl@ubuntu2204:/proc/1751545/ns$ ls -lh ls: cannot read symbolic 
link 'net': Permission denied ls: cannot read symbolic link 'uts': 
Permission denied ls: cannot read symbolic link 'ipc': Permission 
denied ls: cannot read symbolic link 'pid': Permission denied ls: 
cannot read symbolic link 'pid_for_children': Permission denied ls: 
cannot read symbolic link 'user': Permission denied ls: cannot read 
symbolic link 'mnt': Permission denied ls: cannot read symbolic link 
'cgroup': Permission denied ls: cannot read symbolic link 'time': 
Permission denied ls: cannot read symbolic link 'time_for_children': 
Permission denied total 0 lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:28 
cgroup lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:28 ipc lrwxrwxrwx 1 
slovdahl slovdahl 0 maj 3 19:27 mnt lrwxrwxrwx 1 slovdahl slovdahl 0 
maj 3 19:28 net lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:28 pid 
lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:28 pid_for_children 
lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:28 time lrwxrwxrwx 1 
slovdahl slovdahl 0 maj 3 19:28 time_for_children lrwxrwxrwx 1 
slovdahl slovdahl 0 maj 3 19:28 user lrwxrwxrwx 1 slovdahl slovdahl 0 
maj 3 19:28 uts slovdahl@ubuntu2204:/proc/1751545/ns$ sudo ls -lh 
total 0 lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:28 cgroup -> 
'cgroup:[4026531835]' lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:28 ipc 
-> 'ipc:[4026531839]' lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:27 mnt 
-> 'mnt:[4026533862]' lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:28 net 
-> 'net:[4026531840]' lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:28 pid 
-> 'pid:[4026531836]' lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:28 
pid_for_children -> 'pid:[4026531836]' lrwxrwxrwx 1 slovdahl slovdahl 
0 maj 3 19:28 time -> 'time:[4026531834]' lrwxrwxrwx 1 slovdahl 
slovdahl 0 maj 3 19:28 time_for_children -> 'time:[4026531834]' 
lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:28 user -> 
'user:[4026531837]' lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:28 uts 
-> 'uts:[4026531838]' |


FWIW, my IntelliJ also highlighted the fact that the suggested patch 
contains unreachable code. The |else throw new 
IOException(String.format("target process:%d and this do not share 
common mount namespace for: %s attach failed", pid, tmpdir));| path 
can never be taken, because either the |if| statement evaluates to 
|true|, or the |else if|.


I'm not sure if we can do any better than always falling back to 
|/tmp|? But if anyone has suggestions, I'm certainly happy to try it out.


—
Reply to this email directly, view it on GitHub

Re: [External] : Re: [openjdk/jdk] 8327114: Attach in Linux may have wrong behaviour when pid == ns_pid (Kubernetes debug container) (PR #19055)

2024-05-06 Thread Sebastian Lövdahl 
Yep, that sounds reasonable. I'll try to work something out along these 
lines, thanks for the input!


Unfortunately, /proc//cwd is also restricted in the same way 
as /proc//root is.


/Sebastian

On 2024-05-05 00:06, Laurence Cable wrote:

so I think to summarize the logic we require:

1) if we can determine that the attacher and attachee occupy the same 
mnt ns (/proc//ns/mnt == /proc//ns/mnt), return "/tmp"

2) if they are not in the same mnt ns:
    - test the /proc//root/tmp path for readability, if it 
is, return that
    - if it is not readable (its either secured with elevated privs 
and it exists) return "/tmp" which may still fail because they are in 
fact not in the same mnt ns


what about /proc//cwd?

- Larry


On 5/3/24 9:43 AM, Sebastian Lövdahl wrote:


Thanks for the patch @larry-cable 
, 
much appreciated! I really like this idea.


I tried it out a bit locally. These cases seem to work:

  * attaching to a process running on the same host (PID and mount
namespace the same)
  * attaching as root from the host to a JVM inside a container
  * attaching from a sidecar container to a JVM in another container

Unfortunately, attaching to a JVM process running as the same user in 
the same PID and mount namespace but one that has elevated privileges 
no longer works (the case that JDK-8226919 fixed). That case ends up 
failing like this with the patch:


|slovdahl@ubuntu2204:~/reproducer$ 
/home/slovdahl/dev/external/jdk/build/linux-x86_64-server-release/images/jdk/bin/jcmd 
1751545 VM.version 1751545: java.io.IOException: Unable to access 
root directory /proc/1751545/root of target process 1751545 at 
jdk.attach/sun.tools.attach.VirtualMachineImpl.findTargetProcessTmpDirectory(VirtualMachineImpl.java:284) 
at 
jdk.attach/sun.tools.attach.VirtualMachineImpl.findSocketFile(VirtualMachineImpl.java:229) 
at 
jdk.attach/sun.tools.attach.VirtualMachineImpl.(VirtualMachineImpl.java:86) 
at 
jdk.attach/sun.tools.attach.AttachProviderImpl.attachVirtualMachine(AttachProviderImpl.java:58) 
at 
jdk.attach/com.sun.tools.attach.VirtualMachine.attach(VirtualMachine.java:207) 
at jdk.jcmd/sun.tools.jcmd.JCmd.executeCommandForPid(JCmd.java:113) 
at jdk.jcmd/sun.tools.jcmd.JCmd.main(JCmd.java:97) |


I /think/ it boils down to the same reason as why the fix for 
JDK-8226919 was needed in the first place - a non-root user cannot 
read the symlinks in |/proc//ns| for a process running with more 
privileges even though it's run by the same non-root user.


|slovdahl@ubuntu2204:/proc/1751545/ns$ ls -lh ls: cannot read 
symbolic link 'net': Permission denied ls: cannot read symbolic link 
'uts': Permission denied ls: cannot read symbolic link 'ipc': 
Permission denied ls: cannot read symbolic link 'pid': Permission 
denied ls: cannot read symbolic link 'pid_for_children': Permission 
denied ls: cannot read symbolic link 'user': Permission denied ls: 
cannot read symbolic link 'mnt': Permission denied ls: cannot read 
symbolic link 'cgroup': Permission denied ls: cannot read symbolic 
link 'time': Permission denied ls: cannot read symbolic link 
'time_for_children': Permission denied total 0 lrwxrwxrwx 1 slovdahl 
slovdahl 0 maj 3 19:28 cgroup lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 
19:28 ipc lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:27 mnt lrwxrwxrwx 
1 slovdahl slovdahl 0 maj 3 19:28 net lrwxrwxrwx 1 slovdahl slovdahl 
0 maj 3 19:28 pid lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:28 
pid_for_children lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:28 time 
lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:28 time_for_children 
lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:28 user lrwxrwxrwx 1 
slovdahl slovdahl 0 maj 3 19:28 uts 
slovdahl@ubuntu2204:/proc/1751545/ns$ sudo ls -lh total 0 lrwxrwxrwx 
1 slovdahl slovdahl 0 maj 3 19:28 cgroup -> 'cgroup:[4026531835]' 
lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:28 ipc -> 
'ipc:[4026531839]' lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:27 mnt 
-> 'mnt:[4026533862]' lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:28 
net -> 'net:[4026531840]' lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 
19:28 pid -> 'pid:[4026531836]' lrwxrwxrwx 1 slovdahl slovdahl 0 maj 
3 19:28 pid_for_children -> 'pid:[4026531836]' lrwxrwxrwx 1 slovdahl 
slovdahl 0 maj 3 19:28 time -> 'time:[4026531834]' lrwxrwxrwx 1 
slovdahl slovdahl 0 maj 3 19:28 time_for_children -> 
'time:[4026531834]' lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:28 user 
-> 'user:[4026531837]' lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:28 
uts -> 'uts:[4026531838]' |


FWIW, my IntelliJ also highlighted the fact that the suggested patch 
contains unreachable code. The |else throw new 
IOException(String.format("target process:%d and this do not share 
common mount namespace for: %s attach failed", pid, tmpdir));| path 
can never be taken, because either the |if| statement evaluates to 
|true|

Re: [External] : Re: [openjdk/jdk] 8327114: Attach in Linux may have wrong behaviour when pid == ns_pid (Kubernetes debug container) (PR #19055)

2024-05-06 Thread Laurence Cable 

I'll send you another diff, I have something that I think may work...

On 5/6/24 9:16 AM, Sebastian Lövdahl wrote:
Yep, that sounds reasonable. I'll try to work something out along 
these lines, thanks for the input!


Unfortunately, /proc//cwd is also restricted in the same way 
as /proc//root is.


/Sebastian

On 2024-05-05 00:06, Laurence Cable wrote:

so I think to summarize the logic we require:

1) if we can determine that the attacher and attachee occupy the same 
mnt ns (/proc//ns/mnt == /proc//ns/mnt), return 
"/tmp"

2) if they are not in the same mnt ns:
    - test the /proc//root/tmp path for readability, if it 
is, return that
    - if it is not readable (its either secured with elevated privs 
and it exists) return "/tmp" which may still fail because they are in 
fact not in the same mnt ns


what about /proc//cwd?

- Larry


On 5/3/24 9:43 AM, Sebastian Lövdahl wrote:


Thanks for the patch @larry-cable 
, 
much appreciated! I really like this idea.


I tried it out a bit locally. These cases seem to work:

  * attaching to a process running on the same host (PID and mount
namespace the same)
  * attaching as root from the host to a JVM inside a container
  * attaching from a sidecar container to a JVM in another container

Unfortunately, attaching to a JVM process running as the same user 
in the same PID and mount namespace but one that has elevated 
privileges no longer works (the case that JDK-8226919 fixed). That 
case ends up failing like this with the patch:


|slovdahl@ubuntu2204:~/reproducer$ 
/home/slovdahl/dev/external/jdk/build/linux-x86_64-server-release/images/jdk/bin/jcmd 
1751545 VM.version 1751545: java.io.IOException: Unable to access 
root directory /proc/1751545/root of target process 1751545 at 
jdk.attach/sun.tools.attach.VirtualMachineImpl.findTargetProcessTmpDirectory(VirtualMachineImpl.java:284) 
at 
jdk.attach/sun.tools.attach.VirtualMachineImpl.findSocketFile(VirtualMachineImpl.java:229) 
at 
jdk.attach/sun.tools.attach.VirtualMachineImpl.(VirtualMachineImpl.java:86) 
at 
jdk.attach/sun.tools.attach.AttachProviderImpl.attachVirtualMachine(AttachProviderImpl.java:58) 
at 
jdk.attach/com.sun.tools.attach.VirtualMachine.attach(VirtualMachine.java:207) 
at jdk.jcmd/sun.tools.jcmd.JCmd.executeCommandForPid(JCmd.java:113) 
at jdk.jcmd/sun.tools.jcmd.JCmd.main(JCmd.java:97) |


I /think/ it boils down to the same reason as why the fix for 
JDK-8226919 was needed in the first place - a non-root user cannot 
read the symlinks in |/proc//ns| for a process running with 
more privileges even though it's run by the same non-root user.


|slovdahl@ubuntu2204:/proc/1751545/ns$ ls -lh ls: cannot read 
symbolic link 'net': Permission denied ls: cannot read symbolic link 
'uts': Permission denied ls: cannot read symbolic link 'ipc': 
Permission denied ls: cannot read symbolic link 'pid': Permission 
denied ls: cannot read symbolic link 'pid_for_children': Permission 
denied ls: cannot read symbolic link 'user': Permission denied ls: 
cannot read symbolic link 'mnt': Permission denied ls: cannot read 
symbolic link 'cgroup': Permission denied ls: cannot read symbolic 
link 'time': Permission denied ls: cannot read symbolic link 
'time_for_children': Permission denied total 0 lrwxrwxrwx 1 slovdahl 
slovdahl 0 maj 3 19:28 cgroup lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 
19:28 ipc lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:27 mnt 
lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:28 net lrwxrwxrwx 1 
slovdahl slovdahl 0 maj 3 19:28 pid lrwxrwxrwx 1 slovdahl slovdahl 0 
maj 3 19:28 pid_for_children lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 
19:28 time lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:28 
time_for_children lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:28 user 
lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:28 uts 
slovdahl@ubuntu2204:/proc/1751545/ns$ sudo ls -lh total 0 lrwxrwxrwx 
1 slovdahl slovdahl 0 maj 3 19:28 cgroup -> 'cgroup:[4026531835]' 
lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:28 ipc -> 
'ipc:[4026531839]' lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:27 mnt 
-> 'mnt:[4026533862]' lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:28 
net -> 'net:[4026531840]' lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 
19:28 pid -> 'pid:[4026531836]' lrwxrwxrwx 1 slovdahl slovdahl 0 maj 
3 19:28 pid_for_children -> 'pid:[4026531836]' lrwxrwxrwx 1 slovdahl 
slovdahl 0 maj 3 19:28 time -> 'time:[4026531834]' lrwxrwxrwx 1 
slovdahl slovdahl 0 maj 3 19:28 time_for_children -> 
'time:[4026531834]' lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 19:28 
user -> 'user:[4026531837]' lrwxrwxrwx 1 slovdahl slovdahl 0 maj 3 
19:28 uts -> 'uts:[4026531838]' |


FWIW, my IntelliJ also highlighted the fact that the suggested patch 
contains unreachable code. The |else throw new 
IOException(String.format("target process:%d and this do not share 
common mount namespace for