Quantcast

RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

classic Classic list List threaded Threaded
29 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

Dmitry Samersoff
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.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

Robbin Ehn
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
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

Dmitry Samersoff
Robbin,

Agree, I'll fix it.

-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
>>


--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

Dmitry Samersoff
In reply to this post by Robbin Ehn
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
>>


--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

serguei.spitsyn@oracle.com
In reply to this post by Dmitry Samersoff
Hi Dmitry,

We already had a big review thread back in 2014 on this.
I think, it is again going in the wrong order.

First, I think, it is better to start from a CCC (or its equivalent, CSR).
This will allow to focus on and sort out the changes in interface/behavior
first before going into the implementation details.

Second, I'd suggest to separate a couple of other things.
I still see the C .vs. C++ related change in the jdwpTransport.h.
It is better to leave it along for now as it was suggested in early
review rounds.
If you still want to fix it then it is better to file a separate bug that
should include the JNI as well (as it was discussed with Alan before).

Also, I'm thinking if it is a good idea to separate the transport
versioning to an RFE.
It would allow to focus on this aspect as necessary.
In this case, the 8061228 will depend on the versioning.
However, it is much more simple and can be resolved faster.


Thanks,
Serguei


On 2/28/17 01:41, 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
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

Dmitry Samersoff
Serguei,

> I still see the C .vs. C++ related change in the jdwpTransport.h.

done.

http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.12/

see also inline.

On 2017-03-15 00:40, [hidden email] wrote:
> Hi Dmitry,
>
> We already had a big review thread back in 2014 on this.
> I think, it is again going in the wrong order.
>
> First, I think, it is better to start from a CCC (or its equivalent, CSR).
> This will allow to focus on and sort out the changes in interface/behavior
> first before going into the implementation details.

CCC was filed and approved. The only reason I withdraw it - the fix
didn't go to jdk9 but CCC tool doesn't have jdk10.

CSR process is also not yet implemented.

> Second, I'd suggest to separate a couple of other things.
> I still see the C .vs. C++ related change in the jdwpTransport.h.
> It is better to leave it along for now as it was suggested in early
> review rounds.
> If you still want to fix it then it is better to file a separate bug that
> should include the JNI as well (as it was discussed with Alan before).

