Hi Dmitry,
I'd like to resolve my questions before the upcoming design discussion
on Thu.
http://cr.openjdk.java.net/%7Edsamersoff/JDK-8061228/webrev.15/src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c.udiff.html
(0) The design description from the bug report tells:
> Than we change a negotiation protocol between JDWP and transport.
> We pass maximal supported version to transport initialization
routine and expect transport actual version to be returned.
The modified negotiation protocol adds extra complexity.
What is a motivation behind this?
Is it really necessary for the transport library to return an actual
version instead of rejecting the unmatched version?
I do not see it is really used in the webrev.15 implementation.
(1) The following change in the jdwp transport library will reject
theJDWPTRANSPORT_VERSION_1_0 as it is below
the version JDWPTRANSPORT_VERSION_1_1, but will except any version
above the JDWPTRANSPORT_VERSION_1_1
(with providing/returning the actual transport version):
jdwpTransport_OnLoad(JavaVM *vm, jdwpTransportCallback* cbTablePtr,
- jint version, jdwpTransportEnv** result)
+ jint version, void** env)
{
- if (version != JDWPTRANSPORT_VERSION_1_0) {
+ if (version < JDWPTRANSPORT_VERSION_1_1) {
return JNI_EVERSION;
}
Te following change will also prevent supporting the 1_0 version of the
transport library:
- interface.StartListening = &socketTransport_startListening;
+ interface.StartListening = NULL;
I'd suggest the following alternate change to the transport API allowing
to support
both old and new versions at the same time (it would simplify the
negotiation rules):
- Add new function:
jdwpTransportError AllowPeers(const char* peers);
- Keep the original StartListening function.
The function uses the allowed peers data if it was previously
cached by the AllowPeers().
- It seems, the alternate approach does not require adding the
extra_data with version.
But if there is still a real need to get the transport API version
then it'd better
to introduce a function named GetTransportVersion() or
JDWP_TransportVersion().
This would allow to encapsulate any extra_data that is necessary in
such a case.
(2) The following error messages miss the actual IP address or mask that
was found to be illegal:
383 RETURN_ERROR(JDWPTRANSPORT_ERROR_ILLEGAL_ARGUMENT, "invalid ip
address for allow"); 392
RETURN_ERROR(JDWPTRANSPORT_ERROR_ILLEGAL_ARGUMENT, "invalid netmask for
allow");
(3) A suggestion on the following:
377 uint32_t mask = 0xFFFFFFFF; 400 mask = 0xFFFFFFFF; // reset mask
I'd suggest a more explicit approach instead of the L400 for a better
clarity:
386 if (*s == '/') {
387 // netmask specified
388 s = mask_s2u(s + 1, &mask);
389 if (*(s-1) == '/') {
390 // Input is not consumed, something bad happens
391 _peers_cnt = 0;
392 RETURN_ERROR(JDWPTRANSPORT_ERROR_ILLEGAL_ARGUMENT, "invalid netmask
for allow");
393 }
394 } else { mask = 0xFFFFFFFF; }
http://cr.openjdk.java.net/%7Edsamersoff/JDK-8061228/webrev.15/src/jdk.jdwp.agent/share/native/libjdwp/transport.c.udiff.html
(4) Some confusion with the fragment and its comment:
+
+ /* Pass the latest supported version,
+ * expect actual transport version in t->extra_data->version
+ */
+ ver = (*onLoad)(jvm, &callback, JDWPTRANSPORT_VERSION_1_1, &t);
+ if (ver == JNI_EVERSION) {
ver = (*onLoad)(jvm, &callback, JDWPTRANSPORT_VERSION_1_0, &t);
+ // Special handling for versionless transports
+ info->transportVersion = JDWPTRANSPORT_VERSION_1_0;
+ }
+ else {
+ info->transportVersion = (*t)->extra_data->version;
+ }
+
The term "latest supported version" is ambiguous in this context. Do you
mean, supported by the JDWP back-end or by the agent library? Also, it
is not clear in what circumstances the agent library would support the
version 1_0 only. The JDWP back-end is always coupled with the socket
transport library, isn't it? Is it because the libdt_shmem library can
be used on Windows or because a different native library path can be
used? Could you explain a little bit? As we see in (1) above the actual
transport version can be different from requested only if the requested
version is above the latest supported by the transport library.
Otherwise, the version is matched or the JNI_EVERSION is returned. The
subsequent call to the OnLoad function can succeed only if the library
supports just the version 1_0. (5) Memory allocation for the info->allow
field is needed only for the transport version 1_1:
+ if (allow != NULL) {
+ info->allow = jvmtiAllocate((int)strlen(allow)+1);
+ if (info->allow == NULL) {
+ serror = JDWP_ERROR(OUT_OF_MEMORY);
+ goto handleError;
+ }
+ (void)strcpy(info->allow, allow);
+ }
(6) There is no handling for the condition when the *allow* parameter is
passed but the transport version is 1_0 (which does not support the
*allow* parameter):
+ /* Interface version 1.0 doesn't support versioning, so we have to
+ * use global variable and set the version artifically.
+ * Use (*t)->extra_data->version directly when 1.0 is gone.
+ */
+ switch(info->transportVersion) {
+ case JDWPTRANSPORT_VERSION_1_0:
err = (*trans)->StartListening(trans, address, &retAddress);
+ break;
+ case JDWPTRANSPORT_VERSION_1_1:
+ err = (*trans)->StartListening11(trans, address, &retAddress,
info->allow);
+ break;
+ default:
+ err = JNI_EVERSION;
+ }
Thanks, Serguei On 3/29/17 08:10, Dmitry Samersoff wrote:
Robbin,
One follow-up issue is that if you start suspended
and than connect with
an unallowed client the JVM starts and executes the program.
Fixed.
see http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.15
-Dmitry
On 2017-03-16 15:59, Robbin Ehn wrote:
Hi Dmitry, thanks for the update.
One follow-up issue is that if you start suspended and than connect with
an unallowed client the JVM starts and executes the program.
Simple program prints "Hello".
[rehn@rehn-ws vanilla-hs]$ java
-agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=*:9999,allow=1.2.3.0/32
-cp . H
Listening for transport dt_socket at address: 9999
ERROR: Peer not allowed to connect
Listening for transport dt_socket at address: 9999
Hello
I think it would be good if the VM stayed suspended when an unallowed
client connects.
Thanks, Robbin
On 03/13/2017 08:07 PM, Dmitry Samersoff wrote:
Robbin,
Please, see:
http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.11
1:
So connecting with an unallowed client terminates the VM.
Fixed.
2:
Starting with an bad allow filter terminates the VM when connecting a
client.
Moved allowed parameter (and parser call) to StartListening.
-Dmitry
On 2017-03-10 15:56, Robbin Ehn wrote:
Hi Dmitry,
I took a look at this, I have two practical issues:
1:
[rehn@rehn-ws dev]$ java
-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=*:9999,allow=6.6.6.6
-cp runs ForEver
Listening for transport dt_socket at address: 9999
ERROR: transport error 202: peer not allowed to connect: Success
JDWP exit error JVMTI_ERROR_NONE(0): could not connect, timeout or fatal
error [transport.c:358]
So connecting with an unallowed client terminates the VM.
2:
[rehn@rehn-ws dev]$ java
-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=*:9999,allow=6.BAD.6.6
-cp runs ForEver
Listening for transport dt_socket at address: 9999
ERROR: transport error 202: unable to parse list of allowed peers:
Success
JDWP exit error JVMTI_ERROR_NONE(0): could not connect, timeout or fatal
error [transport.c:358]
Starting with an bad allow filter terminates the VM when connecting a
client.
Connecting with an unallowed ip/port should not terminate the VM and we
should verify allow filter directly at startup.
Thanks
/Robbin
On 02/28/2017 10:41 AM, Dmitry Samersoff wrote:
Everybody,
Please review:
http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.10/
These changes introduce new parameter[1] of the socket transport -
allow. Users can explicitly specify a list of hosts that allowed to
connect to jdwp server and it's the second part of JDWP hardening[2].
No restrictions are applied by default now but I'll file a separate CR
to restrict list of allowed peers to localhost by default.
Also these changes implement versioning for jdwp transport and therefor
simplify feature development of jdwp.
1. Example command line:
-agentlib:jdwp=transport=dt_socket,server=y,suspend=n,
address=*,allow="127.0.0.0/8;192.168.0.0/24"
Possible values for allow parameter:
* - accept connections from everywhere.
N.N.N.N - accept connections from this IP address only
N.N.N.N/nn - accept connections from particular ip subnet
2. JDK-8052136 JDWP hardening
-Dmitry
