Hi Dmitry,
Let's get to a consensus for the last item on our plate:
jdwpTransportError AllowPeers(const
char* peers);
.VS.
jdwpTransportError
StartListeningWithAllow(const char* address,
char** actual_address, char *allow);
I still do not like the StartListening() function clone
and its name.
It looks like a wrong direction to me.
This is a fragment of my bug report comment on this issue:
4. The suggested *allow* feature is too destructive for the
transport API.
It makes the original function StartListening unusable and
its entry slot a garbage.
It will become a big mess if we add more function clones like
StartListening11.
. . .
6. The suggested API update makes the transport API VERSION_1_1
incompatible with the initial VERSION_1_0.
One more alternate solution to suggest is to add new function:
jdwpTransportError SetTransportConfiguration(jdwpTransportConfiguration
config);
Where:
typedef struct {
const char* allowed_peers;
} jdwpTransportConfiguration;
This approach allows to extend the jdwpTransportConfiguration in
the future if necessary.
However, I'm not sure we really need this ability.
Both alternatives allow for the transport library to support both
API versions.
It is good by itself and also, it is a way to simplify the
transport library.
We could formulate the following requirements to the API updates:
- the transport API update must support previous API versions
- the updated API layout must be compatible with previous
versions
It seems to me, these requirements are achievable and two
alternate approaches show it.
Please, share your opinions?
Thanks,
Serguei
On 5/12/17 02:10, serguei.spit...@oracle.com wrote:
Hi Dmitry,
Thank you for the update with some review comments resolved!
I looked at it but will not provide my comments at this point.
We will need another update according to the recent design review
and items where we still have to reach an agreement.
Thanks,
Serguei
On 5/10/17 08:27, 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.spit...@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.spit...@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@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
|