Do you mean?

  39 #ifdef __cplusplus
  40 extern "C" {
  41 #endif

I'll add it back to avoid any confusion.

> Also, I'm thinking if it is a good idea to separate the transport
> versioning to an RFE.
> It would allow to focus on this aspect as necessary.
> In this case, the 8061228 will depend on the versioning.
> However, it is much more simple and can be resolved faster.

It's hard to test versioning code without implementation of
new, VERSION_1_1 transport.

Network part of 8061228 is simple and clear separated from versioning,
so I would prefer to keep it together in one CR/one push.

Restriction turned off by default (I'll file separate CR to enable it
later), so we have enough time to fix any issues that might come.

-Dmitry

>
>
> Thanks,
> Serguei
>
>
> On 2/28/17 01:41, 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.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

Robbin Ehn
In reply to this post by Dmitry Samersoff
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
>>>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

serguei.spitsyn@oracle.com
In reply to this post by Dmitry Samersoff
Hi Dmitry,



On 3/15/17 00:56, Dmitry Samersoff wrote:
> Serguei,
>
>> I still see the C .vs. C++ related change in the jdwpTransport.h.
> done.
>
> http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.12/

Good.
Thank you for the update!


>
> see also inline.
>
> On 2017-03-15 00:40, [hidden email] wrote:
>> Hi Dmitry,
>>
>> We already had a big review thread back in 2014 on this.
>> I think, it is again going in the wrong order.
>>
>> First, I think, it is better to start from a CCC (or its equivalent, CSR).
>> This will allow to focus on and sort out the changes in interface/behavior
>> first before going into the implementation details.
> CCC was filed and approved. The only reason I withdraw it - the fix
> didn't go to jdk9 but CCC tool doesn't have jdk10.
>
> CSR process is also not yet implemented.
The CSR preview message from Joe is attached.
My understanding is that we can continue to use CCC.
At some points the CCC process will be moved to the CSR.

The CCC is out of date now as it does not match the webrev 12.
Also, I do not like the addition of new function StartListening11() next
to StartListening().

Does it mean, that for transport version 1.2 we might have more function
clones with 12 suffix?
Perhaps, we need something smarter here but I'm unsure yet what it has
to be.


>> Second, I'd suggest to separate a couple of other things.
>> I still see the C .vs. C++ related change in the jdwpTransport.h.
>> It is better to leave it along for now as it was suggested in early
>> review rounds.
>> If you still want to fix it then it is better to file a separate bug that
>> should include the JNI as well (as it was discussed with Alan before).
> Do you mean?
>
>    39 #ifdef __cplusplus
>    40 extern "C" {
>    41 #endif
>
> I'll add it back to avoid any confusion.
Yes. I see you added it back in new version of webrev.


>
>> Also, I'm thinking if it is a good idea to separate the transport
>> versioning to an RFE.
>> It would allow to focus on this aspect as necessary.
>> In this case, the 8061228 will depend on the versioning.
>> However, it is much more simple and can be resolved faster.
> It's hard to test versioning code without implementation of
> new, VERSION_1_1 transport.
>
> Network part of 8061228 is simple and clear separated from versioning,
> so I would prefer to keep it together in one CR/one push.
No pressure.
I've got your point above.


Thanks,
Serguei

>
> Restriction turned off by default (I'll file separate CR to enable it
> later), so we have enough time to fix any issues that might come.
>
> -Dmitry
>
>>
>> Thanks,
>> Serguei
>>
>>
>> On 2/28/17 01:41, 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
>>>
>

Hello,

The JDK organization has long been served by the ccc webapp at
http://ccc.us.oracle.com. With the migration of bugs to JBS and the
availability of a configurable JIRA system, other workflows can be
supported by JBS as well, as seen in JEPs. As a continuation of this
trend and to allow better integration across processes, the ccc process,
with some adaptations, will soon be moved to JBS. After being moved to
JBS, the process will also be externalized, usable and visible to JBS
users who are not Oracle employees subject to the usual restrictions for
confidential issues.

Initially ccc reviews for post-9 releases will be done in the new
system. To avoid changing JCK and JCP-documentation processes
mid-release, JDK 9 will continue to be run on the existing ccc system.
After JDK 9 GA, there will be a high-fidelity bulk import of the old
requests into the new system.

One of the adaptations of the ccc process is a new name "CSR" standing
for "Compatibility and Specification Review." This new name is more
evocative of the role the process plays and is easier to explain to new
users.

The new "CSR" issue type is available for early preview on the JBS
staging system:

     https://bugs-stage.openjdk.java.net

To create a CSR for an issue, under the "More" menu select "Create CSR".
This is similar to how a backport can be created manually. [1]

Thanks to Tony Squire implementing the JBS support for CSR.

More information about the migration will be available next year,
including expected OpenJDK discussions about the process. Until the CSR
work is publicly disclosed, please keep discussions of this project
internal to Oracle.

The remainder of this email, including the attachment, includes more
detailed information about how the CSR process will differ from the ccc.

Please send initial feedback on the CSR implementation to me; there will
be more opportunities to provide comments before the process goes live.

Cheers,

-Joe

[1] Conceptually, a CSR is a subtask of its parent issue. However, we
cannot implement it that way in JIRA since we want to be able to allow a
CSR issue to have a different security setting than its parent bug. In
particular, we need to allow the import of all legacy ccc issues from
the current system as "Confidential" CSRs even if the parent issue is
publicly visible to avoid having to review and scrub all the legacy ccc
requests.


  Differences between ccc and CSR
-----------------------------

At a high-level, the envisioned CSR process is very similar to the ccc:
engineers present proposals for either a one-phase or two-phase review
by the review body. The body conducts the review, possibly providing
comments, and engineers and the review body update the state of the
request accordingly until it is approved or withdrawn.

Leveraging JIRA, rather than conducting the majority of the discussions
about a request on a separate mailing list, JIRA comments will instead
be used as the primary communication channel. This provides easier
notification to interested parties (e.g. watchers on the issue) and
better archiving of the discussion.

Looking over the sections of a ccc request, some were dropped as not
providing sufficient value for their cost including:

     Requestors
     Interface summary

The sections

     Problem
     Specification
     Solution

remain and the Description field of a CSR is pre-populated with a
markdown version of those three sections. The compatibility risk and
compatibility sections are retained as separate fields.

A CSR request has the new Scope field copied from JEPs with "SE", "JDK",
and "Implementation" choices

The attached picture shows the workflow of a CSR request. It is very
similar to the existing ccc workflow. Some difference and adaptions to JIRA:

* The "Accepted" state is renamed "Provisional." This is to put more
Hamming distance between "Accepted" and "Approved" to avoid confusion
for new and occasional ccc/csr users.

* To model JCK voting without building a full voting mechanism, the new
states "JCK Initial Review" and "JCK Final Review" have been introduced.
The JCK team can transition proposed and finalized requests,
respectively, to the new states.

* The chair can transition requests to the "Provisional" and "Closed /
Approved" states.

* Any CSR member can transition a request to "Pended".

* The assignee and reporter can transition a request to "Draft", "Closed
/ Will-not-fix" (the new "Withdrawn"), "Proposed", and "Finalized".

A few open issues about how the new process will work:

* Per general JBS guidelines, technical information about security
vulnerabilities should *not* be stored in JBS. This should be extended
to include CSR requests. As security vulnerability can and do need
ccc/CSR review, some alternative approach will need to be found.
Discussions on on the existing  ccc alias may be used as a fallback,
possibly with pointers into bugdb.

* Multi-release tracking: as currently implemented, if the same API
change is being made in multiple releases, say, 7uX, 8uX, and 9, rather
than filing three requests, one per release train, a single ccc request
against the oldest release can be filed with the understanding it also
applies by default to newer releases. The most direct way to support a
similar capability in CSR would be to use multiple values in the Fix
Version/s field. However, we have a strong convention for all other JBS
but to only use a single value of Fix Version/s and to instead use
backports for multi-release tracking.


csr-workflow.jpg (31K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

serguei.spitsyn@oracle.com
Dmitry,


Some quick comments on the webrev.12.


The style of comments must be /* */, not //.


http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.12/src/jdk.jdwp.agent/share/native/libjdwp/transport.c.frames.html

205 // Pass MAXIMAL supported version, expect actual transport version in

    Would it better to replace 'MAXIMAL' with 'the latest' ?


516 // interface version 1.0 doesn't support versioning, so we have to
517 // a use global variable and set the version artifically.
518 // Use (*t)->extra_data->version directly when 1.0 is gone

   516: interface => Interface
   517: Typo: a use => use
   518: Dot is missed at the end.


33 static unsigned transportVersion = JDWPTRANSPORT_VERSION_1_0; ... 207
ver = (*onLoad)(jvm, &callback, JDWPTRANSPORT_VERSION_1_1, &t);
208 if (ver == JNI_EVERSION) {
  209             ver = (*onLoad)(jvm, &callback, JDWPTRANSPORT_VERSION_1_0, &t);
210 // Special handling for versionless transports
211 info->transportVersion = JDWPTRANSPORT_VERSION_1_0;
212 }
213 else {
214 info->transportVersion = (*t)->extra_data->version;
215 } ...263 transportVersion = pTransportVersion; ... 459
info->transportVersion = transportVersion; It is not clear why do you
need the static variable transportVersion and this dance with it. It
seems, the transport version is always enforced by the TransportOnLoad
function anyway. At the line 459, you could just have:
459 info->transportVersion = JDWPTRANSPORT_VERSION_1_0; Thanks, Serguei

On 3/17/17 00:03, [hidden email] wrote:

> Hi Dmitry, On 3/15/17 00:56, Dmitry Samersoff wrote:
>> Serguei,
>>> I still see the C .vs. C++ related change in the jdwpTransport.h.
>> done. http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.12/ 
> Good. Thank you for the update!
>> see also inline. On 2017-03-15 00:40, [hidden email] wrote:
>>> Hi Dmitry, We already had a big review thread back in 2014 on this.
>>> I think, it is again going in the wrong order. First, I think, it is
>>> better to start from a CCC (or its equivalent, CSR). This will allow
>>> to focus on and sort out the changes in interface/behavior first
>>> before going into the implementation details.
>> CCC was filed and approved. The only reason I withdraw it - the fix
>> didn't go to jdk9 but CCC tool doesn't have jdk10. CSR process is
>> also not yet implemented.
> The CSR preview message from Joe is attached. My understanding is that
> we can continue to use CCC. At some points the CCC process will be
> moved to the CSR. The CCC is out of date now as it does not match the
> webrev 12. Also, I do not like the addition of new function
> StartListening11() next to StartListening(). Does it mean, that for
> transport version 1.2 we might have more function clones with 12
> suffix? Perhaps, we need something smarter here but I'm unsure yet
> what it has to be.
>>> Second, I'd suggest to separate a couple of other things. I still
>>> see the C .vs. C++ related change in the jdwpTransport.h. It is
>>> better to leave it along for now as it was suggested in early review
>>> rounds. If you still want to fix it then it is better to file a
>>> separate bug that should include the JNI as well (as it was
>>> discussed with Alan before).
>> Do you mean?    39 #ifdef __cplusplus    40 extern "C" {    41 #endif
>> I'll add it back to avoid any confusion.
> Yes. I see you added it back in new version of webrev.
>>> Also, I'm thinking if it is a good idea to separate the transport
>>> versioning to an RFE. It would allow to focus on this aspect as
>>> necessary. In this case, the 8061228 will depend on the versioning.
>>> However, it is much more simple and can be resolved faster.
>> It's hard to test versioning code without implementation of new,
>> VERSION_1_1 transport. Network part of 8061228 is simple and clear
>> separated from versioning, so I would prefer to keep it together in
>> one CR/one push.
> No pressure. I've got your point above. Thanks, Serguei
>> Restriction turned off by default (I'll file separate CR to enable it
>> later), so we have enough time to fix any issues that might come.
>> -Dmitry
>>> Thanks, Serguei On 2/28/17 01:41, 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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

Dmitry Samersoff
In reply to this post by Robbin Ehn
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
>>>>
>>
>>


--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

Dmitry Samersoff
In reply to this post by serguei.spitsyn@oracle.com
Serguei,

1. New webrev with couple of issues addressed (see Robbin's e-mails):

 http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.15

2. We have to keep transport version in global variable because old
transport doesn't initialize reserved1 field and we are loosing version
information after first disconnect. i.e. if I remove this "dancing"
debugger is not able to connect second time.

I'd reorganized code a bit for better readability.

3. ccc tool doesn't have JDK10 so we can't go forward.

-Dmitry


On 2017-03-17 12:20, [hidden email] wrote:

> Dmitry,
>
>
> Some quick comments on the webrev.12.
>
>
> The style of comments must be /* */, not //.
>
>
> http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.12/src/jdk.jdwp.agent/share/native/libjdwp/transport.c.frames.html
>
>
> 205 // Pass MAXIMAL supported version, expect actual transport version in
>
>    Would it better to replace 'MAXIMAL' with 'the latest' ?
>
>
> 516 // interface version 1.0 doesn't support versioning, so we have to
> 517 // a use global variable and set the version artifically.
> 518 // Use (*t)->extra_data->version directly when 1.0 is gone
>
>   516: interface => Interface
>   517: Typo: a use => use
>   518: Dot is missed at the end.
>
>
> 33 static unsigned transportVersion = JDWPTRANSPORT_VERSION_1_0; ... 207
> ver = (*onLoad)(jvm, &callback, JDWPTRANSPORT_VERSION_1_1, &t);
> 208 if (ver == JNI_EVERSION) {
>  209             ver = (*onLoad)(jvm, &callback,
> JDWPTRANSPORT_VERSION_1_0, &t);
> 210 // Special handling for versionless transports
> 211 info->transportVersion = JDWPTRANSPORT_VERSION_1_0;
> 212 }
> 213 else {
> 214 info->transportVersion = (*t)->extra_data->version;
> 215 } ...263 transportVersion = pTransportVersion; ... 459
> info->transportVersion = transportVersion; It is not clear why do you
> need the static variable transportVersion and this dance with it. It
> seems, the transport version is always enforced by the TransportOnLoad
> function anyway. At the line 459, you could just have:
> 459 info->transportVersion = JDWPTRANSPORT_VERSION_1_0; Thanks, Serguei
>
> On 3/17/17 00:03, [hidden email] wrote:
>> Hi Dmitry, On 3/15/17 00:56, Dmitry Samersoff wrote:
>>> Serguei,
>>>> I still see the C .vs. C++ related change in the jdwpTransport.h.
>>> done. http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.12/ 
>> Good. Thank you for the update!
>>> see also inline. On 2017-03-15 00:40, [hidden email] wrote:
>>>> Hi Dmitry, We already had a big review thread back in 2014 on this.
>>>> I think, it is again going in the wrong order. First, I think, it is
>>>> better to start from a CCC (or its equivalent, CSR). This will allow
>>>> to focus on and sort out the changes in interface/behavior first
>>>> before going into the implementation details.
>>> CCC was filed and approved. The only reason I withdraw it - the fix
>>> didn't go to jdk9 but CCC tool doesn't have jdk10. CSR process is
>>> also not yet implemented.
>> The CSR preview message from Joe is attached. My understanding is that
>> we can continue to use CCC. At some points the CCC process will be
>> moved to the CSR. The CCC is out of date now as it does not match the
>> webrev 12. Also, I do not like the addition of new function
>> StartListening11() next to StartListening(). Does it mean, that for
>> transport version 1.2 we might have more function clones with 12
>> suffix? Perhaps, we need something smarter here but I'm unsure yet
>> what it has to be.
>>>> Second, I'd suggest to separate a couple of other things. I still
>>>> see the C .vs. C++ related change in the jdwpTransport.h. It is
>>>> better to leave it along for now as it was suggested in early review
>>>> rounds. If you still want to fix it then it is better to file a
>>>> separate bug that should include the JNI as well (as it was
>>>> discussed with Alan before).
>>> Do you mean?    39 #ifdef __cplusplus    40 extern "C" {    41 #endif
>>> I'll add it back to avoid any confusion.
>> Yes. I see you added it back in new version of webrev.
>>>> Also, I'm thinking if it is a good idea to separate the transport
>>>> versioning to an RFE. It would allow to focus on this aspect as
>>>> necessary. In this case, the 8061228 will depend on the versioning.
>>>> However, it is much more simple and can be resolved faster.
>>> It's hard to test versioning code without implementation of new,
>>> VERSION_1_1 transport. Network part of 8061228 is simple and clear
>>> separated from versioning, so I would prefer to keep it together in
>>> one CR/one push.
>> No pressure. I've got your point above. Thanks, Serguei
>>> Restriction turned off by default (I'll file separate CR to enable it
>>> later), so we have enough time to fix any issues that might come.
>>> -Dmitry
>>>> Thanks, Serguei On 2/28/17 01:41, 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.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

Dmitry Samersoff
In reply to this post by serguei.spitsyn@oracle.com
Serguei,

(resending lost e-mail)

Please, see:

http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.15/

Nits fixed.

> It is not clear why do you
> need the static variable transportVersion and this dance with it.

We have to keep transport version in the global variable because

void *reserved1; (_extra_data in new code)

is not initialized by old transport and we can't rely on it's content
but have to keep transport version between subsequent connections.

I'd reorganized this part of code a bit for better readability.

-Dmitry

On 2017-03-17 12:20, [hidden email] wrote:

> Dmitry,
>
>
> Some quick comments on the webrev.12.
>
>
> The style of comments must be /* */, not //.
>
>
> http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.12/src/jdk.jdwp.agent/share/native/libjdwp/transport.c.frames.html
>
>
> 205 // Pass MAXIMAL supported version, expect actual transport version in
>
>    Would it better to replace 'MAXIMAL' with 'the latest' ?
>
>
> 516 // interface version 1.0 doesn't support versioning, so we have to
> 517 // a use global variable and set the version artifically.
> 518 // Use (*t)->extra_data->version directly when 1.0 is gone
>
>   516: interface => Interface
>   517: Typo: a use => use
>   518: Dot is missed at the end.
>
>
> 33 static unsigned transportVersion = JDWPTRANSPORT_VERSION_1_0; ... 207
> ver = (*onLoad)(jvm, &callback, JDWPTRANSPORT_VERSION_1_1, &t);
> 208 if (ver == JNI_EVERSION) {
>  209             ver = (*onLoad)(jvm, &callback,
> JDWPTRANSPORT_VERSION_1_0, &t);
> 210 // Special handling for versionless transports
> 211 info->transportVersion = JDWPTRANSPORT_VERSION_1_0;
> 212 }
> 213 else {
> 214 info->transportVersion = (*t)->extra_data->version;
> 215 } ...263 transportVersion = pTransportVersion; ... 459
> info->transportVersion = transportVersion; It is not clear why do you
> need the static variable transportVersion and this dance with it. It
> seems, the transport version is always enforced by the TransportOnLoad
> function anyway. At the line 459, you could just have:
> 459 info->transportVersion = JDWPTRANSPORT_VERSION_1_0; Thanks, Serguei
>
> On 3/17/17 00:03, [hidden email] wrote:
>> Hi Dmitry, On 3/15/17 00:56, Dmitry Samersoff wrote:
>>> Serguei,
>>>> I still see the C .vs. C++ related change in the jdwpTransport.h.
>>> done. http://cr.openjdk.java.net/~dsamersoff/JDK-8061228/webrev.12/ 
>> Good. Thank you for the update!
>>> see also inline. On 2017-03-15 00:40, [hidden email] wrote:
>>>> Hi Dmitry, We already had a big review thread back in 2014 on this.
>>>> I think, it is again going in the wrong order. First, I think, it is
>>>> better to start from a CCC (or its equivalent, CSR). This will allow
>>>> to focus on and sort out the changes in interface/behavior first
>>>> before going into the implementation details.
>>> CCC was filed and approved. The only reason I withdraw it - the fix
>>> didn't go to jdk9 but CCC tool doesn't have jdk10. CSR process is
>>> also not yet implemented.
>> The CSR preview message from Joe is attached. My understanding is that
>> we can continue to use CCC. At some points the CCC process will be
>> moved to the CSR. The CCC is out of date now as it does not match the
>> webrev 12. Also, I do not like the addition of new function
>> StartListening11() next to StartListening(). Does it mean, that for
>> transport version 1.2 we might have more function clones with 12
>> suffix? Perhaps, we need something smarter here but I'm unsure yet
>> what it has to be.
>>>> Second, I'd suggest to separate a couple of other things. I still
>>>> see the C .vs. C++ related change in the jdwpTransport.h. It is
>>>> better to leave it along for now as it was suggested in early review
>>>> rounds. If you still want to fix it then it is better to file a
>>>> separate bug that should include the JNI as well (as it was
>>>> discussed with Alan before).
>>> Do you mean?    39 #ifdef __cplusplus    40 extern "C" {    41 #endif
>>> I'll add it back to avoid any confusion.
>> Yes. I see you added it back in new version of webrev.
>>>> Also, I'm thinking if it is a good idea to separate the transport
>>>> versioning to an RFE. It would allow to focus on this aspect as
>>>> necessary. In this case, the 8061228 will depend on the versioning.
>>>> However, it is much more simple and can be resolved faster.
>>> It's hard to test versioning code without implementation of new,
>>> VERSION_1_1 transport. Network part of 8061228 is simple and clear
>>> separated from versioning, so I would prefer to keep it together in
>>> one CR/one push.
>> No pressure. I've got your point above. Thanks, Serguei
>>> Restriction turned off by default (I'll file separate CR to enable it
>>> later), so we have enough time to fix any issues that might come.
>>> -Dmitry
>>>> Thanks, Serguei On 2/28/17 01:41, 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.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

Robbin Ehn
In reply to this post by Dmitry Samersoff
Hi Dmitry, sorry for the delay.

Yes thanks, everything seems to work.

Code looks reasonable.

/Robbin

On 03/29/2017 05:10 PM, 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
>>>>>
>>>
>>>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

serguei.spitsyn@oracle.com
In reply to this post by Dmitry Samersoff
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
>>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

serguei.spitsyn@oracle.com
In reply to this post by Dmitry Samersoff
(The formatting is broken in my reply.
I've changed my Zhunderbird 'Send Options' to avoid this in the future.
I tried to fix the formatting below.)

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.

The returned 'ver' is not checked for an error after the second *OnLoad call.



(5) Memory allocation for the info->allow filed
     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
>>>>>
>>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

Dmitry Samersoff
In reply to this post by serguei.spitsyn@oracle.com
Serguei,

Please see my comments in-line.


On 2017-05-10 00:42, [hidden email] 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@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
>>>>>>
>>>>
>>


--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

serguei.spitsyn@oracle.com
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, [hidden email] 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
>>>>>>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

serguei.spitsyn@oracle.com
Dmitry,

Please, see one correction below.



On 5/10/17 02:37, [hidden email] 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, [hidden email] 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@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
>>>>>>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

Dmitry Samersoff
In reply to this post by serguei.spitsyn@oracle.com
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, [hidden email] 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, [hidden email] 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
>>>>>>>>
>>
>


--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR(M): JDK-8061228 Allow JDWP socket connector to accept connections from certain ip addresses only

Robbin Ehn
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, [hidden email] 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, [hidden email] 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
>>>>>>>>>
>>>
>>
>
>
12
Loading...