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

[Pavel Rappo]

On Fri, 7 Jul 2023 23:19:27 GMT, Pavel Rappo <prappo at 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 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

> > > Took another pass at this, looks good, but I would like to take another last look and make sure that changing the hash code for some of the classes like X509CRL is a benign change.
> > 
> > 
> > Thanks, Sean. Take your time, you're an expert in this area. Meanwhile, I'll reflow other similar `{@return ... }` constructs that I missed before, for readability.
> 
> This comment is NOT to rush the review.
> 
> I once again note, that those classes in this PR that skip the first array element in `hashCode`, do not seem to skip that same element in `equals`. Although that does not breach equals-hashCode contract, it puzzles the reader and theoretically makes `hashCode` more blunt (i.e. more instances may share a hash code value).
> 
> This observation is an investigation opportunity. But equally, I'm okay with skipping the investigation and reverting those particular changes, if security-dev wants to be on the safe side. We might soon have a more flexible way to compute hashCode [1](#user-content-fn-*-d08640fefec8cc5b91d743888afc0a5d). If we have it, we could then both refactor those hashCode implementations and preserve their behaviour.
> 
> ## Footnotes
> 1. https://github.com/openjdk/jdk/pull/14831 [↩](#user-content-fnref-*-d08640fefec8cc5b91d743888afc0a5d)

To add to my previous comment. Don't know if it helps, but I found this bug, that applies similar (to my non-expert eye) refactoring to java.security.cert.Certificate.hashCode: 

8011402: Move blacklisting certificate logic from hard code to data

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

PR Comment: https://git.openjdk.org/jdk/pull/14738#issuecomment-1631563886