RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only
Robbin Ehn
robbin.ehn at oracle.com
Thu May 11 09:01:39 UTC 2017
Hi,
I find both your approaches acceptable regarding the version and StartListening11 vs AllowPeers.
Personally I prefer not using version instead using sizeof as syscall does.
E.g.
http://man7.org/linux/man-pages/man2/bind.2.html
But obviously this also require a new StartListeningXX method.
/Robbin
On 05/10/2017 05:27 PM, Dmitry Samersoff wrote:
> Serguei,
>
> Fixed minor issues (comments, netmask etc).
> Added an error for attempt to use allow with an old transport.
>
> http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.17/
>
> see also below.
>
>
>> 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.
>
> Added a diagram explaining transport version negotiation to CR. I use
> future versions (1.3; 1.4; 1.5) because all this versioning staff has a
> long term goal and allow us to develop better transport without breaking
> existing one.
>
>> 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.
>
> Correct. We changed behavior of StartListening function and 1.1
> transport shouldn't care about old one.
>
> i.e. when we document 1.1 interface we describe the only function
> StartListening(env, address, actualAddress, allow)
> that have to be placed to the StartListening11 slot.
>
>
> Back in 2015 I proposed to separate interfaces entirely (see webrev.04),
> but we (Alan?) decided that it's an overkill.
>
>> The original StartListening function is being removed.
>> It is much simpler to introduce new function AllowPeers(char* peers)
>> with the same parameter.
>
> This separate function have to be called explicitly before we start
> listening, It is extra communication step. IMHO, not obvious one.
>
> So I would prefer to keep StartListening11
>
> -Dmitry
>
>
> On 2017-05-10 12: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.
>> 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