date:20181101


On 11/1/2018 11:24 AM, Sean Mullan wrote:

On 10/31/18 11:52 AM, Chris Hegarty wrote:

Xuelei,

On 30/10/18 20:55, Xuelei Fan wrote:

Hi,

For the current HttpsURLConnection, there is not much security 
parameters exposed in the public APIs.  An application may need 
richer information for the underlying TLS connections, for example 
the negotiated TLS protocol version.


Please let me know if you have concerns to add a new method 
HttpsURLConnection.getSSLSession() and deprecate the duplicated 
methods, by the end of Nov. 2, 2018.


Here is the proposal:
 https://bugs.openjdk.java.net/browse/JDK-8213161


Are there any security issues associated with returning the SSLSession, 
since it is mutable?
It should be fine.  The update APIs of the session (invalidating, bind 
values) does not impact the connection.




+ *   SHOULD override this method with appropriate 
implementation.


s/appropriate/an appropriate/

I would probably not capitalize "SHOULD" and just say "should". "SHOULD" 
is more common in RFCs. I don't see that much in javadocs.


+ * @implNote The JDK Reference Implementation supports this operation.
+ *   As an application may have to use this operation for more
+ *   security parameters, it is recommended to support this
+ *   operation in all implementations.

I think it should be obvious that the JDK implementation would override 
this method so not sure that first sentence is necessary. The other 
sentence seems like it could be combined with the previous sentence, ex:


"Subclasses should override this method with an appropriate 
implementation since an application may need to access additional 
parameters associated with the SSL session."



Updated accordingly, in the CSR and webrev:
https://bugs.openjdk.java.net/browse/JDK-8213161
http://cr.openjdk.java.net/~xuelei/8212261/webrev.02/

Thanks,
Xuelei

Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged


On 11/1/18 12:39 PM, Sean Mullan wrote:
I also replaced getCallerPD with the faster getProtectionDomain and 
removed a stale comment about impliesCreateAccessControlContext being 
called by the VM. It should be safe to remove now, but I left it in 
to minimize changes.


I would just remove it, so we don't forget about it later. 


OK, I removed it:

http://cr.openjdk.java.net/~dlong/8212605/webrev.4.incr/

dl

Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged

2018-11-01 Thread Mandy Chung




On 11/1/18 2:34 PM, dean.l...@oracle.com wrote:



@Hidden is internal and no CSR is needed.

FYI.  JDK-8212620 is the RFE to consider providing a standard 
mechanism to hide frames from stack trace.




OK, I already filed JDK-8213234 but I think I should just close it as 
a duplicate of JDK-8212620.




JDK-8212620 likely does not make JDK 12.  I assume Vladimir suggests to 
do the rename with your fix (or a follow up).


Mandy

Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged


On 11/1/18 1:45 PM, Mandy Chung wrote:



On 11/1/18 1:18 AM, Vladimir Ivanov wrote:


I think it's a good idea, but I believe it would require a CSR 
request. Do you mind if I file a separate issue for 
jdk.internal.vm.annotation.Hidden?


Sure.

Most of the annotations in jdk.internal.vm.annotation were originally 
located in java.lang.invoke. Not sure CSR will be required in this 
particular case.




@Hidden is internal and no CSR is needed.

FYI.  JDK-8212620 is the RFE to consider providing a standard 
mechanism to hide frames from stack trace.




OK, I already filed JDK-8213234 but I think I should just close it as a 
duplicate of JDK-8212620.


dl


Mandy


Best regards,
Vladimir Ivanov


On 10/31/18 6:11 PM, Vladimir Ivanov wrote:

Dean,

src/java.base/share/classes/java/security/AccessController.java:
+    /**
+ * Internal marker for hidden implementation frames.
+ */
+    /*non-public*/
+    @Target(ElementType.METHOD)
+    @Retention(RetentionPolicy.RUNTIME)
+    @interface Hidden {
+    }

You declare @Hidden, but then map it to _method_Hidden along with 
@Hidden from java.lang.invoke.LambdaForm.


What do you think about moving LambdaForm.Hidden to 
jdk.internal.vm.annotation instead and share among all usages?


Best regards,
Vladimir Ivanov

On 31/10/2018 15:23, dean.l...@oracle.com wrote:

https://bugs.openjdk.java.net/browse/JDK-8212605
http://cr.openjdk.java.net/~dlong/8212605/webrev.1

This change implements AccessController.doPrivileged in Java. This 
gives a performance improvement while also being useful to Project 
Loom by removing the Java --> native --> Java transition.  One 
reason doPrivileged has historically been in native is because of 
the need to guarantee the cleanup of the privileged context when 
doPrivileged returns.  To do that in Java, the information that 
AccessController.getContext needs is pushed onto the Java stack as 
arguments to a method that getContext will recognize during its 
stack walk.  This allows us to remove the native privileged stack 
while guaranteeing that the privileged context goes away when the 
method returns.


Tested with tier1-tier3 hotspot and jdk tests and JCK 
api/java_security tests.  For the first few rounds of testing, I 
kept the old native privileged stack and compared the results of 
the old and new implementations for each getContext call, which 
did catch some early bugs.  The changes were also examined by 
internal security experts and run through additional internal 
security tests.


The improvement on this [1] doPrivileged microbenchmark is 
approximate 50x.


There is no attempt to optimize getContext() or security 
permission checks in this change, however, this is intended to be 
a first step towards other possible improvements, for example 
those proposed here [2].


dl

[1] 
http://hg.openjdk.java.net/code-tools/jmh-jdk-microbenchmarks/file/fc4783360f58/src/main/java/org/openjdk/bench/java/security/DoPrivileged.java 

[2] 
http://mail.openjdk.java.net/pipermail/security-dev/2017-December/016627.html

Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged

2018-11-01 Thread Mandy Chung




On 11/1/18 1:18 AM, Vladimir Ivanov wrote:


I think it's a good idea, but I believe it would require a CSR 
request. Do you mind if I file a separate issue for 
jdk.internal.vm.annotation.Hidden?


Sure.

Most of the annotations in jdk.internal.vm.annotation were originally 
located in java.lang.invoke. Not sure CSR will be required in this 
particular case.




