Re: RFR: 8270946: X509CertImpl.getFingerprint should not return the empty String
On Fri, 23 Jul 2021 17:16:26 GMT, Sean Mullan wrote:
> Please review this fix to change the internal `X509CertImpl.getFingerprint`
> method to not return "" as a fingerprint if there is an error generating that
> fingerprint. Instead, `null` is now returned, and "" is no longer cached as a
> valid fingerprint. Although errors generating fingerprints should be very
> rare, this is a cleaner way to handle them.
>
> Also, debugging messages have been added when there is an exception. And, as
> a memory/performance improvement, `X509CertImpl.getFingerprint` now calls
> `X509CertImpl.getEncodedInternal` which avoids cloning the encoded bytes if
> the `Certificate` is an instance of `X509CertImpl`.
src/java.base/share/classes/sun/security/validator/SymantecTLSPolicy.java line
161:
> 159: X509Certificate anchor = chain[chain.length-1];
> 160: String fp = fingerprint(anchor);
> 161: if (fp != null && FINGERPRINTS.contains(fp)) {
I understand the original behavior is also bypassing the check if fingerprint
cannot be calculated, but this sounds a little irresponsible. Same as in
`UntrustedCertificates`.
src/java.base/share/classes/sun/security/x509/X509CertImpl.java line 1924:
> 1922: x -> getFingerprintInternal(x, debug));
> 1923: }
> 1924:
I'm a little confused by these methods. Can you inline `getFingerprintInternal`
and rename `getFingerprint` on line 1936 to `getFingerprintInternal`?
test/jdk/sun/security/x509/X509CertImpl/GetFingerprintError.java line 53:
> 51:
> 52: // test cert with bad encoding
> 53: X509Certificate fcert = new X509CertificateWithBadEncoding(cert);
In fact, `new X509CertImpl()` satisfies your requirement perfectly, which is an
unpopulated cert with no encoding. It might be a little weird though. You can
continue with your choice.
-
PR: https://git.openjdk.java.net/jdk/pull/4891
Re: JEP 411 Headaches: Instrumenting private methods in the JDK for authorization checkpoints.
I think it's worth noting that there isn't a way to securely run code with malicious intent now, so I'm surprised that at this late stage you were still providing support for sand boxing (whack a mole). It's just for us many assumptions have been made on a Java platform with SM, using POLP (not sandboxing) as this was one of the foundational principles of secure coding guidelines (just like following concurrency best practice, were were following security best practice). Sandboxing is an all or nothing approach, if you had a trusted applet that was signed, it had AllPermission, if you had an unsigned applet, then it had no permissions. Sandboxing was one of the use cases for SM, when combined with ClassLoader visibility, but we never realized that OpenJDK developers meant sandboxing == authorization access controls. When you remove that pillar, everything it's supporting collapses, not just sand boxing, so when you say you are removing support for sandboxing, we say, good idea, but we didn't realize you were saying you were removing support for all authorization access controls. Reduced and revised authorization and access control would have been acceptable, as tightening reflection visibility using a different form of access control removes the need for authorization based reflection access checks, but also removing atomic construction guarantee's just seems like were doing this at a rapid pace without the community understanding what you have in mind, and this may have more uses than just stopping finalizer attacks. Unfortunately when you develop a feature, you can't be sure developers won't adapt and utilize it for multiple purposes. Personally I think a better approach would have been to first reduce and simplify authorization access controls, replacing some of the functionality with different but more appropriate mechanisms. Removing authorization access control features, without replacement means our software would be insecure, there isn't an obvious way to re-secure it, without re-architecting or re-designing it from the ground up, and Java is now a moving target anyway, so we would have to wait for it to re-stabilize as it transitions from Hippy to Hipster Java (just making a point it's not the same Java). With this new understanding, we need to reconsider both the language and the platform that we'll be developing on. Clearly a language with less boilerplate is an obvious start, I don't yet know which, we will still consider the JVM, but it would be with Kafka or Clojure, but then we also need to consider whether we will be able to secure the underlying platform, or at least use it securely. Arguably we can do things now that aren't possible on other platforms, so we need to develop that capability as well, not just secure it. Regards, Peter. On 23/07/2021 9:45 pm, Alan Bateman wrote: On 23/07/2021 11:48, Peter Firmstone wrote: Perhaps the solution is to replace the entire class, instead of instrumenting one method? Compile a patched copy of the JVM, with modified class files, then replace the existing classes in the JVM with the modified classes? Kinda like maintaining a fork, but using Agents to instrument the original JVM with classes from the fork? I sure wish there was a better option, if anyone knows one, I'm all ears. JEP 411 puts the JDK on the road to dropping support for sandboxing. This means there won't be a built-in means to securely run code that has malicious intent. It means that many of the concerns for finalizer attacks go away too. In the case of the ClassLoader example in your first mail then it may be that the private static method that you want to instrument will be removed. If removed, then it should make the instrumentation a bit easier so that you can instrument the protected constructors to invokestatic your equivalent of a permission check before the invokespecial. So I think this specific case is surmountable but in general I don't think it will be tenable to patch hundreds of classes and be confident that you've got everything, esp. with a moving code base and new features. I can't tell if your "authorization layer" is for use when running with code that has malicious intent. If it is, then I don't think it will be tenable to duplicate all the deeply invasive permission checks that exist today and keep it up to date as new features and changes. When agents were muted in the early discussion on JEP 411 then the context was file and network access where several people were interested in having a means to veto access. Expanding this to have a SM equivalent be able to veto every reflective access, prevent trusted method chain and other attacks, amounts to keeping the SM forever. As regards the comments about agents having the power to instrument methods that aren't accessible to user code then that is normal. Java agents are for tools to do powerful things, they aren't intended
Re: RFR: 8270317: Large Allocation in CipherSuite [v5]
On Fri, 23 Jul 2021 17:08:11 GMT, Clive Verghese wrote:
>> src/java.base/share/classes/sun/security/ssl/CipherSuite.java line 972:
>>
>>> 970: boolean found = false;
>>> 971: CipherSuite cs;
>>> 972: if ((cs = cipherSuiteNames.get(name)) != null &&
>>> !cs.supportedProtocols.isEmpty()) {
>>
>> Nice update! I appreciate if you could avoid lines longer than 80
>> characters, since they’re not handled well by many terminals and tools (See
>> [Java Code
>> Conventions](https://www.oracle.com/java/technologies/javase/codeconventions-indentation.html)).
>
> Thank you @XueleiFan, Updated the length of the line.
Thank you for the update. This PR looks good to me, and I have no further
comment.
-
PR: https://git.openjdk.java.net/jdk/pull/4783
Re: RFR: 8270317: Large Allocation in CipherSuite [v7]
> ### Benchmark results > > I have benchmarked 3 cases. > > 1. The current situation. > > Benchmark > (cipherSuite) Mode CntScore Error Units > CipherSuiteBench.benchmarkCipherSuite > TLS_AES_256_GCM_SHA384 avgt 25 124.783 ? 2.050 ns/op > CipherSuiteBench.benchmarkCipherSuite > TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384 avgt 25 125.403 ? 0.554 ns/op > CipherSuiteBench.benchmarkCipherSuite > TLS_DHE_DSS_WITH_AES_128_CBC_SHA256 avgt 25 127.117 ? 0.789 ns/op > CipherSuiteBench.benchmarkCipherSuite > TLS_DHE_RSA_WITH_AES_256_CBC_SHA avgt 25 127.869 ? 1.112 ns/op > > > 2. Use `static final array` instead of calling `CipherSuite.values` each > time. > > Benchmark > (cipherSuite) Mode Cnt Score Error Units > CipherSuiteBench.benchmarkCipherSuite > TLS_AES_256_GCM_SHA384 avgt 25 10.146 ? 0.252 ns/op > CipherSuiteBench.benchmarkCipherSuite > TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384 avgt 25 30.501 ? 0.207 ns/op > CipherSuiteBench.benchmarkCipherSuite > TLS_DHE_DSS_WITH_AES_128_CBC_SHA256 avgt 25 47.375 ? 0.150 ns/op > CipherSuiteBench.benchmarkCipherSuite > TLS_DHE_RSA_WITH_AES_256_CBC_SHA avgt 25 55.887 ? 3.786 ns/op > > > 3. Using Hashmap for lookup instead of iterating through the array each time. > (Method in this PR) > > Benchmark > (cipherSuite) Mode Cnt Score Error Units > CipherSuiteBench.benchmarkCipherSuite > TLS_AES_256_GCM_SHA384 avgt 25 13.533 ? 0.148 ns/op > CipherSuiteBench.benchmarkCipherSuite > TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384 avgt 25 11.269 ? 0.147 ns/op > CipherSuiteBench.benchmarkCipherSuite > TLS_DHE_DSS_WITH_AES_128_CBC_SHA256 avgt 25 11.507 ? 0.107 ns/op > CipherSuiteBench.benchmarkCipherSuite > TLS_DHE_RSA_WITH_AES_256_CBC_SHA avgt 25 10.932 ? 0.146 ns/op > > > I have picked 4 cipher suite from the start of the list and are roughly 10 > positions apart. I have opted to go with HashMap for name and id lookup as > they provide a more consistent times and benchmarks are similar for the first > few cipher suits in the enum as well. Clive Verghese has updated the pull request incrementally with one additional commit since the last revision: Add new line in end of CipherSuiteBench - Changes: - all: https://git.openjdk.java.net/jdk/pull/4783/files - new: https://git.openjdk.java.net/jdk/pull/4783/files/6e75c829..b0375b25 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4783=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4783=05-06 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4783.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4783/head:pull/4783 PR: https://git.openjdk.java.net/jdk/pull/4783
Re: RFR: 8270317: Large Allocation in CipherSuite [v6]
On Fri, 23 Jul 2021 17:11:30 GMT, Clive Verghese wrote: >> ### Benchmark results >> >> I have benchmarked 3 cases. >> >> 1. The current situation. >> >> Benchmark >> (cipherSuite) Mode CntScore Error Units >> CipherSuiteBench.benchmarkCipherSuite >> TLS_AES_256_GCM_SHA384 avgt 25 124.783 ? 2.050 ns/op >> CipherSuiteBench.benchmarkCipherSuite >> TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384 avgt 25 125.403 ? 0.554 ns/op >> CipherSuiteBench.benchmarkCipherSuite >> TLS_DHE_DSS_WITH_AES_128_CBC_SHA256 avgt 25 127.117 ? 0.789 ns/op >> CipherSuiteBench.benchmarkCipherSuite >> TLS_DHE_RSA_WITH_AES_256_CBC_SHA avgt 25 127.869 ? 1.112 ns/op >> >> >> 2. Use `static final array` instead of calling `CipherSuite.values` each >> time. >> >> Benchmark >> (cipherSuite) Mode Cnt Score Error Units >> CipherSuiteBench.benchmarkCipherSuite >> TLS_AES_256_GCM_SHA384 avgt 25 10.146 ? 0.252 ns/op >> CipherSuiteBench.benchmarkCipherSuite >> TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384 avgt 25 30.501 ? 0.207 ns/op >> CipherSuiteBench.benchmarkCipherSuite >> TLS_DHE_DSS_WITH_AES_128_CBC_SHA256 avgt 25 47.375 ? 0.150 ns/op >> CipherSuiteBench.benchmarkCipherSuite >> TLS_DHE_RSA_WITH_AES_256_CBC_SHA avgt 25 55.887 ? 3.786 ns/op >> >> >> 3. Using Hashmap for lookup instead of iterating through the array each >> time. (Method in this PR) >> >> Benchmark >> (cipherSuite) Mode Cnt Score Error Units >> CipherSuiteBench.benchmarkCipherSuite >> TLS_AES_256_GCM_SHA384 avgt 25 13.533 ? 0.148 ns/op >> CipherSuiteBench.benchmarkCipherSuite >> TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384 avgt 25 11.269 ? 0.147 ns/op >> CipherSuiteBench.benchmarkCipherSuite >> TLS_DHE_DSS_WITH_AES_128_CBC_SHA256 avgt 25 11.507 ? 0.107 ns/op >> CipherSuiteBench.benchmarkCipherSuite >> TLS_DHE_RSA_WITH_AES_256_CBC_SHA avgt 25 10.932 ? 0.146 ns/op >> >> >> I have picked 4 cipher suite from the start of the list and are roughly 10 >> positions apart. I have opted to go with HashMap for name and id lookup as >> they provide a more consistent times and benchmarks are similar for the >> first few cipher suits in the enum as well. > > Clive Verghese has updated the pull request incrementally with one additional > commit since the last revision: > > Fix line length This change looks good to me except the vary minor fact that the test has no newline at the end of the file. I wonder why jcheck hasn't caught this issue. If I remember right it did it in the old Mercurial version. I'd appreciate if you could fix that as well before integrating (and no need for a new review from my side). I'll sponsor the change one you integrate. Thanks for this nice improvement! - Marked as reviewed by simonis (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4783
RFR: 8270946: X509CertImpl.getFingerprint should not return the empty String
Please review this fix to change the internal `X509CertImpl.getFingerprint` method to not return "" as a fingerprint if there is an error generating that fingerprint. Instead, `null` is now returned, and "" is no longer cached as a valid fingerprint. Although errors generating fingerprints should be very rare, this is a cleaner way to handle them. Also, debugging messages have been added when there is an exception. And, as a memory/performance improvement, `X509CertImpl.getFingerprint` now calls `X509CertImpl.getEncodedInternal` which avoids cloning the encoded bytes if the `Certificate` is an instance of `X509CertImpl`. - Commit messages: - Regression test for JDK-8270946. - 8270946: X509CertImpl.getFingerprint should not return the empty String Changes: https://git.openjdk.java.net/jdk/pull/4891/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4891=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8270946 Stats: 225 lines in 6 files changed: 186 ins; 13 del; 26 mod Patch: https://git.openjdk.java.net/jdk/pull/4891.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4891/head:pull/4891 PR: https://git.openjdk.java.net/jdk/pull/4891
Re: RFR: 8270317: Large Allocation in CipherSuite [v5]
On Fri, 23 Jul 2021 17:05:58 GMT, Xue-Lei Andrew Fan wrote:
>> Clive Verghese has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove for loop from validValuesOf
>
> src/java.base/share/classes/sun/security/ssl/CipherSuite.java line 972:
>
>> 970: boolean found = false;
>> 971: CipherSuite cs;
>> 972: if ((cs = cipherSuiteNames.get(name)) != null &&
>> !cs.supportedProtocols.isEmpty()) {
>
> Nice update! I appreciate if you could avoid lines longer than 80
> characters, since they’re not handled well by many terminals and tools (See
> [Java Code
> Conventions](https://www.oracle.com/java/technologies/javase/codeconventions-indentation.html)).
Thank you @XueleiFan, Updated the length of the line.
-
PR: https://git.openjdk.java.net/jdk/pull/4783
Re: RFR: 8270317: Large Allocation in CipherSuite [v5]
On Fri, 23 Jul 2021 16:52:38 GMT, Clive Verghese wrote:
>> ### Benchmark results
>>
>> I have benchmarked 3 cases.
>>
>> 1. The current situation.
>>
>> Benchmark
>> (cipherSuite) Mode CntScore Error Units
>> CipherSuiteBench.benchmarkCipherSuite
>> TLS_AES_256_GCM_SHA384 avgt 25 124.783 ? 2.050 ns/op
>> CipherSuiteBench.benchmarkCipherSuite
>> TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384 avgt 25 125.403 ? 0.554 ns/op
>> CipherSuiteBench.benchmarkCipherSuite
>> TLS_DHE_DSS_WITH_AES_128_CBC_SHA256 avgt 25 127.117 ? 0.789 ns/op
>> CipherSuiteBench.benchmarkCipherSuite
>> TLS_DHE_RSA_WITH_AES_256_CBC_SHA avgt 25 127.869 ? 1.112 ns/op
>>
>>
>> 2. Use `static final array` instead of calling `CipherSuite.values` each
>> time.
>>
>> Benchmark
>> (cipherSuite) Mode Cnt Score Error Units
>> CipherSuiteBench.benchmarkCipherSuite
>> TLS_AES_256_GCM_SHA384 avgt 25 10.146 ? 0.252 ns/op
>> CipherSuiteBench.benchmarkCipherSuite
>> TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384 avgt 25 30.501 ? 0.207 ns/op
>> CipherSuiteBench.benchmarkCipherSuite
>> TLS_DHE_DSS_WITH_AES_128_CBC_SHA256 avgt 25 47.375 ? 0.150 ns/op
>> CipherSuiteBench.benchmarkCipherSuite
>> TLS_DHE_RSA_WITH_AES_256_CBC_SHA avgt 25 55.887 ? 3.786 ns/op
>>
>>
>> 3. Using Hashmap for lookup instead of iterating through the array each
>> time. (Method in this PR)
>>
>> Benchmark
>> (cipherSuite) Mode Cnt Score Error Units
>> CipherSuiteBench.benchmarkCipherSuite
>> TLS_AES_256_GCM_SHA384 avgt 25 13.533 ? 0.148 ns/op
>> CipherSuiteBench.benchmarkCipherSuite
>> TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384 avgt 25 11.269 ? 0.147 ns/op
>> CipherSuiteBench.benchmarkCipherSuite
>> TLS_DHE_DSS_WITH_AES_128_CBC_SHA256 avgt 25 11.507 ? 0.107 ns/op
>> CipherSuiteBench.benchmarkCipherSuite
>> TLS_DHE_RSA_WITH_AES_256_CBC_SHA avgt 25 10.932 ? 0.146 ns/op
>>
>>
>> I have picked 4 cipher suite from the start of the list and are roughly 10
>> positions apart. I have opted to go with HashMap for name and id lookup as
>> they provide a more consistent times and benchmarks are similar for the
>> first few cipher suits in the enum as well.
>
> Clive Verghese has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove for loop from validValuesOf
src/java.base/share/classes/sun/security/ssl/CipherSuite.java line 972:
> 970: boolean found = false;
> 971: CipherSuite cs;
> 972: if ((cs = cipherSuiteNames.get(name)) != null &&
> !cs.supportedProtocols.isEmpty()) {
Nice update! I appreciate if you could avoid lines longer than 80 characters,
since they’re not handled well by many terminals and tools (See [Java Code
Conventions](https://www.oracle.com/java/technologies/javase/codeconventions-indentation.html)).
-
PR: https://git.openjdk.java.net/jdk/pull/4783
Re: RFR: 8270317: Large Allocation in CipherSuite [v6]
> ### Benchmark results > > I have benchmarked 3 cases. > > 1. The current situation. > > Benchmark > (cipherSuite) Mode CntScore Error Units > CipherSuiteBench.benchmarkCipherSuite > TLS_AES_256_GCM_SHA384 avgt 25 124.783 ? 2.050 ns/op > CipherSuiteBench.benchmarkCipherSuite > TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384 avgt 25 125.403 ? 0.554 ns/op > CipherSuiteBench.benchmarkCipherSuite > TLS_DHE_DSS_WITH_AES_128_CBC_SHA256 avgt 25 127.117 ? 0.789 ns/op > CipherSuiteBench.benchmarkCipherSuite > TLS_DHE_RSA_WITH_AES_256_CBC_SHA avgt 25 127.869 ? 1.112 ns/op > > > 2. Use `static final array` instead of calling `CipherSuite.values` each > time. > > Benchmark > (cipherSuite) Mode Cnt Score Error Units > CipherSuiteBench.benchmarkCipherSuite > TLS_AES_256_GCM_SHA384 avgt 25 10.146 ? 0.252 ns/op > CipherSuiteBench.benchmarkCipherSuite > TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384 avgt 25 30.501 ? 0.207 ns/op > CipherSuiteBench.benchmarkCipherSuite > TLS_DHE_DSS_WITH_AES_128_CBC_SHA256 avgt 25 47.375 ? 0.150 ns/op > CipherSuiteBench.benchmarkCipherSuite > TLS_DHE_RSA_WITH_AES_256_CBC_SHA avgt 25 55.887 ? 3.786 ns/op > > > 3. Using Hashmap for lookup instead of iterating through the array each time. > (Method in this PR) > > Benchmark > (cipherSuite) Mode Cnt Score Error Units > CipherSuiteBench.benchmarkCipherSuite > TLS_AES_256_GCM_SHA384 avgt 25 13.533 ? 0.148 ns/op > CipherSuiteBench.benchmarkCipherSuite > TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384 avgt 25 11.269 ? 0.147 ns/op > CipherSuiteBench.benchmarkCipherSuite > TLS_DHE_DSS_WITH_AES_128_CBC_SHA256 avgt 25 11.507 ? 0.107 ns/op > CipherSuiteBench.benchmarkCipherSuite > TLS_DHE_RSA_WITH_AES_256_CBC_SHA avgt 25 10.932 ? 0.146 ns/op > > > I have picked 4 cipher suite from the start of the list and are roughly 10 > positions apart. I have opted to go with HashMap for name and id lookup as > they provide a more consistent times and benchmarks are similar for the first > few cipher suits in the enum as well. Clive Verghese has updated the pull request incrementally with one additional commit since the last revision: Fix line length - Changes: - all: https://git.openjdk.java.net/jdk/pull/4783/files - new: https://git.openjdk.java.net/jdk/pull/4783/files/39b78e65..6e75c829 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4783=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4783=04-05 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4783.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4783/head:pull/4783 PR: https://git.openjdk.java.net/jdk/pull/4783
Re: RFR: 8268873: Unnecessary Vector usage in java.base
On Fri, 18 Jun 2021 18:28:26 GMT, Sean Mullan wrote: >> Usage of thread-safe collection `Vector` is unnecessary. It's recommended to >> use `ArrayList` if a thread-safe implementation is not needed. In >> post-BiasedLocking times, this is gets worse, as every access is >> synchronized. >> I checked only places where `Vector` was used as local variable. > > The change to `PKCS7::verify(byte[])` looks fine. > @seanjmullan Are you planning to sponsor this? Yes, I can do that, but I will wait until Monday in case there are any post-integration issues that I need to follow up on. - PR: https://git.openjdk.java.net/jdk/pull/4482
Re: RFR: 8270317: Large Allocation in CipherSuite [v5]
> ### Benchmark results > > I have benchmarked 3 cases. > > 1. The current situation. > > Benchmark > (cipherSuite) Mode CntScore Error Units > CipherSuiteBench.benchmarkCipherSuite > TLS_AES_256_GCM_SHA384 avgt 25 124.783 ? 2.050 ns/op > CipherSuiteBench.benchmarkCipherSuite > TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384 avgt 25 125.403 ? 0.554 ns/op > CipherSuiteBench.benchmarkCipherSuite > TLS_DHE_DSS_WITH_AES_128_CBC_SHA256 avgt 25 127.117 ? 0.789 ns/op > CipherSuiteBench.benchmarkCipherSuite > TLS_DHE_RSA_WITH_AES_256_CBC_SHA avgt 25 127.869 ? 1.112 ns/op > > > 2. Use `static final array` instead of calling `CipherSuite.values` each > time. > > Benchmark > (cipherSuite) Mode Cnt Score Error Units > CipherSuiteBench.benchmarkCipherSuite > TLS_AES_256_GCM_SHA384 avgt 25 10.146 ? 0.252 ns/op > CipherSuiteBench.benchmarkCipherSuite > TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384 avgt 25 30.501 ? 0.207 ns/op > CipherSuiteBench.benchmarkCipherSuite > TLS_DHE_DSS_WITH_AES_128_CBC_SHA256 avgt 25 47.375 ? 0.150 ns/op > CipherSuiteBench.benchmarkCipherSuite > TLS_DHE_RSA_WITH_AES_256_CBC_SHA avgt 25 55.887 ? 3.786 ns/op > > > 3. Using Hashmap for lookup instead of iterating through the array each time. > (Method in this PR) > > Benchmark > (cipherSuite) Mode Cnt Score Error Units > CipherSuiteBench.benchmarkCipherSuite > TLS_AES_256_GCM_SHA384 avgt 25 13.533 ? 0.148 ns/op > CipherSuiteBench.benchmarkCipherSuite > TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384 avgt 25 11.269 ? 0.147 ns/op > CipherSuiteBench.benchmarkCipherSuite > TLS_DHE_DSS_WITH_AES_128_CBC_SHA256 avgt 25 11.507 ? 0.107 ns/op > CipherSuiteBench.benchmarkCipherSuite > TLS_DHE_RSA_WITH_AES_256_CBC_SHA avgt 25 10.932 ? 0.146 ns/op > > > I have picked 4 cipher suite from the start of the list and are roughly 10 > positions apart. I have opted to go with HashMap for name and id lookup as > they provide a more consistent times and benchmarks are similar for the first > few cipher suits in the enum as well. Clive Verghese has updated the pull request incrementally with one additional commit since the last revision: Remove for loop from validValuesOf - Changes: - all: https://git.openjdk.java.net/jdk/pull/4783/files - new: https://git.openjdk.java.net/jdk/pull/4783/files/158714cf..39b78e65 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4783=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4783=03-04 Stats: 7 lines in 1 file changed: 0 ins; 3 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/4783.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4783/head:pull/4783 PR: https://git.openjdk.java.net/jdk/pull/4783
Re: RFR: 8270317: Large Allocation in CipherSuite [v4]
On Fri, 23 Jul 2021 05:52:25 GMT, djelinski
wrote:
>> Clive Verghese has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add blank space before and after the for loop
>
> src/java.base/share/classes/sun/security/ssl/CipherSuite.java line 971:
>
>> 969:
>> 970: boolean found = false;
>> 971: for (CipherSuite cs : allowedCipherSuites) {
>
> We can avoid this loop; look up the cipher by name first (`cs =
> nameOf(name)`), then check for supported protocols.
> Other than that, LGTM.
Thank you for the feedback. I have updated the loop accordingly.
-
PR: https://git.openjdk.java.net/jdk/pull/4783
Re: RFR: 8268873: Unnecessary Vector usage in java.base
On Fri, 18 Jun 2021 18:28:26 GMT, Sean Mullan wrote: >> Usage of thread-safe collection `Vector` is unnecessary. It's recommended to >> use `ArrayList` if a thread-safe implementation is not needed. In >> post-BiasedLocking times, this is gets worse, as every access is >> synchronized. >> I checked only places where `Vector` was used as local variable. > > The change to `PKCS7::verify(byte[])` looks fine. @seanjmullan Are you planning to sponsor this? - PR: https://git.openjdk.java.net/jdk/pull/4482
Integrated: 8270280: security/infra/java/security/cert/CertPathValidator/certification/LetsEncryptCA.java OCSP response error
On Thu, 22 Jul 2021 17:29:32 GMT, Rajan Halade wrote: > I have updated revoked test certificate but this test may again fail in Sept > as test certificate expire leading to OCSP error. > > CA is not willing to issue test certificates with more than 90 day validity > so this test will fail every quarter. I am re-thinking the CA certification > testing approach to may be try a TLS connection with test websites. This will > ensure that test will pass as long as CA keeps test website updated. This pull request has now been integrated. Changeset: f4b3ee5d Author:Rajan Halade URL: https://git.openjdk.java.net/jdk/commit/f4b3ee5dca8cfdc2fbb8ee64a1e8cdb8894b0061 Stats: 29 lines in 2 files changed: 0 ins; 4 del; 25 mod 8270280: security/infra/java/security/cert/CertPathValidator/certification/LetsEncryptCA.java OCSP response error Reviewed-by: mullan - PR: https://git.openjdk.java.net/jdk/pull/4877
Re: RFR: 8270280: security/infra/java/security/cert/CertPathValidator/certification/LetsEncryptCA.java OCSP response error
On Fri, 23 Jul 2021 15:00:44 GMT, Sean Mullan wrote: > But you could cache the OCSPResponse now while the certificate is not > expired, and use that in the test by calling > `PKIXRevocationChecker.setOcspResponses()`. For CRLs, you could also do > something similar by caching the CRL and storing it in `CollectionCertStore` > and adding that to `PKIXParameters`. Just some ideas to avoid having to > continuously update the test certificates every 3 months. > > I can approve this now, but can you file a follow-on issue to look into this > some more? Sure. I will investigate this along with idea of using TLS connection to test websites. Thanks! - PR: https://git.openjdk.java.net/jdk/pull/4877
Re: RFR: 8270280: security/infra/java/security/cert/CertPathValidator/certification/LetsEncryptCA.java OCSP response error
On Thu, 22 Jul 2021 17:29:32 GMT, Rajan Halade wrote: > I have updated revoked test certificate but this test may again fail in Sept > as test certificate expire leading to OCSP error. > > CA is not willing to issue test certificates with more than 90 day validity > so this test will fail every quarter. I am re-thinking the CA certification > testing approach to may be try a TLS connection with test websites. This will > ensure that test will pass as long as CA keeps test website updated. Marked as reviewed by mullan (Reviewer). But you could cache the OCSPResponse now while the certificate is not expired, and use that in the test by calling `PKIXRevocationChecker.setOcspResponses()`. For CRLs, you could also do something similar by caching the CRL and storing it in `CollectionCertStore` and adding that to `PKIXParameters`. Just some ideas to avoid having to continuously update the test certificates every 3 months. I can approve this now, but can you file a follow-on issue to look into this some more? - PR: https://git.openjdk.java.net/jdk/pull/4877
Re: RFR: 8270280: security/infra/java/security/cert/CertPathValidator/certification/LetsEncryptCA.java OCSP response error
On Fri, 23 Jul 2021 13:18:16 GMT, Sean Mullan wrote: > Have you thought about using a cached OCSPResponse to avoid the expiration > issues? You would not be testing a live OCSP network request/response, but it > might be an acceptable workaround for cases like this. For OCSP, it is possible to do backdated query and we do this when needed. The problem is some OCSP servers return UNAUTHORIZED error code after certificate expiry. We also need to update these certificates after expiry for CRL check. - PR: https://git.openjdk.java.net/jdk/pull/4877
Integrated: 8243543: jtreg test security/infra/java/security/cert/CertPathValidator/certification/BuypassCA.java fails
On Thu, 22 Jul 2021 18:07:42 GMT, Rajan Halade wrote: > Test certificates are updated for now. I am re-thinking the CA certification > testing approach to may be try a TLS connection with test websites. This will > ensure that test will pass as long as CA keeps test website updated. This pull request has now been integrated. Changeset: 45abbeed Author:Rajan Halade URL: https://git.openjdk.java.net/jdk/commit/45abbeed2f4f2899a3c1595b0cd8e573990a16fa Stats: 246 lines in 2 files changed: 49 ins; 7 del; 190 mod 8243543: jtreg test security/infra/java/security/cert/CertPathValidator/certification/BuypassCA.java fails Reviewed-by: mullan - PR: https://git.openjdk.java.net/jdk/pull/4878
Re: RFR: 8270280: security/infra/java/security/cert/CertPathValidator/certification/LetsEncryptCA.java OCSP response error
On Thu, 22 Jul 2021 17:29:32 GMT, Rajan Halade wrote: > I have updated revoked test certificate but this test may again fail in Sept > as test certificate expire leading to OCSP error. > > CA is not willing to issue test certificates with more than 90 day validity > so this test will fail every quarter. I am re-thinking the CA certification > testing approach to may be try a TLS connection with test websites. This will > ensure that test will pass as long as CA keeps test website updated. Have you thought about using a cached OCSPResponse to avoid the expiration issues? You would not be testing a live OCSP network request/response, but it might be an acceptable workaround for cases like this. - PR: https://git.openjdk.java.net/jdk/pull/4877
Re: RFR: 8243543: jtreg test security/infra/java/security/cert/CertPathValidator/certification/BuypassCA.java fails
On Thu, 22 Jul 2021 18:07:42 GMT, Rajan Halade wrote: > Test certificates are updated for now. I am re-thinking the CA certification > testing approach to may be try a TLS connection with test websites. This will > ensure that test will pass as long as CA keeps test website updated. Marked as reviewed by mullan (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4878
Re: JEP 411 Headaches: Instrumenting private methods in the JDK for authorization checkpoints.
Trusted code only, but it's intended to be run in a locked down principle of least privilege environment, while we perform static analysis and targeted review, we don't have the resources required to perform thorough trusted code audits, so have been reliant on the principle of least privilege. Hence the ability to generate policy files that we use for auditing. I utilize the constructor guarantee added in Java 6, that prevents finalizer attacks for atomic de-serialization, if invariants aren't satisfied, failure is atomic, thanks to this feature. We use the SM to ensure network connections used are secure and only have authenticated users, otherwise de-serialization and dynamic class loading is prevented. We don't need all the SM permission check features, but preventing unauthorized class loading is one of them. Network, file access, class loading, properties are a few that come to mind, I'm not worried about reflection, other recent changes are taking care of reflective access. We have a heap of our own Permission implementations as well. It appears that Java's future path is diverging from our software's needs and Java won't be a suitable platform in future. I wasn't aware that Java was heading in this direction. JEP 411 came as a big surprise, it just seemed like it was a core Java feature. I think the best course of action, will be to focus on Java 8, until that's EOL'd. I do like the improvements to TLS stateless connections and new features in later Java versions, but it looks like a mistake to try and keep up with Java, with this new knowledge. I think I'll back out some more recent changes that were intended to allow our software to run on Java 17, as these cause breakages for downstream developers and it doesn't seem to make much sense now, to make those breaking changes. I was actually looking forward to taking advantage of loom and the new vector API, but it looks like I need to be focused on what's going to be our next software platform. I've always found llvm to be interesting, so maybe I'll focus on some languages that run on that, and implement privileged constraints at a lower level. Perhaps by the time Java 8's EOL'd there will be a new platform and language that's more suitable. Thank you for your time, I do appreciate your replies and it gives me the clarity I needed. I wish it wasn't the case, but I have to accept that times are changing. Best of luck for the future my Java friends, it's been a powerful language, especially with regard to concurrency and scaling, we got great performance, all our hotspots were native methods and Java let us get close to the metal at low levels as well using bit shift operations on primitives, that are magnitudes faster than standard string operations. TLS ran so much faster with stateless session tickets too. Regards, Peter. On 23/07/2021 9:45 pm, Alan Bateman wrote: On 23/07/2021 11:48, Peter Firmstone wrote: Perhaps the solution is to replace the entire class, instead of instrumenting one method? Compile a patched copy of the JVM, with modified class files, then replace the existing classes in the JVM with the modified classes? Kinda like maintaining a fork, but using Agents to instrument the original JVM with classes from the fork? I sure wish there was a better option, if anyone knows one, I'm all ears. JEP 411 puts the JDK on the road to dropping support for sandboxing. This means there won't be a built-in means to securely run code that has malicious intent. It means that many of the concerns for finalizer attacks go away too. In the case of the ClassLoader example in your first mail then it may be that the private static method that you want to instrument will be removed. If removed, then it should make the instrumentation a bit easier so that you can instrument the protected constructors to invokestatic your equivalent of a permission check before the invokespecial. So I think this specific case is surmountable but in general I don't think it will be tenable to patch hundreds of classes and be confident that you've got everything, esp. with a moving code base and new features. I can't tell if your "authorization layer" is for use when running with code that has malicious intent. If it is, then I don't think it will be tenable to duplicate all the deeply invasive permission checks that exist today and keep it up to date as new features and changes. When agents were muted in the early discussion on JEP 411 then the context was file and network access where several people were interested in having a means to veto access. Expanding this to have a SM equivalent be able to veto every reflective access, prevent trusted method chain and other attacks, amounts to keeping the SM forever. As regards the comments about agents having the power to instrument methods that aren't accessible to user code then that is normal. Java agents are for tools to
Re: JEP 411 Headaches: Instrumenting private methods in the JDK for authorization checkpoints.
On 23/07/2021 11:48, Peter Firmstone wrote: Perhaps the solution is to replace the entire class, instead of instrumenting one method? Compile a patched copy of the JVM, with modified class files, then replace the existing classes in the JVM with the modified classes? Kinda like maintaining a fork, but using Agents to instrument the original JVM with classes from the fork? I sure wish there was a better option, if anyone knows one, I'm all ears. JEP 411 puts the JDK on the road to dropping support for sandboxing. This means there won't be a built-in means to securely run code that has malicious intent. It means that many of the concerns for finalizer attacks go away too. In the case of the ClassLoader example in your first mail then it may be that the private static method that you want to instrument will be removed. If removed, then it should make the instrumentation a bit easier so that you can instrument the protected constructors to invokestatic your equivalent of a permission check before the invokespecial. So I think this specific case is surmountable but in general I don't think it will be tenable to patch hundreds of classes and be confident that you've got everything, esp. with a moving code base and new features. I can't tell if your "authorization layer" is for use when running with code that has malicious intent. If it is, then I don't think it will be tenable to duplicate all the deeply invasive permission checks that exist today and keep it up to date as new features and changes. When agents were muted in the early discussion on JEP 411 then the context was file and network access where several people were interested in having a means to veto access. Expanding this to have a SM equivalent be able to veto every reflective access, prevent trusted method chain and other attacks, amounts to keeping the SM forever. As regards the comments about agents having the power to instrument methods that aren't accessible to user code then that is normal. Java agents are for tools to do powerful things, they aren't intended for libraries for applications to use directly. This is why agents are opt-in on the command line. Agent maintainers weld great power and must take care to never leak the Instrumentation object to applications or libraries. -Alan
RFR: 8271199: Mutual TLS handshake fails signing client certificate with custom sensitive PKCS11 key
Hello, Could you please review the small patch for the issue described in JDK-8271199: Mutual TLS handshake fails signing client certificate with custom sensitive PKCS11 key I suggest updating the RSAPSSSignature.isValid() method to verify if provided key components can be applied to SunRSASign implementation. If not applied, implementation can try to select signer from other providers Regards Alexey - Commit messages: - Fixed formatting - 8271199: Mutual TLS handshake fails signing client certificate with custom sensitive PKCS11 key Changes: https://git.openjdk.java.net/jdk/pull/4887/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4887=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8271199 Stats: 20 lines in 1 file changed: 20 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4887.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4887/head:pull/4887 PR: https://git.openjdk.java.net/jdk/pull/4887
Re: JEP 411 Headaches: Instrumenting private methods in the JDK for authorization checkpoints.
Perhaps the solution is to replace the entire class, instead of
instrumenting one method?
Compile a patched copy of the JVM, with modified class files, then
replace the existing classes in the JVM with the modified classes?
Kinda like maintaining a fork, but using Agents to instrument the
original JVM with classes from the fork?
I sure wish there was a better option, if anyone knows one, I'm all ears.
Regards,
Peter.
On 23/07/2021 6:36 pm, Peter Firmstone wrote:
Post JEP 411, we need to write Agents to replace the current
permission checks performed within the JVM by instrumenting it,
following advice by OpenJDK developers, however for us this goes
against all our previous development practices, no part of our
codebase accesses any JDK implementation classes or internal api's.
We also don't release anything that depends on deprecated API's.
Modules didn't break our code, neither has tightening rules around
access in JEP 403.
We have been advised that we need to instrument the JDK with Agent's
by OpenJDK.
I am now ready to write these Agents, so that I can begin testing my
new authorization layer for Java:
https://github.com/pfirmstone/HighPerformanceSecurity
As an example, we need to instrument java.lang.ClassLoader, in this
case we need to instrument the following method:
private static Void checkCreateClassLoader(String name); This check
must be performed prior to calling java.lang.Object's constructor, to
throw an exception, without making ClassLoader susceptible to a
finalizer attack.
Accessing private internal methods goes against all our current
development practices, these are at risk of breaking in future.
We cannot add methods with Agent's only change method contents.
I am requesting hooks, in the form of an annotation, such as the
following, so that OpenJDK developers know that this method will be
instrumented by Agent's and not to change it's signature.
@Instrumented
private static Void checkCreateClassLoader(String name);
If OpenJDK will provide instrumentation hooks, then this is a workable
solution for us.
Or is OpenJDK encouraging us to start accessing private methods and
have to test each Java release for breakages?
I'm wondering what the point of JEP 403 is if, our solution to JEP
411, is to start instrumenting private methods? I don't think this
is what OpenJDK developers are proposing.
Currently removing SM will allow an attacker to cause our software
running on the JVM to parse untrusted data, which previously required
an authenticated client? Permission is only granted to Principal's,
of course post JEP 411, these checks will stop working in future,
making our software vulnerable to attacks by unauthenticated users.
We're still up in the air about how to provide credentials for our TLS
and Kerberos connections, for authentication, I've created support for
obtaining the Subject from the Authorization context, in my
authorization layer software (linked above), but instrumenting private
methods in the JDK goes against all previously learned best practices.
--
Regards,
Peter Firmstone
Code snippet from java.lang.ClassLoader:
if (name != null && name.isEmpty()) {
throw new IllegalArgumentException("name must be non-empty or null");
}
@SuppressWarnings("removal")
SecurityManager security = System.getSecurityManager();
if (security != null) {
security.checkCreateClassLoader();
}
return null;
}
private ClassLoader(Void unused, String name, ClassLoader parent) {
this.name = name;
this.parent = parent;
this.unnamedModule = new Module(this);
if (ParallelLoaders.isRegistered(this.getClass())) {
parallelLockMap = new ConcurrentHashMap<>();
assertionLock = new Object();
} else {
// no finer-grained lock; lock on the classloader instance
parallelLockMap = null;
assertionLock = this;
}
this.package2certs = new ConcurrentHashMap<>();
this.nameAndId = nameAndId(this);
}
/**
* If the defining loader has a name explicitly set then
* '' @
* If the defining loader has no name then
* @
* If it's built-in loader then omit `@` as there is only one
instance.
*/
private static String nameAndId(ClassLoader ld) {
String nid = ld.getName() != null ? "\'" + ld.getName() + "\'"
: ld.getClass().getName();
if (!(ld instanceof BuiltinClassLoader)) {
String id = Integer.toHexString(System.identityHashCode(ld));
nid = nid + " @" + id;
}
return nid;
}
/**
* Creates a new class loader of the specified name and using the
* specified parent class loader for delegation.
*
* @apiNote If the parent is specified as {@codenull} (for the
* bootstrap class loader) then there is no guarantee that all platform
* classes are visible.
*
* @param name class loader name; or {@codenull} if not named
* @param parent the parent class loader
*
* @throws IllegalArgumentException if the given name is empty.
*
* @throws SecurityException
* If a security manager exists and its
* {@link
JEP 411 Headaches: Instrumenting private methods in the JDK for authorization checkpoints.
Post JEP 411, we need to write Agents to replace the current permission
checks performed within the JVM by instrumenting it, following advice by
OpenJDK developers, however for us this goes against all our previous
development practices, no part of our codebase accesses any JDK
implementation classes or internal api's. We also don't release anything
that depends on deprecated API's. Modules didn't break our code, neither
has tightening rules around access in JEP 403.
We have been advised that we need to instrument the JDK with Agent's by
OpenJDK.
I am now ready to write these Agents, so that I can begin testing my new
authorization layer for Java:
https://github.com/pfirmstone/HighPerformanceSecurity
As an example, we need to instrument java.lang.ClassLoader, in this case
we need to instrument the following method:
private static Void checkCreateClassLoader(String name); This check
must be performed prior to calling java.lang.Object's constructor, to
throw an exception, without making ClassLoader susceptible to a
finalizer attack.
Accessing private internal methods goes against all our current
development practices, these are at risk of breaking in future.
We cannot add methods with Agent's only change method contents.
I am requesting hooks, in the form of an annotation, such as the
following, so that OpenJDK developers know that this method will be
instrumented by Agent's and not to change it's signature.
@Instrumented
private static Void checkCreateClassLoader(String name);
If OpenJDK will provide instrumentation hooks, then this is a workable
solution for us.
Or is OpenJDK encouraging us to start accessing private methods and have
to test each Java release for breakages?
I'm wondering what the point of JEP 403 is if, our solution to JEP 411,
is to start instrumenting private methods? I don't think this is what
OpenJDK developers are proposing.
Currently removing SM will allow an attacker to cause our software
running on the JVM to parse untrusted data, which previously required an
authenticated client? Permission is only granted to Principal's, of
course post JEP 411, these checks will stop working in future, making
our software vulnerable to attacks by unauthenticated users.
We're still up in the air about how to provide credentials for our TLS
and Kerberos connections, for authentication, I've created support for
obtaining the Subject from the Authorization context, in my
authorization layer software (linked above), but instrumenting private
methods in the JDK goes against all previously learned best practices.
--
Regards,
Peter Firmstone
Code snippet from java.lang.ClassLoader:
if (name != null && name.isEmpty()) {
throw new IllegalArgumentException("name must be non-empty or null");
}
@SuppressWarnings("removal")
SecurityManager security = System.getSecurityManager();
if (security != null) {
security.checkCreateClassLoader();
}
return null;
}
private ClassLoader(Void unused, String name, ClassLoader parent) {
this.name = name;
this.parent = parent;
this.unnamedModule = new Module(this);
if (ParallelLoaders.isRegistered(this.getClass())) {
parallelLockMap = new ConcurrentHashMap<>();
assertionLock = new Object();
} else {
// no finer-grained lock; lock on the classloader instance
parallelLockMap = null;
assertionLock = this;
}
this.package2certs = new ConcurrentHashMap<>();
this.nameAndId = nameAndId(this);
}
/**
* If the defining loader has a name explicitly set then
* '' @
* If the defining loader has no name then
* @
* If it's built-in loader then omit `@` as there is only one instance.
*/
private static String nameAndId(ClassLoader ld) {
String nid = ld.getName() != null ? "\'" + ld.getName() + "\'"
: ld.getClass().getName();
if (!(ld instanceof BuiltinClassLoader)) {
String id = Integer.toHexString(System.identityHashCode(ld));
nid = nid + " @" + id;
}
return nid;
}
/**
* Creates a new class loader of the specified name and using the
* specified parent class loader for delegation.
*
* @apiNote If the parent is specified as {@codenull} (for the
* bootstrap class loader) then there is no guarantee that all platform
* classes are visible.
*
* @param name class loader name; or {@codenull} if not named
* @param parent the parent class loader
*
* @throws IllegalArgumentException if the given name is empty.
*
* @throws SecurityException
* If a security manager exists and its
* {@link SecurityManager#checkCreateClassLoader()}
* method doesn't allow creation of a new class loader.
*
* @since 9
*/
protected ClassLoader(String name, ClassLoader parent) {
this(checkCreateClassLoader(name), name, parent);
}
private static Void checkCreateClassLoader(String name) {
