On Wed, 5 Jul 2023 14:52:22 GMT, Pavel Rappo <pra...@openjdk.org> wrote:
>> Please review this PR to use modern APIs and language features to simplify >> `equals` and `hashCode` in security area. >> >> I understand that security area is sensitive and a non-expert, such as >> myself, should tread carefully; so below are my notes to assist the review. >> >> * Unlike `hashCode`, non-secure `equals` implementations are typically >> short-circuit. But because of "timing attacks", we seem to have specialized >> implementations, such as `java.security.MessageDigest.isEqual(byte[], >> byte[])` and a more general `sun.security.util.ByteArrays.isEqual(byte[], >> int, int, byte[], int, int)`. So while reviewing this PR, take an >> opportunity to audit the affected `equals` implementations: perhaps some of >> them need to become secure, not modern. I have no domain knowledge to tell >> those cases apart, I only note that those cases exist. >> >> * This PR sacrifices compatibility for pragmatism: it changes some >> `hashCode` implementations to produce different values than before to allow >> more utilization of methods from `Objects` and `Arrays`. To my mind, those >> changes are **benign**. If you disagree, I'd be happy to discuss that and/or >> retract the concerning part of the change. >> >> * BitArray could be a topic of its own, but I'll do my best to be concise. >> >> * Truth to be told, BitArray's `equals` and `hashCode` are not used >> anywhere in source, and `equals` is only used in one test. For that reason, >> I refrained from reimplementing internals of `BitArray` using more general >> `java.util.BitSet`: too much effort and risk for almost nothing. >> * Speaking of `BitSet`-powered `BitArray`. Such an implementation is not >> for the faint of heart: there's too much impedance mismatch between data >> structures that those classes use to store bits. That said, for the sake of >> testing that it is possible and that I understand the `BitArray` correctly, >> I actually implemented it using `BitSet`. While that implementation is >> **NOT** part of this PR, you can have a look at it >> [here](https://cr.openjdk.org/~prappo/8311170/BitArray.java). >> >> * One suggestion to consider is to change this somewhat arcane piece in >> java.security.UnresolvedPermission.equals: >> >> // check certs >> if (this.certs == null && that.certs != null || >> this.certs != null && that.certs == null || >> this.certs != null && >> this.certs.length != that.certs.length) { >> return false; >> } >> >> int i,j; >> boolea... > > Pavel Rappo has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains six additional > commits since the last revision: > > - Merge branch 'master' into 8311170 > - Be consistent with the rest of the change > - Fix reported bugs > - Add even more cases and tidy up > - More cases > - Initial commit src/java.base/share/classes/java/security/cert/X509CRLEntry.java line 110: > 108: public int hashCode() { > 109: try { > 110: return Arrays.hashCode(this.getEncoded()); What data is at `entryData[0]` that will now be included in the hashCode when it was skipped before? src/java.base/share/classes/javax/security/cert/Certificate.java line 107: > 105: try { > 106: return Arrays.hashCode(this.getEncoded()); > 107: } catch (CertificateException e) { Ditto, what value at `certData[0]` is now included in the hash? src/java.base/share/classes/sun/security/util/BitArray.java line 72: > 70: * specified byte array. The most significant bit of {@code a[0]} gets > 71: * index zero in the BitArray. The array must be large enough to > specify > 72: * a value for every bit of the BitArray, i.e. {@code 8*a.length >= > length}. The original `<=` was correct, the number of bits in the input array must be less than the requested length of the BitArray. The constructors also describe the length using `<=`; they all should be consistent. src/java.base/share/classes/sun/security/x509/X500Name.java line 422: > 420: // quick check that number of RDNs and AVAs match before > canonicalizing > 421: if (!Arrays.equals(this.names, other.names, > 422: Comparator.comparingInt(n -> n.assertion.length))) I'd keep the original comparison of the lengths; its a lot less magical than `Comparator.comparingInt(n -> n.assertion.length))` ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/14738#discussion_r1253446806 PR Review Comment: https://git.openjdk.org/jdk/pull/14738#discussion_r1253456172 PR Review Comment: https://git.openjdk.org/jdk/pull/14738#discussion_r1253462853 PR Review Comment: https://git.openjdk.org/jdk/pull/14738#discussion_r1253469604