@Hidden is internal and no CSR is needed.

FYI.  JDK-8212620 is the RFE to consider providing a standard mechanism 
to hide frames from stack trace.


Mandy


Best regards,
Vladimir Ivanov


On 10/31/18 6:11 PM, Vladimir Ivanov wrote:

Dean,

src/java.base/share/classes/java/security/AccessController.java:
+    /**
+ * Internal marker for hidden implementation frames.
+ */
+    /*non-public*/
+    @Target(ElementType.METHOD)
+    @Retention(RetentionPolicy.RUNTIME)
+    @interface Hidden {
+    }

You declare @Hidden, but then map it to _method_Hidden along with 
@Hidden from java.lang.invoke.LambdaForm.


What do you think about moving LambdaForm.Hidden to 
jdk.internal.vm.annotation instead and share among all usages?


Best regards,
Vladimir Ivanov

On 31/10/2018 15:23, dean.l...@oracle.com wrote:

https://bugs.openjdk.java.net/browse/JDK-8212605
http://cr.openjdk.java.net/~dlong/8212605/webrev.1

This change implements AccessController.doPrivileged in Java. This 
gives a performance improvement while also being useful to Project 
Loom by removing the Java --> native --> Java transition.  One 
reason doPrivileged has historically been in native is because of 
the need to guarantee the cleanup of the privileged context when 
doPrivileged returns.  To do that in Java, the information that 
AccessController.getContext needs is pushed onto the Java stack as 
arguments to a method that getContext will recognize during its 
stack walk.  This allows us to remove the native privileged stack 
while guaranteeing that the privileged context goes away when the 
method returns.


Tested with tier1-tier3 hotspot and jdk tests and JCK 
api/java_security tests.  For the first few rounds of testing, I 
kept the old native privileged stack and compared the results of 
the old and new implementations for each getContext call, which did 
catch some early bugs.  The changes were also examined by internal 
security experts and run through additional internal security tests.


The improvement on this [1] doPrivileged microbenchmark is 
approximate 50x.


There is no attempt to optimize getContext() or security permission 
checks in this change, however, this is intended to be a first step 
towards other possible improvements, for example those proposed 
here [2].


dl

[1] 
http://hg.openjdk.java.net/code-tools/jmh-jdk-microbenchmarks/file/fc4783360f58/src/main/java/org/openjdk/bench/java/security/DoPrivileged.java 

[2] 
http://mail.openjdk.java.net/pipermail/security-dev/2017-December/016627.html

Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged

On 11/1/18 3:21 PM, dean.l...@oracle.com wrote:

On 11/1/18 9:48 AM, Sean Mullan wrote:

On 11/1/18 1:29 AM, dean.l...@oracle.com wrote:

On 10/31/18 9:39 PM, Bernd Eckenfels wrote:
http://cr.openjdk.java.net/~dlong/8212605/webrev.1/src/java.base/share/classes/java/security/AccessController.java.udiff.html

In checkContext should the security manager be null checked first
instead of last to optimize for the typical case? (If the side
effects in that expression are desired it should be documented)

I was following the example of createWrapper. The side-effects of
getInnocuousAcc() will only happen once, so the order shouldn't
matter here, except for performance reasons. I don't have a strong
opinion about the order, but it looks like the typical case for
createWrapper would also be the typical case for checkContext, so
maybe they both should be changed, if indeed a null security manager
is the more typical case.

A null SM should be the more common case. I am ok with changing the
order so the SM is checked first, but it should be done in both the
createWrapper and checkContext methods. Alternatively, we could see if
they could be combined to eliminate the duplicate code but that might
not be practical from looking at them.

I don't see a way to do it without using a lambda or doing extra work in
one version, so I just changed the order for now.

Yes, I was thinking about using a lambda too, but I'm a bit wary of
using lambdas in the security checking code. Best to keep the methods
separate for now.

I also replaced
getCallerPD with the faster getProtectionDomain and removed a stale
comment about impliesCreateAccessControlContext being called by the VM.
It should be safe to remove now, but I left it in to minimize changes.

I would just remove it, so we don't forget about it later.

Looks good otherwise.

--Sean

http://cr.openjdk.java.net/~dlong/8212605/webrev.3.incr/
http://cr.openjdk.java.net/~dlong/8212605/webrev.3/

--Sean

Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged


On 11/1/18 9:48 AM, Sean Mullan wrote:

On 11/1/18 1:29 AM, dean.l...@oracle.com wrote:

On 10/31/18 9:39 PM, Bernd Eckenfels wrote:
http://cr.openjdk.java.net/~dlong/8212605/webrev.1/src/java.base/share/classes/java/security/AccessController.java.udiff.html 



In checkContext should the security manager be null checked first 
instead of last to optimize for the typical case? (If the side 
effects in that expression are desired it should be documented)


I was following the example of createWrapper.  The side-effects of 
getInnocuousAcc() will only happen once, so the order shouldn't 
matter here, except for performance reasons.  I don't have a strong 
opinion about the order, but it looks like the typical case for 
createWrapper would also be the typical case for checkContext, so 
maybe they both should be changed, if indeed a null security manager 
is the more typical case.


A null SM should be the more common case. I am ok with changing the 
order so the SM is checked first, but it should be done in both the 
createWrapper and checkContext methods. Alternatively, we could see if 
they could be combined to eliminate the duplicate code but that might 
not be practical from looking at them.




I don't see a way to do it without using a lambda or doing extra work in 
one version, so I just changed the order for now.  I also replaced 
getCallerPD with the faster getProtectionDomain and removed a stale 
comment about impliesCreateAccessControlContext being called by the VM.  
It should be safe to remove now, but I left it in to minimize changes.


http://cr.openjdk.java.net/~dlong/8212605/webrev.3.incr/
http://cr.openjdk.java.net/~dlong/8212605/webrev.3/

dl


--Sean

Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged


On 11/1/18 10:01 AM, Sean Mullan wrote:

Some of the copyrights need to be updated to 2018.



Fixed.

