RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only
Dmitry Samersoff
dmitry.samersoff at oracle.com
Wed May 10 08:10:55 UTC 2017
Serguei,
Please see my comments in-line.
On 2017-05-10 00:42, serguei.spitsyn at oracle.com wrote:
> 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.
Transport have to return it's actual version in order to allow agent
to perform appropriate action.
see libjdwp/transport.c:526
Today it's just a selection of proper API call but in a future it might
be too-old-transport-error or deprecation warning or security warning or
something else.
> (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;
libdt_socket/socketTransport.c is an implementation of 1.1 *transport*
it's not intended to run with 1.0 *backend*.
i.e. 1.1 *backend* can run 1.1 and 1.0 transports but 1.1 *transport*
require 1.1 or greater backend.
see: libjdwp/transport.c:206 for version logic on backend (agent) side
> 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.
>From pure JDWP hardening point of view we can just add extra parameter
*allow* to existing StartListening(). Caller is responsible for stack
consistency so old transport continues to work.
But my goal was to create a versioning in JDWP agent -> transport
relations that was missed in initial JDWP design. Further, more
complicated, changes (IPv6 support, UDS sockets support etc) require
this logic.
We have a structure jdwpTransportNativeInterface with a fixed set of
functions (see jdwpTransport.h:153). To add any new function to this
structure we have to create a method to detect presence of this
function. So we can't use GetTransportVersion().
With as separate AllowPeer() function nobody remind an agent writer that
they should use it, but extra parameter makes api changes and
requirements clear visible (up to compiler warning).
Also I'm against of changing behavior of existing function.
> (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");
Other parameter parsing functions (e.g. "invalid port number specified"
at 274) doesn't explain what parameter is bad.
I think typical allow would have one or two entry, so verbose error
message is not worth significant complication of parsing code.
I would prefer to leave it as is.
> (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; }
I'll try it.
> 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?
Supported by the JDWP backend. I'll update comments.
> 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?
It's more generic question: Do we need any backward compatibility here?
I assume *yes* (can't recall why - probably it was a tree-year old
decision).
If we state that new backend will not support old transports today and
in a future - all versioning logic is not necessary.
> 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.
See answer to (1) above. transport library has exactly one version but
JDWP backend supports couple of transport library versions back.
> (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);
> + }
Correct. allocation needed for 1.1 and *greater*. I can change it, but
it makes code less readable.
> (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):
Correct. Warning or ever error should be here.
I plan to open a separate follow-up CR for this problem - we have to
decide how we should handle this situation (warning or error) and change
the code,
but I can add a plain printf() here if you like.
>
> + /* 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;
> + }
-Dmitry
>
> 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 at 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 at 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 at 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
>>>>>>
>>>>
>>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
More information about the serviceability-dev
mailing list