RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Wed May 10 09:42:56 UTC 2017
Dmitry,
Please, see one correction below.
On 5/10/17 02:37, serguei.spitsyn at oracle.com wrote:
> Dmitry,
>
> Thank you a lot for the detailed reply!
>
>
> On 5/10/17 01:10, Dmitry Samersoff wrote:
>> 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
>
> This requirement adds extra complexity to the rules (transport
> negotiation protocol).
> It is not really necessary.
> The loadTransport() already does a lookup of a version that is accepted
> (not rejected) by the transport library and can save that version.
> The transport_startTransport() then should use the version found by
> the loadTransport().
>
>
>> 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.
>
> I do not understand this reason for adding more complexity.
> It seems, there should not be any issues in the future with rejecting
> all unsupported versions by the transport library.
> However, it will be even more simple if one transport library API could
> support/accept all possible versions (see my alternate suggestion below).
>
>
>>> (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*.
>
> Why not?
> It would simplifies things if the transport library (and its API) is
> backward compatible.
>
>> i.e. 1.1 *backend* can run 1.1 and 1.0 transports but 1.1 *transport*
>> require 1.1 or greater backend.
>
> This statement creates a confusion.
> The truce is that the transport library can support some number of
> versions.
> The latest supported version can satisfy the agent if it supports it.
>
>> see: libjdwp/transport.c:206 for version logic on backend (agent) side
>
> The logic at L206 does not require the transport library to return its
> version.
> It will work Ok if the library rejects unsupported versions.
>
>
>>> 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.
>
> We are not adding extra parameter, we are introducing new function
> that is a clone of another StartListening function with a version
> suffix '11' in its name and with an extra parameter.
> The original StartListening function is being removed.
A correction: Not removed but nulled out.
Do I get it right?
Thanks,
Serguei
> It is much simpler to introduce new function AllowPeers(char* peers)
> with the same parameter.
> The original StartListening function works as before.
> The updated API can support both versions 1_0 and 1_1.
>
>
>> 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.
>
> Would introducing function JdwpTransportVersion() achieve what you
> wanted?
>
>
>> 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().
> It is not clear why would you need such a method for any new function?
> The transport version maps to the whole set of functions.
>
>
>> 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).
>
> I do not see any difference.
> No compiler warning if NULL is passed in place of the 'allow' parameter.
>
>
>> 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.
>
> It is not good either.
> But new functionality is added, so the more diagnostic details the
> better.
>
>> I think typical allow would have one or two entry, so verbose error
>> message is not worth significant complication of parsing code.
>
> It is still better to print what was not accepted.
> It should not be a problem to print it anyway.
>
>
>> 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.
>
> Ok, thanks.
>
>
>>> 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.
>
> Ok, thanks.
>
>
>>> 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.
>
> I've replied above too. :)
>
>
>>> (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.
>
> The same fragment in a different place should not be less readable,
> maybe just less elegant.
>
>
>>> (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.
>
> I'm Ok with both error or warning but what is needed from a security
> point of view?
> We can avoid filing a separate follow-up CR in this case.
> Should it be an error if the *allow* parameter is used with the old
> transport library that does not support it?
>
>
> Thanks,
> Serguei
>
>
>>> + /* 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
>>>>>>>>
>>
>
More information about the serviceability-dev
mailing list