All else looks good to me as I had reviewed an earlier version of this 
before. We have talked about doing this for a while now, so I am 
finally glad we and are able to pretty much eliminate one of the more 
common SecurityManager related hot-spots and give a performance boost 
to applications that don't use the SM as well.




Thanks Sean.

dl


--Sean

On 10/31/18 6:23 PM, dean.l...@oracle.com wrote:

https://bugs.openjdk.java.net/browse/JDK-8212605
http://cr.openjdk.java.net/~dlong/8212605/webrev.1

This change implements AccessController.doPrivileged in Java. This 
gives a performance improvement while also being useful to Project 
Loom by removing the Java --> native --> Java transition.  One reason 
doPrivileged has historically been in native is because of the need 
to guarantee the cleanup of the privileged context when doPrivileged 
returns.  To do that in Java, the information that 
AccessController.getContext needs is pushed onto the Java stack as 
arguments to a method that getContext will recognize during its stack 
walk.  This allows us to remove the native privileged stack while 
guaranteeing that the privileged context goes away when the method 
returns.


Tested with tier1-tier3 hotspot and jdk tests and JCK 
api/java_security tests.  For the first few rounds of testing, I kept 
the old native privileged stack and compared the results of the old 
and new implementations for each getContext call, which did catch 
some early bugs.  The changes were also examined by internal security 
experts and run through additional internal security tests.


The improvement on this [1] doPrivileged microbenchmark is 
approximate 50x.


There is no attempt to optimize getContext() or security permission 
checks in this change, however, this is intended to be a first step 
towards other possible improvements, for example those proposed here 
[2].


dl

[1] 
http://hg.openjdk.java.net/code-tools/jmh-jdk-microbenchmarks/file/fc4783360f58/src/main/java/org/openjdk/bench/java/security/DoPrivileged.java 

[2] 
http://mail.openjdk.java.net/pipermail/security-dev/2017-December/016627.html

Re: TLSv1.3 fails to read cert chain after HTTP redirect

2018-11-01 Thread Jamil Nimeh

Hi Daniel thanks for bringing this up, this sounds like 
https://bugs.openjdk.java.net/browse/JDK-8212885.  I'm very close to a 
fix on this one, just working out a few issues in testing.


--Jamil

On 10/8/2018 2:34 PM, Daniel Christensen wrote:
I have a custom HostnameVerifier that attempts to examine the 
certificate chain using SSLSession#getPeerCertificates(). After 
upgrading to Java 11, where it seems that TLSv1.3 is used by default, 
I am seeing that getPeerCertificates() throws an 
SSLPeerUnverifiedException after an HTTP redirect has occurred. If I 
force the protocol to TLSv1.2 this does not occur. If there is no 
redirect, then this does not occur.


Is this a bug in Java or a change in behavior with TLSv1.3?

The code below demonstrates the problem when 'protocol' is either 
'TLS' or 'TLSv1.3' and path is '/redirect'.


doTest("TLSv1.3", "/redirect"); // Fails with SSLPeerUnverifiedException
doTest("TLSv1.3", "/content"); // Succeeds
doTest("TLSv1.2", "/redirect"); // Succeeds
doTest("TLSv1.2", "/content"); // Succeeds

    private void doTest(String protocol, String path) throws 
IOException, NoSuchAlgorithmException, KeyManagementException

    {
    whenHttp(server)
    .match(get("/redirect"))
.then(status(HttpStatus.MOVED_PERMANENTLY_301), 
contentType("text/html"), header("Location", "/content"), 
stringContent("redirected"));

    whenHttp(server)
    .match(get("/content"))
    .then(ok(), contentType("text/html"), 
stringContent("ok"));


    URL url = new URL("https", "localhost", server.getPort(), path);
    HttpsURLConnection conn = 
(HttpsURLConnection)url.openConnection();

    SSLContext ctx = SSLContext.getInstance(protocol);
    TrustManager[] tms = {new X509TrustManager()
    {
    @Override public void checkClientTrusted(X509Certificate[] 
chain, String authType){}
    @Override public void checkServerTrusted(X509Certificate[] 
chain, String authType){}
    @Override public X509Certificate[] getAcceptedIssuers() { 
return new X509Certificate[0]; }

    }};
    ctx.init(null, tms, new SecureRandom());
    conn.setSSLSocketFactory(ctx.getSocketFactory());
    conn.setHostnameVerifier(new HostnameVerifier()
    {
    @Override
    public boolean verify(String hostname, SSLSession session)
    {
    java.security.cert.Certificate[] chain = null;
    try
    {
    chain = session.getPeerCertificates();
    }
    catch (SSLPeerUnverifiedException e)
    {
    throw new RuntimeException(e);
    }
    return true;
    }
    });
    int status = conn.getResponseCode();
    assertEquals(200, status);
    }


Thanks,
Dan

Daniel L. Christensen
Distinguished Engineer
Micro Focus
http://www.microfocus.com

Re: A new proposal to add methods to HttpsURLConnection to access SSLSession


On 10/31/18 11:52 AM, Chris Hegarty wrote:

Xuelei,

On 30/10/18 20:55, Xuelei Fan wrote:

Hi,

For the current HttpsURLConnection, there is not much security 
parameters exposed in the public APIs.  An application may need richer 
information for the underlying TLS connections, for example the 
negotiated TLS protocol version.


Please let me know if you have concerns to add a new method 
HttpsURLConnection.getSSLSession() and deprecate the duplicated 
methods, by the end of Nov. 2, 2018.


Here is the proposal:
 https://bugs.openjdk.java.net/browse/JDK-8213161


Are there any security issues associated with returning the SSLSession, 
since it is mutable?


+ *   SHOULD override this method with appropriate 
implementation.


s/appropriate/an appropriate/

I would probably not capitalize "SHOULD" and just say "should". "SHOULD" 
is more common in RFCs. I don't see that much in javadocs.


+ * @implNote The JDK Reference Implementation supports this operation.
+ *   As an application may have to use this operation for more
+ *   security parameters, it is recommended to support this
+ *   operation in all implementations.

I think it should be obvious that the JDK implementation would override 
this method so not sure that first sentence is necessary. The other 
sentence seems like it could be combined with the previous sentence, ex:


