x/vulndb: potential Go vuln in github.com/keep-network/keep-ecdsa: GHSA-gp6j-vx54-5pmf

[GoVulnBot]

In GitHub Security Advisory GHSA-gp6j-vx54-5pmf, there is a vulnerability in the following Go packages or modules:

Unit	Fixed	Vulnerable Ranges
github.com/keep-network/keep-ecdsa	1.8.1	< 1.8.1

See doc/triage.md for instructions on how to triage this report.

package: github.com/keep-network/keep-ecdsa
versions:
  - introduced: v0.0.0
    fixed: v1.8.1
description: |-
    # Summary

    In the threshold signature scheme, participants start by dividing secrets into shares using a secret sharing scheme. The Verifiable Secret Sharing scheme generates shares from the user’s IDs but does not properly validate them. Using a malicious ID will make other users reveal their secrets during the secret-sharing procedure. In addition, a second issue resulting from lack of validation could cause nodes to crash when sent maliciously formed user IDs.

    # Details

    The creation of Parties IDs does not properly validate for maliciously chosen IDs. Parties generate the secret shares in `binance-chain/tss-lib` codebase that `keep-network/keep-ecdsa` uses by evaluating the polynomial with the other parties’ IDs. It is critical that these party ids are non-zero because evaluating the polynomial at point 0 reveals the secret.

    There is a check that these ids are not zero, but this is insufficient since it is not performed modulo the curve order. Therefore, one can set their ID equal to the order of the elliptic curve, which equals 0 during the polynomial evaluation modulo the curve order.
    ```
        shares := make(Shares, num)
        for i := 0; i < num; i++ {
            if indexes[i].Cmp(big.NewInt(0)) == 0 {
                return nil, nil, fmt.Errorf("party index should not be 0")
            }
            share := evaluatePolynomial(ec, threshold, poly, indexes[i])
            shares[i] = &Share{Threshold: threshold, ID: indexes[i], Share: share}
        }
    ```
    (https://github.com/binance-chain/tss-lib/blob/73560daec7f83d7355107ea9b5e59d16de8765be/crypto/vss/feldman_vss.go#L64-L70)

    Thus, a party with an ID equal to the order of the curve will receive the secret key as its share.

    Another issue with the tss-lib implementation was the lack of verification for modularly equal user IDs that can cause nodes to crash during key generation or resharing.

    # Timeline

    ### 6 December 2021, 20:28 CET
    The team is informed by Trail of Bits about the issue in `binance-chain/tss-lib`. The vulnerability was already disclosed with Binance and they have implemented fixes in their recent commits. We are advised to update our dependency to the most recent `binance-chain/tss-lib` version.

    ### 7 December 2021, 9:10 CET
    The team confirms with Trail of Bits we started the work on a fix.

    ### 7 December 2021, 20:03 CET
    The team informs Trail of Bits that we found a potential problem in Binance's fix that may lead to the signing code failing with Go panic. We identified the problematic line and suggested its removal.

    To address the vulnerability in the protocol, it is required to ensure that:
    - all indexes are non-zero,
    - all indexes are non-zero modulo the curve order,
    - all indexes are unique modulo the curve order.

    The first two are guarded in `CheckIndexes` function by:
    ```
    vMod := new(big.Int).Mod(v, ec.Params().N)
    if vMod.Cmp(zero) == 0 {
      return nil, errors.New("party index should not be 0")
    }
    ```
    The last one is guarded by:
    ```
    if sortutil.Dedupe(sortutil.BigIntSlice(dup)) < len(indexes) {
      return nil, errors.New("duplicate in indexes")
    }
    ```
    However, `CheckIndexes` was additionally modified to update values of passed indexes by doing `indexes[i] = vMod`.

    This line was not backward-compatible and caused signatures to fail with `panic: runtime error: invalid memory address or nil pointer dereference` in case the signing group was generated with at least one `PartyID.Key` higher than `ec.Params().N`. This would also be problematic for new code that constructs such `PartyID` - we tested that key generation completed successfully but signing failed with the mentioned panic.

    ### 8 December 2021, 12:49 CET
    Trail of Bits confirmed the line we flagged is not critical to the security of the protocol.

    ### 8 December 2021, 15:29 CET
    The team informed Trail of Bits we are planning to open a PR to `binance-chain/tss-lib` with a fix and we suggest extending the embargo for communicating this issue for one more week, until Friday, Dec 17h to give everyone more time update their code given the problem was found. We also ask Trail of Bits to get in touch with Binance to review our fix given that we are not going to provide a sufficient explanation in the commit and PR description to do not threaten the security of projects that have already upgraded their dependency.

    ### 8 December 2021, 18:17 CET

    The team informs Trail of Bits about opening a PR https://github.com/binance-chain/tss-lib/pull/155 and explains all the details of the issue.

    The problematic scenario is:

    1. We start the keygen by creating `PartyID` for each member. The `PartyID` struct has `Id` and `Key` fields. We set `PartyID.Key` that is higher than the elliptic curve's `N`.
    2. This goes to tss-lib which starts the round 1 code (`ecdsa/keygen/round_1.go`). It takes `PartyID.Key` of each member and assembles the `ids` slice. This slice is thrown into `CheckIndexes` which overwrites each element by doing the modulo `N`.
    3. After returning from `CheckIndexes` the modified `ids` slice is used to set the `Ks` slice in the final `LocalPartySaveData` which is received by each group member as keygen final result.
    4. Each member takes this `LocalPartySaveData` and saves it on disk.
    5. Upon signing, we do the same as in point 1, so we construct `PartyID`s in the same way and start the protocol. At this point, `PartyID.Key` is higher than curve's `N`.
    6. Before starting round 1 of signing, `LocalPartySaveData` obtained at keygen and our `PartyID`s built at point 5 are used in `BuildLocalSaveDataSubset` to build a new `LocalPartySaveData` instance.
    7. `BuildLocalSaveDataSubset` is the place where things go wrong because `Key` field of each `PartyID` from point 5 doesn't correspond to encoded `Ks` elements of `LocalPartySaveData` obtained after keygen, the final `LocalPartySaveData` is corrupted, and cause panic upon validation before signing round 1.

    ### 8 December 2021, 21:50 CET

    Trail of Bits confirms they are going to followup with Binance and they agree to extend the embargo by a week.

    ### 8 December 2021, 15:16 CET

    Trail of Bits validates the problematic scenario we described and our fix. Trail of Bits reaches to Binance and recommends merging our fix and in addition to some other fixes that would secure the code to do not panic even if `LocalPartySaveData` has corrupted data.

    ### 10 December 2021, 8:16 CET

    Binance merges our fix https://github.com/binance-chain/tss-lib/commit/cd95cee01ea2af6d4aa8316612803be944d5369a

    ### 13 December 2021, 8:35 CET

    Binance applies more fixes so that even in case of `LocalPartySaveData` corrupted, the code is not going to panic.

    ### 15 December 2021

    The team releases a new version of the client, v1.8.1, and announces the need for an upgrade.
    https://github.com/keep-network/keep-ecdsa/releases/tag/v1.8.1
published: 2022-01-06T18:30:26Z
last_modified: 2022-01-06T18:30:26Z
ghsas:
  - GHSA-gp6j-vx54-5pmf

[neild]

Vulnerability in tool.

[gopherbot]

Change https://go.dev/cl/592767 mentions this issue: data/reports: unexclude 50 reports

[gopherbot]

Change https://go.dev/cl/607217 mentions this issue: data/reports: unexclude 20 reports (15)