Bug 252894 - Fix public key derivation if WireGuard implementation
Summary: Fix public key derivation if WireGuard implementation
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Many People
Assignee: freebsd-bugs (Nobody)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-01-21 18:45 UTC by shamaz.mazum
Modified: 2021-02-09 13:01 UTC (History)
7 users (show)

See Also:


Attachments
This fixes key derivation (579 bytes, patch)
2021-01-21 18:45 UTC, shamaz.mazum
no flags Details | Diff
Alternate fix for wireguard key clamping (493 bytes, patch)
2021-01-27 07:59 UTC, Peter Grehan
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description shamaz.mazum 2021-01-21 18:45:10 UTC
Created attachment 221795 [details]
This fixes key derivation

The public key is incorrectly derived from the "unclamped" private key. This patch fixes it.
Comment 1 shamaz.mazum 2021-01-21 18:46:36 UTC
I started a thread on freebsd-net which describes the problem and leads to this solution.
Comment 2 Peter Grehan freebsd_committer 2021-01-27 07:59:01 UTC
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).
Comment 3 shamaz.mazum 2021-01-27 08:10:53 UTC
Peter, did you test your patch with any of VPN service providers?
Comment 4 Peter Grehan freebsd_committer 2021-01-27 08:14:53 UTC
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.
Comment 5 shamaz.mazum 2021-01-27 08:19:48 UTC
Uh, OK. If public keys derived by if_wg and by Mullvad match, then it should work. I think so.
Comment 6 Peter Grehan freebsd_committer 2021-01-27 08:22:16 UTC
I believe it will be ok.

Many thanks for diagnosing this issue.
Comment 7 Dave Cottlehuber freebsd_committer 2021-01-28 14:43:50 UTC
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!
Comment 8 commit-hook freebsd_committer 2021-02-03 11:06:37 UTC
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(+)
Comment 9 commit-hook freebsd_committer 2021-02-06 06:10:36 UTC
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(+)
Comment 10 Daniel Ebdrup Jensen freebsd_committer 2021-02-09 12:06:29 UTC
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?
Comment 11 Dave Cottlehuber freebsd_committer 2021-02-09 12:59:46 UTC
I think so.
Comment 12 Peter Grehan freebsd_committer 2021-02-09 13:01:44 UTC
Closing this.

(note: it's not in stable/12, just stable/13 and releng/13.0)