"Subclasses should override this method with an appropriate 
implementation since an application may need to access additional 
parameters associated with the SSL session."



--Sean

Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged

2018-11-01 Thread Ioi Lam





On 10/31/18 5:13 PM, dean.l...@oracle.com wrote:

On 10/31/18 4:06 PM, David Holmes wrote:

Hi Dean,

Looking only at the hotspot changes. The removal of the DoPrivileged 
and related privileged_stack code seems okay. I have a few related 
comments:


src/hotspot/share/classfile/systemDictionary.hpp

You added the java_security_AccessController class after 
java_security_AccessControlContext. Did you actually verify where in 
the load/initialization order the AccessController class appears 
today, and where it appears after your change? (Note the comments at 
the start of WK_KLASSES_DO). Changes to the initialization order 
would be my biggest concern with this patch.




No, I did not notice that comment and assumed alphabetical order was 
OK here.  However, these classes appear to be only resolved, not 
initialized (and AccessController does not have a static initializer), 
so could you explain how the order in this list can affect 
initialization order?


I only need this in JVM_GetStackAccessControlContext, which is a 
static JNI method, so I could get the klass* from the incoming jclass 
instead of using a well-known class entry.


Hi Dean,

You're correct that those classes are only resolved, and not 
initialized. So the order shouldn't matter too much. However, the order 
of the first few classes is important, as the creation of Object, 
Serializable, Cloneable, String, etc, has to be done in a certain order.


I'm not sure whether the order of the reference classes, 292 classes, 
and box classes are important. I'll experiment of getting rid of the 
separate phases after the call to Universe::fixup_mirrors(). This might 
be relics from old days where the classes were once indeed initialized 
in SystemDictionary::initialize_well_known_classes, which was the old 
name for SystemDictionary::resolve_well_known_classes.


(-XX:+TraceBytecodes shows no Java code is executed before 
resolve_well_known_classes returns).


I filed https://bugs.openjdk.java.net/browse/JDK-8213230

For the time being, I think as long as you put the new AccessController 
class near the existing class AccessControlContext, you should be OK.


Thanks
- Ioi



---

I'm unclear about the change to the test:

test/hotspot/jtreg/runtime/JVMDoPrivileged/DoPrivRunAbstract.jasm

as it still refers to the now non-existent JVM_DoPrivileged in the 
summary. Does this test still make sense? Should it be moved to the 
Java side now it doesn't actually test anything in the VM?




I think these tests still make sense, unless we have similar coverage 
somewhere else.  How about if I fix the reference to JVM_DoPrivileged 
for now and file a separate bug about moving or removing these tests?



---

There may be further dead code to remove now:

vmSymbols.hpp: codesource_permissioncollection_signature is now 
unreferenced and can be removed.


javaClasses.*:
  - java_lang_System::has_security_manager
  - java_security_AccessControlContext::is_authorized

./share/memory/universe.hpp:  static Method* 
protection_domain_implies_method()




Good catches!  I have uploaded a new webrev:

http://cr.openjdk.java.net/~dlong/8212605/webrev.2/
http://cr.openjdk.java.net/~dlong/8212605/webrev.2.incr/ (incremental 
diff)


dl


---

Thanks,
David


On 1/11/2018 8:23 AM, dean.l...@oracle.com wrote:

https://bugs.openjdk.java.net/browse/JDK-8212605
http://cr.openjdk.java.net/~dlong/8212605/webrev.1

This change implements AccessController.doPrivileged in Java. This 
gives a performance improvement while also being useful to Project 
Loom by removing the Java --> native --> Java transition.  One 
reason doPrivileged has historically been in native is because of 
the need to guarantee the cleanup of the privileged context when 
doPrivileged returns.  To do that in Java, the information that 
AccessController.getContext needs is pushed onto the Java stack as 
arguments to a method that getContext will recognize during its 
stack walk.  This allows us to remove the native privileged stack 
while guaranteeing that the privileged context goes away when the 
method returns.


Tested with tier1-tier3 hotspot and jdk tests and JCK 
api/java_security tests.  For the first few rounds of testing, I 
kept the old native privileged stack and compared the results of the 
old and new implementations for each getContext call, which did 
catch some early bugs.  The changes were also examined by internal 
security experts and run through additional internal security tests.


The improvement on this [1] doPrivileged microbenchmark is 
approximate 50x.


There is no attempt to optimize getContext() or security permission 
checks in this change, however, this is intended to be a first step 
towards other possible improvements, for example those proposed here 
[2].


dl

[1] 
http://hg.openjdk.java.net/code-tools/jmh-jdk-microbenchmarks/file/fc4783360f58/src/main/java/org/openjdk/bench/java/security/DoPrivileged.java 

[2]

Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged

2018-11-01 Thread Vladimir Ivanov


Dean,

src/java.base/share/classes/java/security/AccessController.java:
+/**
+ * Internal marker for hidden implementation frames.
+ */
+/*non-public*/
+@Target(ElementType.METHOD)
+@Retention(RetentionPolicy.RUNTIME)
+@interface Hidden {
+}

You declare @Hidden, but then map it to _method_Hidden along with 
@Hidden from java.lang.invoke.LambdaForm.


What do you think about moving LambdaForm.Hidden to 
jdk.internal.vm.annotation instead and share among all usages?


Best regards,
Vladimir Ivanov

On 31/10/2018 15:23, dean.l...@oracle.com wrote:

https://bugs.openjdk.java.net/browse/JDK-8212605
http://cr.openjdk.java.net/~dlong/8212605/webrev.1

This change implements AccessController.doPrivileged in Java.  This 
gives a performance improvement while also being useful to Project Loom 
by removing the Java --> native --> Java transition.  One reason 
doPrivileged has historically been in native is because of the need to 
guarantee the cleanup of the privileged context when doPrivileged 
returns.  To do that in Java, the information that 
AccessController.getContext needs is pushed onto the Java stack as 
arguments to a method that getContext will recognize during its stack 
walk.  This allows us to remove the native privileged stack while 
guaranteeing that the privileged context goes away when the method returns.


