Created attachment 221795 [details] This fixes key derivation The public key is incorrectly derived from the "unclamped" private key. This patch fixes it.
I started a thread on freebsd-net which describes the problem and leads to this solution.
Created attachment 221957 [details] Alternate fix for wireguard key clamping I had some communication with Jason Donenfeld about this issue, and he suggested the curve22519 crypto routine should be clamping the key (the OpenBSD version does this).
Peter, did you test your patch with any of VPN service providers?
I didn't test traffic against service providers, but I verified the generated public key was correct using the private key from your test case posted to freebsd-net. The change is functionally identical to the one you posted: just broader in scope, and fixes a divergence in curve22519 in the FreeBSD implementation.
Uh, OK. If public keys derived by if_wg and by Mullvad match, then it should work. I think so.
I believe it will be ok. Many thanks for diagnosing this issue.
LGTM. Peter are you able to commit this, or do we need to ask somebody else to do so? I would love to have this in 13.0-RELEASE so that the keys we generate match other implementations!
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=5aaea4b99e5cc724e97e24a68876e8768d3d8012 commit 5aaea4b99e5cc724e97e24a68876e8768d3d8012 Author: Peter Grehan <grehan@FreeBSD.org> AuthorDate: 2021-02-03 09:05:09 +0000 Commit: Peter Grehan <grehan@FreeBSD.org> CommitDate: 2021-02-03 09:05:09 +0000 Always clamp curve25519 keys prior to use. This fixes an issue where a private key contained bits that should have been cleared by the clamping process, but were passed through to the scalar multiplication routine and resulted in an invalid public key. Issue diagnosed (and an initial fix proposed) by shamaz.mazum in PR 252894. This fix suggested by Jason Donenfeld. PR: 252894 Reported by: shamaz.mazum Reviewed by: dch MFC after: 3 days sys/dev/if_wg/module/curve25519.c | 1 + 1 file changed, 1 insertion(+)
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=6136a10e355a7a837edecbccbed04c34b4bc32c9 commit 6136a10e355a7a837edecbccbed04c34b4bc32c9 Author: Peter Grehan <grehan@FreeBSD.org> AuthorDate: 2021-02-03 09:05:09 +0000 Commit: Peter Grehan <grehan@FreeBSD.org> CommitDate: 2021-02-06 04:01:18 +0000 Always clamp curve25519 keys prior to use. This fixes an issue where a private key contained bits that should have been cleared by the clamping process, but were passed through to the scalar multiplication routine and resulted in an invalid public key. Issue diagnosed (and an initial fix proposed) by shamaz.mazum in PR 252894. This fix suggested by Jason Donenfeld. PR: 252894 Reported by: shamaz.mazum (cherry picked from commit 5aaea4b99e5cc724e97e24a68876e8768d3d8012) sys/dev/if_wg/module/curve25519.c | 1 + 1 file changed, 1 insertion(+)
This has landed in releng/13.0 as of https://cgit.freebsd.org/src/commit/?h=releng/13.0&id=82874dcb3610b1e57fb6b1b9db96ac4996bfa620 and stable/12 as of https://cgit.freebsd.org/src/commit/?id=82874dcb3610b1e57fb6b1b9db96ac4996bfa620&h=stable%2F12, so is it okay to close this?
I think so.
Closing this. (note: it's not in stable/12, just stable/13 and releng/13.0)