RFR: 8311170: Simplify and modernize equals and hashCode in security area [v7]

Pavel Rappo prappo at openjdk.org
Fri Jul 7 23:19:27 UTC 2023


> 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;
>           boolean match;
>   
>           for (i = 0; this.certs != nu...

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 10 additional commits since the last revision:

 - Merge branch 'master' into 8311170
 - Reflow previously missed doc comment
 - Reflow doc comment as suggested
 - Revert for readability
 - 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

-------------

Changes:
  - all: https://git.openjdk.org/jdk/pull/14738/files
  - new: https://git.openjdk.org/jdk/pull/14738/files/054ae4b9..bb504a7a

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=14738&range=06
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14738&range=05-06

  Stats: 42868 lines in 93 files changed: 1587 ins; 40719 del; 562 mod
  Patch: https://git.openjdk.org/jdk/pull/14738.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/14738/head:pull/14738

PR: https://git.openjdk.org/jdk/pull/14738


More information about the security-dev mailing list