Tested with tier1-tier3 hotspot and jdk tests and JCK api/java_security 
tests.  For the first few rounds of testing, I kept the old native 
privileged stack and compared the results of the old and new 
implementations for each getContext call, which did catch some early 
bugs.  The changes were also examined by internal security experts and 
run through additional internal security tests.


The improvement on this [1] doPrivileged microbenchmark is approximate 50x.

There is no attempt to optimize getContext() or security permission 
checks in this change, however, this is intended to be a first step 
towards other possible improvements, for example those proposed here [2].


dl

[1] 
http://hg.openjdk.java.net/code-tools/jmh-jdk-microbenchmarks/file/fc4783360f58/src/main/java/org/openjdk/bench/java/security/DoPrivileged.java 

[2] 
http://mail.openjdk.java.net/pipermail/security-dev/2017-December/016627.html

Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged

2018-11-01 Thread Vladimir Ivanov




I think it's a good idea, but I believe it would require a CSR request. 
Do you mind if I file a separate issue for 
jdk.internal.vm.annotation.Hidden?


Sure.

Most of the annotations in jdk.internal.vm.annotation were originally 
located in java.lang.invoke. Not sure CSR will be required in this 
particular case.


Best regards,
Vladimir Ivanov


On 10/31/18 6:11 PM, Vladimir Ivanov wrote:

Dean,

src/java.base/share/classes/java/security/AccessController.java:
+    /**
+ * Internal marker for hidden implementation frames.
+ */
+    /*non-public*/
+    @Target(ElementType.METHOD)
+    @Retention(RetentionPolicy.RUNTIME)
+    @interface Hidden {
+    }

You declare @Hidden, but then map it to _method_Hidden along with 
@Hidden from java.lang.invoke.LambdaForm.


What do you think about moving LambdaForm.Hidden to 
jdk.internal.vm.annotation instead and share among all usages?


Best regards,
Vladimir Ivanov

On 31/10/2018 15:23, dean.l...@oracle.com wrote:

https://bugs.openjdk.java.net/browse/JDK-8212605
http://cr.openjdk.java.net/~dlong/8212605/webrev.1

This change implements AccessController.doPrivileged in Java. This 
gives a performance improvement while also being useful to Project 
Loom by removing the Java --> native --> Java transition.  One reason 
doPrivileged has historically been in native is because of the need 
to guarantee the cleanup of the privileged context when doPrivileged 
returns.  To do that in Java, the information that 
AccessController.getContext needs is pushed onto the Java stack as 
arguments to a method that getContext will recognize during its stack 
walk.  This allows us to remove the native privileged stack while 
guaranteeing that the privileged context goes away when the method 
returns.


Tested with tier1-tier3 hotspot and jdk tests and JCK 
api/java_security tests.  For the first few rounds of testing, I kept 
the old native privileged stack and compared the results of the old 
and new implementations for each getContext call, which did catch 
some early bugs.  The changes were also examined by internal security 
experts and run through additional internal security tests.


The improvement on this [1] doPrivileged microbenchmark is 
approximate 50x.


There is no attempt to optimize getContext() or security permission 
checks in this change, however, this is intended to be a first step 
towards other possible improvements, for example those proposed here 
[2].


dl

[1] 
http://hg.openjdk.java.net/code-tools/jmh-jdk-microbenchmarks/file/fc4783360f58/src/main/java/org/openjdk/bench/java/security/DoPrivileged.java 

[2] 
http://mail.openjdk.java.net/pipermail/security-dev/2017-December/016627.html

TLSv1.3 fails to read cert chain after HTTP redirect

2018-11-01 Thread Daniel Christensen

I have a custom HostnameVerifier that attempts to examine the certificate chain 
using SSLSession#getPeerCertificates(). After upgrading to Java 11, where it 
seems that TLSv1.3 is used by default, I am seeing that getPeerCertificates() 
throws an SSLPeerUnverifiedException after an HTTP redirect has occurred. If I 
force the protocol to TLSv1.2 this does not occur. If there is no redirect, 
then this does not occur.

Is this a bug in Java or a change in behavior with TLSv1.3?

The code below demonstrates the problem when 'protocol' is either 'TLS' or 
'TLSv1.3' and path is '/redirect'.

doTest("TLSv1.3", "/redirect"); // Fails with SSLPeerUnverifiedException
doTest("TLSv1.3", "/content"); // Succeeds
doTest("TLSv1.2", "/redirect"); // Succeeds
doTest("TLSv1.2", "/content"); // Succeeds

private void doTest(String protocol, String path) throws IOException, 
NoSuchAlgorithmException, KeyManagementException
{
whenHttp(server)
.match(get("/redirect"))
.then(status(HttpStatus.MOVED_PERMANENTLY_301), 
contentType("text/html"), header("Location", "/content"), 
stringContent("redirected"));
whenHttp(server)
.match(get("/content"))
.then(ok(), contentType("text/html"), 
stringContent("ok"));

URL url = new URL("https", "localhost", server.getPort(), path);
HttpsURLConnection conn = (HttpsURLConnection)url.openConnection();
SSLContext ctx = SSLContext.getInstance(protocol);
TrustManager[] tms = {new X509TrustManager()
{
@Override public void checkClientTrusted(X509Certificate[] 
chain, String authType){}
@Override public void checkServerTrusted(X509Certificate[] 
chain, String authType){}
@Override public X509Certificate[] getAcceptedIssuers() { 
return new X509Certificate[0]; }
}};
ctx.init(null, tms, new SecureRandom());
conn.setSSLSocketFactory(ctx.getSocketFactory());
conn.setHostnameVerifier(new HostnameVerifier()
{
@Override
public boolean verify(String hostname, SSLSession session)
{
java.security.cert.Certificate[] chain = null;
try
{
chain = session.getPeerCertificates();
}
catch (SSLPeerUnverifiedException e)
{
throw new RuntimeException(e);
}
return true;
}
});
int status = conn.getResponseCode();
assertEquals(200, status);
}


Thanks,
Dan

Daniel L. Christensen
Distinguished Engineer
Micro Focus
http://www.microfocus.com


<>

Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged


