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

[Pavel Rappo]

On Thu, 13 Jul 2023 08:51:23 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 12 additional commits since the last revision:
> 
>  - Much more cases: apologies for extra review work
>  - Merge branch 'master' into 8311170
>  - 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
>  - ... and 2 more: https://git.openjdk.org/jdk/compare/2e25106e...457b9c56

> I pushed a new commit, [457b9c5](https://github.com/openjdk/jdk/commit/457b9c56a937b98d64201ce378f6960f769dcf65)
> 
> FWIW, the tests are green.

Our CI disagrees with me: testing at 457b9c56a93 revealed three failures. Apologies for misinforming my reviewers.

At 3caa774d007 all three failures are fixed, more previously missed use cases are addressed, [feedback](https://github.com/openjdk/jdk/pull/14738#discussion_r1262696059) from @RogerRiggs is incorporated, and the tests are run. This time, the tests are completely green, not just mostly green; I double-checked that.

A few comments on new commits.

* 6d339fce adds a must-have null-check to equals here. https://github.com/openjdk/jdk/blob/3caa774d007d6a133fd1238f3c4459b1a15d6050/src/java.base/share/classes/com/sun/crypto/provider/PBKDF2KeyImpl.java#L167 I believe there's no compatibility risk.

* f83c1578f13 fixes a peculiar failure related to (JFR?) security events.

  PKIXCertPathValidator generates an event that TestX509ValidationLog checks. While the actual and the expected formats have mismatched for some time, the bug reveals itself only now because the new hashCode implementation returns a negative int value, which is not expected.

  I don't know how big can an X.509 key be, but in TestX509ValidationLog it's 294 bytes. That corresponds to a maximum possible old hash code value of 1,392,678, which is a positive int. Now, if my figures are correct, it would require a key of a minimum size of 453,343 (443 kB) for the old hashCode implementation to overflow and return a negative int. And even then, the overflow will only happen if the key consists of all `0xff` bytes, which probably is a bad key. So, in reality, the key would need to be bigger than that for the old hash code to overflow. Hence, the failure hasn't been previously observed.

  With the new hashCode implementation, it takes as few as 1 byte to overflow. Because I think that it's unreasonable to require hashCode to not be negative, I fix the test, not code.

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

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