On Fri Dec 8 11:28:16 2023 +0000, Paul Gofman wrote:
I've put a couple of specific suggestions inside the patches. Not related to this specific patchset (rather the already merged implementation), one other thing I came across is that BCryptSecretAgreement is not performing validation (unlike current Proton patches) because actual secret agreement calculation is performed during final secret derivation BCryptDeriveKey(). I am attaching a small addition to test which shows that it actually performed on Windows. The reason for failing validation in this test is that the first key doesn't have a private key in it. There are probably more possible reasons for failures there, like possibly not matching DH parameters between the keys (although I am not sure now if I explicitly tested that case). I understand why it was done the way its done now, that saves some mess around saving the computed secret, checking the state in BCryptDeriveKey(). I don't know if anything depends on that (properly failing BCryptSecretAgreement on bad keys), but I if we want to be accurate here we'd need to compute the agreement in BCryptSecretAgreement(). I checked that all the other tests I have (mostly integrated in this patchset) pass with gnutls 3.8.2 and this patchset plus attached second diff. While I expect the tests to sufficiently cover what games known to use DH need, I picked this to current Proton (with the additional diff) and checked Red Dead Online just in case, that works. The game sets DH parameters before finalizing the key, it is probably unavoidable as at least one peer should do that so the peer keys have matching parameters and the same secret can be established. [test.patch](/uploads/a88dff20bd2587a12aef12d355ea957e/test.patch)
I decided to leave validation in BCryptSecretAgreement() for another day.