On 10/31/18 9:15 PM, Peter wrote:

Hello Dean & David,

Interesting reading.   Is DomainCombiner still being considered for 
deprecation?


There are no immediate plans to deprecate DomainCombiner, but we are 
looking at the SecurityManager (SM) and all of its related APIs and 
evaluating different paths forward, which could include deprecation or 
simplification. In the near term, we are looking at small SM-related 
enhancements which improve performance (such as this) or other things 
like removing legacy SM APIs and/or code.


I had expressed some initial thoughts on this topic here and hope to 
share some more ideas in the near future:


http://mail.openjdk.java.net/pipermail/security-dev/2018-September/018273.html

In the meantime, please let me know if you have any thoughts or ideas on 
this topic.


--Sean

Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged

Hi Peter.  Thanks for the input.  I don't know about DomainCombiner, but 
perhaps someone else on this list does.


dl

On 10/31/18 6:15 PM, Peter wrote:

Hello Dean & David,

Interesting reading.   Is DomainCombiner still being considered for 
deprecation?


Our code makes heavy use of AccessController.doPrivileged.

Regards,

Peter.

"Hot Spots - Method","Self time [%]","Self time","Self time 
(CPU)","Samples"
"java.net.DualStackPlainSocketImpl.accept0[native]()","38.88627","1837851.568 
ms","1837851.568 ms","16"
"java.net.SocketInputStream.socketRead0[native]()","20.505322","969127.11 
ms","969127.11 ms","315"
"java.net.TwoStacksPlainDatagramSocketImpl.peekData[native]()","20.19654","954533.467 
ms","954533.467 ms","11"
"sun.management.ThreadImpl.dumpThreads0[native]()","11.148865","526920.162 
ms","526920.162 ms","239"
"java.net.TwoStacksPlainDatagramSocketImpl.socketNativeSetOption[native]()","5.3582244","253241.609 
ms","253241.609 ms","17"
"sun.misc.Unsafe.unpark[native]()","2.6599514","125715.216 
ms","125715.216 ms","759"
"sun.misc.Unsafe.park[native]()","0.19931908","1.6213131447E7 
ms","9420.263 ms","1401"
"java.util.concurrent.ScheduledThreadPoolExecutor$DelayedWorkQueue.siftDown()","0.15738875","7438.542 
ms","7438.542 ms","71"
"java.security.AccessController.doPrivileged[native]()","0.10248028","4843.446 
ms","4843.446 ms","715"
"java.lang.Thread.isInterrupted[native]()","0.09570652","4523.303 
ms","4523.303 ms","40"
"java.net.NetworkInterface.getAll[native]()","0.07478044","3534.29 
ms","3534.29 ms","4"
"java.util.concurrent.ThreadPoolExecutor.runWorker()","0.046051156","2176.48 
ms","2176.48 ms","91"
"java.lang.SecurityManager.getClassContext[native]()","0.043098312","2036.922 
ms","2036.922 ms","20"
"java.security.AccessController.getStackAccessControlContext[native]()","0.039155032","1850.554 
ms","1850.554 ms","15"
"au.net.zeus.collection.ReferenceProcessor$EnqueGarbageTask.run()","0.035316195","1669.122 
ms","1669.122 ms","18"
"java.net.SocketOutputStream.socketWrite0[native]()","0.03470353","1640.166 
ms","1640.166 ms","17"
"java.lang.Class.forName0[native]()","0.029112596","1375.926 
ms","1375.926 ms","10"
"java.lang.Throwable.fillInStackTrace[native]()","0.028279837","1336.568 
ms","1336.568 ms","18"
"java.lang.Thread.holdsLock[native]()","0.023839759","1126.72 
ms","1126.72 ms","7"

"java.lang.String.indexOf()","0.019056467","900.651 ms","900.651 ms","1"
"au.net.zeus.collection.AbstractReferenceComparator.compare()","0.017307268","817.98 
ms","817.98 ms","18"
"sun.reflect.Reflection.getCallerClass[native]()","0.017031383","804.941 
ms","804.941 ms","11"
"java.net.DualStackPlainSocketImpl.available0[native]()","0.015988858","755.669 
ms","755.669 ms","14"
"java.lang.String.intern[native]()","0.014759737","697.578 
ms","697.578 ms","8"
"au.net.zeus.collection.ReferenceMap.wrapKey()","0.0139064975","657.252 
ms","657.252 ms","4"
"org.apache.river.api.net.Uri.toAsciiLowerCase()","0.013079452","618.164 
ms","618.164 ms","1"
"sun.reflect.DelegatingMethodAccessorImpl.invoke()","0.012204483","576.811 
ms","576.811 ms","753"
"java.lang.Object.clone[native]()","0.011227106","530.618 ms","530.618 
ms","4"
"java.lang.Class.getClassLoader0[native]()","0.010788701","509.898 
ms","509.898 ms","9"
"org.apache.river.impl.thread.SynchronousExecutors$Distributor.run()","0.010243974","484.153 
ms","484.153 ms","3"
"java.util.concurrent.locks.AbstractQueuedSynchronizer.release()","0.008626911","407.727 
ms","407.727 ms","4"
"java.util.concurrent.ConcurrentSkipListMap.keyIterator()","0.008402821","397.136 
ms","397.136 ms","4"
"sun.reflect.misc.ReflectUtil.checkPackageAccess()","0.008291823","391.89 
ms","391.89 ms","8"
"java.util.concurrent.ScheduledThreadPoolExecutor.schedule()","0.0078934925","373.064 
ms","373.064 ms","1"
"sun.management.VMManagementImpl.getDaemonThreadCount[native]()","0.007857989","371.386 
ms","371.386 ms","1"
"java.util.concurrent.ScheduledThreadPoolExecutor$DelayedWorkQueue.take()","0.0068998444","326.102 
ms","326.102 ms","255"
"java.lang.Class.getConstantPool[native]()","0.0065752934","310.763 
ms","310.763 ms","2"
"net.jini.jeri.connection.ConnectionManager.connect()","0.0060991626","8072.071 
ms","288.26 ms","14"
"java.util.concurrent.Executors$RunnableAdapter.call()","0.006036745","285.31 
ms","285.31 ms","797"
"java.util.Collections$SynchronizedCollection.size()","0.0057329717","270.953 
ms","270.953 ms","1"
"sun.management.ThreadImpl.getThreadInfo1[native]()","0.005478752","258.938 
ms","258.938 ms","4"
"java.util.concurrent.ScheduledThreadPoolExecutor.reExecutePeriodic()","0.00411447","194.459 
ms","194.459 ms","3"
"java.lang.reflect.Array.newArray[native]()","0.003456418","163.358 
ms","163.358 ms","1"
"com.sun.jini.jeri.internal.mux.MuxInputStream.read()","0.003161764","149.432 
ms","149.432 ms","17"
"net.jini.loader.pref.PreferredClassProvider.lookupLoader()","0.0031255828","147.722 
ms","147.722 ms","2"
"java.lang.Class.isPrimitive[native]()","0.0030873707","145.916

Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged


Some of the copyrights need to be updated to 2018.

All else looks good to me as I had reviewed an earlier version of this 
before. We have talked about doing this for a while now, so I am finally 
glad we and are able to pretty much eliminate one of the more common 
SecurityManager related hot-spots and give a performance boost to 
applications that don't use the SM as well.


--Sean

On 10/31/18 6:23 PM, dean.l...@oracle.com wrote:

https://bugs.openjdk.java.net/browse/JDK-8212605
http://cr.openjdk.java.net/~dlong/8212605/webrev.1

This change implements AccessController.doPrivileged in Java.  This 
gives a performance improvement while also being useful to Project Loom 
by removing the Java --> native --> Java transition.  One reason 
doPrivileged has historically been in native is because of the need to 
guarantee the cleanup of the privileged context when doPrivileged 
returns.  To do that in Java, the information that 
AccessController.getContext needs is pushed onto the Java stack as 
arguments to a method that getContext will recognize during its stack 
walk.  This allows us to remove the native privileged stack while 
guaranteeing that the privileged context goes away when the method returns.


Tested with tier1-tier3 hotspot and jdk tests and JCK api/java_security 
tests.  For the first few rounds of testing, I kept the old native 
privileged stack and compared the results of the old and new 
implementations for each getContext call, which did catch some early 
bugs.  The changes were also examined by internal security experts and 
run through additional internal security tests.


The improvement on this [1] doPrivileged microbenchmark is approximate 50x.

There is no attempt to optimize getContext() or security permission 
checks in this change, however, this is intended to be a first step 
towards other possible improvements, for example those proposed here [2].


dl

[1] 
http://hg.openjdk.java.net/code-tools/jmh-jdk-microbenchmarks/file/fc4783360f58/src/main/java/org/openjdk/bench/java/security/DoPrivileged.java 

[2] 
http://mail.openjdk.java.net/pipermail/security-dev/2017-December/016627.html

Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged


On 11/1/18 1:29 AM, dean.l...@oracle.com wrote:

On 10/31/18 9:39 PM, Bernd Eckenfels wrote:
http://cr.openjdk.java.net/~dlong/8212605/webrev.1/src/java.base/share/classes/java/security/AccessController.java.udiff.html 



In checkContext should the security manager be null checked first 
instead of last to optimize for the typical case? (If the side effects 
in that expression are desired it should be documented)


I was following the example of createWrapper.  The side-effects of 
getInnocuousAcc() will only happen once, so the order shouldn't matter 
here, except for performance reasons.  I don't have a strong opinion 
about the order, but it looks like the typical case for createWrapper 
would also be the typical case for checkContext, so maybe they both 
should be changed, if indeed a null security manager is the more typical 
case.


A null SM should be the more common case. I am ok with changing the 
order so the SM is checked first, but it should be done in both the 
createWrapper and checkContext methods. Alternatively, we could see if 
they could be combined to eliminate the duplicate code but that might 
not be practical from looking at them.


--Sean

Re: A new proposal to add methods to HttpsURLConnection to access SSLSession


I removed the deprecation parts in the update.  Here is the new webrev:
   http://cr.openjdk.java.net/~xuelei/8212261/webrev.01/

And the CSR was updated accordingly.
   https://bugs.openjdk.java.net/browse/JDK-8213161

Thanks,
Xuelei

On 11/1/2018 4:57 AM, Chris Hegarty wrote:


On 31 Oct 2018, at 20:03, Xuelei Fan > wrote:

...
Yes.  I had the same concern about the optional operation.  However, I 
came to a different conclusion if I'm from viewpoint of these users 
that need to use this new operation.


If an application have to use this new operation, for example to 
access the negotiated TLS version, this operation is essential to it. 
Unfortunately, because of compatibility impact, we cannot force all 
implementation supports this new added operation.  It is a potential 
problem to those applications that need it.


It would be nice if an implementation can support this operation as 
soon as possible.  If we just add the operation, providers may not 
aware there is a need to update their implementation.  Unfortunately. 
there is not much we can do to notify the provider.


If we deprecated the duplicated methods, it is easier for providers to 
catch up with this new added operation, and move forward to support 
it. As the deprecating will show up building warnings, or even error 
if run in strict building mode.


I have no issue with the addition of the new method to access the
SSLSession. Unfortunately, due to the evolutionary constraints of this
API, the `getSSLSession` method will likely need to be specified to
throw UOE. The actual JDK's HTTPS protocol handler implementation will
not throw UOE.

Clearly there is a desire for non-JDK HTTPS protocol handler
implementations to quickly determine the need to update their code and
override the default implementation of `getSSLSession` ( to do something
useful ), hence the request to deprecate the the existing individual
security parameter accessor methods.

I don't believe that deprecating these individual security parameter
accessors is a good idea for the following reasons:

1) Deprecated warnings are only issued at compile-time, so only HTTPS
    protocol handler implementations being recompiled may encounter the
    warnings. Existing binary artifacts will not.

2) More importantly. Forever more, developers wanting access to say,
    the peer principle or the server's certificate chain, will be
    encouraged to get them through an optional API ( rather than a
    non-optional API ). This increases the risk that the code will
    encounter an UOE. I don't think that this increased risk is justified
    by the desire to not-have-two-ways-to-do-the-same-thing. I would
    agree if this were a new API, but it is not.

HTTPS protocol handler implementations actively being maintained will
likely quickly get requests from their users to provide a better
implementation of this new method, as folk move towards TLS 1.3, etc.
Maybe such requests will be sufficient to help adoption, at which point
the deprecation of these methods could then be re-considered.

-Chris.

Re: RFR (12): 8212669: Add note to Cipher javadoc about using full transformation and not relying on defaults


Okay.  Looks fine to me.

Thanks,
Xuelei

On 11/1/2018 8:47 AM, Sean Mullan wrote:

On 11/1/18 11:27 AM, Xuelei Fan wrote:
What do you think if adding a note that the default value may be 
different for each provider, and may be changed from time to time with 
the development of crypto analysis?


I didn't want to get too wordy, just to make a concise point that 
defaults can be problematic and are not recommended. My preference would 
be to put more wording like that in the security guides.


--Sean



Xuelei

On 11/1/2018 7:57 AM, Sean Mullan wrote:
Please review this javadoc-only change to the Cipher class. An 
@apiNote has been added to each of the getInstance methods to 
recommend that the full transformation be specified when creating a 
Cipher and to avoid relying on the defaults. Also a link to the 
defaults used by the JDK providers has been added as an @implNote.


webrev: http://cr.openjdk.java.net/~mullan/webrevs/8212669/webrev.00/
bug: https://bugs.openjdk.java.net/browse/JDK-8212669

Thanks,
Sean

Re: RFR (12): 8212669: Add note to Cipher javadoc about using full transformation and not relying on defaults


On 11/1/18 11:27 AM, Xuelei Fan wrote:
What do you think if adding a note that the default value may be 
different for each provider, and may be changed from time to time with 
the development of crypto analysis?


I didn't want to get too wordy, just to make a concise point that 
defaults can be problematic and are not recommended. My preference would 
be to put more wording like that in the security guides.


--Sean



Xuelei

On 11/1/2018 7:57 AM, Sean Mullan wrote:
Please review this javadoc-only change to the Cipher class. An 
@apiNote has been added to each of the getInstance methods to 
recommend that the full transformation be specified when creating a 
Cipher and to avoid relying on the defaults. Also a link to the 
defaults used by the JDK providers has been added as an @implNote.


webrev: http://cr.openjdk.java.net/~mullan/webrevs/8212669/webrev.00/
bug: https://bugs.openjdk.java.net/browse/JDK-8212669

Thanks,
Sean

Re: [RFR] JDK-8213154: Update copyright headers of files in src tree that are missing Classpath exception

2018-11-01 Thread Martin Balao

Hi Andrew,

Thanks for having a look at this.

Webrev.02 without "All rights reserved" and "affiliates" parts:

 * http://cr.openjdk.java.net/~mbalao/webrevs/8213154/8213154.webrev.02/
 * http://cr.openjdk.java.net/~mbalao/webrevs/8213154/8213154.webrev.02.zip

Are you okay to go?

Kind regards,
Martin.-

On Thu, Nov 1, 2018 at 8:12 AM, Andrew Hughes  wrote:

> On Tue, 30 Oct 2018 at 18:00, Martin Balao  wrote:
> >
> > Hi,
> >
> > You're right, this is not relevant for a test.
> >
> >  * http://cr.openjdk.java.net/~mbalao/webrevs/8213154/8213154.webrev.01
> >  * http://cr.openjdk.java.net/~mbalao/webrevs/8213154/
> 8213154.webrev.01.zip
> >
> > Thanks,
> > Martin.-
> >
> > On Tue, Oct 30, 2018 at 2:50 PM, Alan Bateman 
> wrote:
> >>
> >> On 30/10/2018 17:44, Martin Balao wrote:
> >>
> >> Hi,
> >>
> >> Can I have a review for JDK-8213154 [1]?
> >>
> >>  * http://cr.openjdk.java.net/~mbalao/webrevs/8213154/
> 8213154.webrev.00/
> >>  * http://cr.openjdk.java.net/~mbalao/webrevs/8213154/
> 8213154.webrev.00.zip
> >>
> >> Did you mean to include a test in this update? Just asking because
> tests doesn't usually have the Classpath exception.
> >>
> >> -Alan
> >
> >
>
> Classpath exception addition looks fine and appropriate for the JDK
> code. The "All rights reserved" additions are unnecessary for RH
> copyrights.
> --
> Andrew :)
>
> Senior Free Java Software Engineer
> Red Hat, Inc. (http://www.redhat.com)
>
> Web Site: http://fuseyism.com
> Twitter: https://twitter.com/gnu_andrew_java
> PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
> Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
>

Re: RFR (12): 8212669: Add note to Cipher javadoc about using full transformation and not relying on defaults

What do you think if adding a note that the default value may be 
different for each provider, and may be changed from time to time with 
the development of crypto analysis?


Xuelei

On 11/1/2018 7:57 AM, Sean Mullan wrote:
Please review this javadoc-only change to the Cipher class. An @apiNote 
has been added to each of the getInstance methods to recommend that the 
full transformation be specified when creating a Cipher and to avoid 
relying on the defaults. Also a link to the defaults used by the JDK 
providers has been added as an @implNote.


webrev: http://cr.openjdk.java.net/~mullan/webrevs/8212669/webrev.00/
bug: https://bugs.openjdk.java.net/browse/JDK-8212669

Thanks,
Sean

RFR (12): 8212669: Add note to Cipher javadoc about using full transformation and not relying on defaults

Please review this javadoc-only change to the Cipher class. An @apiNote 
has been added to each of the getInstance methods to recommend that the 
full transformation be specified when creating a Cipher and to avoid 
relying on the defaults. Also a link to the defaults used by the JDK 
providers has been added as an @implNote.


webrev: http://cr.openjdk.java.net/~mullan/webrevs/8212669/webrev.00/
bug: https://bugs.openjdk.java.net/browse/JDK-8212669

Thanks,
Sean

Re: RFR 8212217: JGSS: Don't dispose() of creds too eagerly