Skip to content

pubkey: Assert CPubKey's ECCVerifyHandle precondition #17275

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
maflcko merged 1 commit into from
Dec 6, 2019
Merged

pubkey: Assert CPubKey's ECCVerifyHandle precondition #17275

maflcko merged 1 commit into from
Dec 6, 2019

Conversation

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Oct 27, 2019

Assert CPubKey's ECCVerifyHandle precondition.

This makes it more clear for fuzzing harness writers and others that ECCVerifyHandle is expected to be held when interacting with CPubKey.

Related PR #17274.

@fanquake fanquake requested a review from sipa October 27, 2019 23:59
@laanwj
Copy link
Member

laanwj commented Oct 28, 2019

Concept ACK, I think the "Users of this module must hold an ECCVerifyHandle." message is unclear, doesn't provide context.
what about a more direct "secp256k1_context_verify must be initialized to use CPubKey"

@laanwj laanwj added the Tests label Oct 28, 2019
@practicalswift practicalswift force-pushed the assert-CPubKey-preconditions branch from 7d74d51 to d8daa8f Compare October 28, 2019 15:10
@practicalswift
Copy link
Contributor Author

practicalswift commented Oct 28, 2019

@laanwj

I've now changed to the suggested wording.

The old wording was taken from here:

bitcoin/src/pubkey.h

Line 227 in 7730260

/** Users of this module must hold an ECCVerifyHandle. The constructor and

Please re-review :)

@practicalswift
Copy link
Contributor Author

practicalswift commented Oct 29, 2019

@fanquake No longer waiting for author? :)

@practicalswift
Copy link
Contributor Author

practicalswift commented Nov 14, 2019

@laanwj Would you mind reviewing? :)

@practicalswift
Copy link
Contributor Author

practicalswift commented Dec 6, 2019 •
edited
Loading

I think it would be good to have these assertions in.

Without these it is possible for someone to mess up and have a NULL secp256k1_context_verify passed to the secp256k1-functions (by forgetting the ECCVerifyHandle pre-conditiion).

Consider the following example:

bool CPubKey::IsFullyValid() const {
    if (!IsValid())
        return false;
    secp256k1_pubkey pubkey;
    return secp256k1_ec_pubkey_parse(secp256k1_context_verify, &pubkey, vch, size());
}

That function will return also in cases where secp256k1_context_verify == nullptr.

I think it would be good to guard against those sharp edges by adding the suggested assertions :)

In this case:

bool CPubKey::IsFullyValid() const {
    if (!IsValid())
        return false;
    secp256k1_pubkey pubkey;
    assert(secp256k1_context_verify && "secp256k1_context_verify must be initialized to use CPubKey.");
    return secp256k1_ec_pubkey_parse(secp256k1_context_verify, &pubkey, vch, size());
}

sipa
sipa approved these changes Dec 6, 2019
View reviewed changes
Copy link
Member

@sipa sipa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK d8daa8f

maflcko pushed a commit that referenced this pull request Dec 6, 2019
Merge #17275: pubkey: Assert CPubKey's ECCVerifyHandle precondition
da1af85 
d8daa8f pubkey: Assert CPubKey's ECCVerifyHandle precondition (practicalswift)

Pull request description:

  Assert `CPubKey`'s `ECCVerifyHandle` precondition.

  This makes it more clear for fuzzing harness writers and others that `ECCVerifyHandle` is expected to be held when interacting with `CPubKey`.

  Related PR #17274.

ACKs for top commit:
  sipa:
    ACK d8daa8f

Tree-SHA512: 9e74086599799dc9b5c3fb8357445b662e5bf896d826af63d6d6b6ddb616612966f3bb5de3bd3ae0e692c47de85672f64b8ab6d3a1c45899dc25ba46990b5ec7
@maflcko maflcko merged commit d8daa8f into bitcoin:master Dec 6, 2019
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 6, 2019
Merge bitcoin#17275: pubkey: Assert CPubKey's ECCVerifyHandle precond…
ac4deea 
…ition

d8daa8f pubkey: Assert CPubKey's ECCVerifyHandle precondition (practicalswift)

Pull request description:

  Assert `CPubKey`'s `ECCVerifyHandle` precondition.

  This makes it more clear for fuzzing harness writers and others that `ECCVerifyHandle` is expected to be held when interacting with `CPubKey`.

  Related PR bitcoin#17274.

ACKs for top commit:
  sipa:
    ACK d8daa8f

Tree-SHA512: 9e74086599799dc9b5c3fb8357445b662e5bf896d826af63d6d6b6ddb616612966f3bb5de3bd3ae0e692c47de85672f64b8ab6d3a1c45899dc25ba46990b5ec7
@MarkLTZ MarkLTZ mentioned this pull request Apr 4, 2020
Closed
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Nov 6, 2020
@practicalswift @PiRK
pubkey: Assert CPubKey's ECCVerifyHandle precondition
0948e1d 
Summary:
> This makes it more clear for fuzzing harness writers and others that ECCVerifyHandle is expected to be held when interacting with CPubKey.
>
> Related [[bitcoin/bitcoin#17274 | PR17274]] (D6883).

This is a backport of Core [[bitcoin/bitcoin#17275 | PR17275]]

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D8301
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
Merge bitcoin#17275: pubkey: Assert CPubKey's ECCVerifyHandle precond…
ff5075e 
…ition

d8daa8f pubkey: Assert CPubKey's ECCVerifyHandle precondition (practicalswift)

Pull request description:

  Assert `CPubKey`'s `ECCVerifyHandle` precondition.

  This makes it more clear for fuzzing harness writers and others that `ECCVerifyHandle` is expected to be held when interacting with `CPubKey`.

  Related PR bitcoin#17274.

ACKs for top commit:
  sipa:
    ACK d8daa8f

Tree-SHA512: 9e74086599799dc9b5c3fb8357445b662e5bf896d826af63d6d6b6ddb616612966f3bb5de3bd3ae0e692c47de85672f64b8ab6d3a1c45899dc25ba46990b5ec7
@practicalswift practicalswift deleted the assert-CPubKey-preconditions branch April 10, 2021 19:39
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
@PastaPastaPasta
Merge bitcoin#17275: pubkey: Assert CPubKey's ECCVerifyHandle precond…
4b12666 
…ition

d8daa8f pubkey: Assert CPubKey's ECCVerifyHandle precondition (practicalswift)

Pull request description:

  Assert `CPubKey`'s `ECCVerifyHandle` precondition.

  This makes it more clear for fuzzing harness writers and others that `ECCVerifyHandle` is expected to be held when interacting with `CPubKey`.

  Related PR bitcoin#17274.

ACKs for top commit:
  sipa:
    ACK d8daa8f

Tree-SHA512: 9e74086599799dc9b5c3fb8357445b662e5bf896d826af63d6d6b6ddb616612966f3bb5de3bd3ae0e692c47de85672f64b8ab6d3a1c45899dc25ba46990b5ec7
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 15, 2021
@PastaPastaPasta
Merge bitcoin#17275: pubkey: Assert CPubKey's ECCVerifyHandle precond…
65bac01 
…ition

d8daa8f pubkey: Assert CPubKey's ECCVerifyHandle precondition (practicalswift)

Pull request description:

  Assert `CPubKey`'s `ECCVerifyHandle` precondition.

  This makes it more clear for fuzzing harness writers and others that `ECCVerifyHandle` is expected to be held when interacting with `CPubKey`.

  Related PR bitcoin#17274.

ACKs for top commit:
  sipa:
    ACK d8daa8f

Tree-SHA512: 9e74086599799dc9b5c3fb8357445b662e5bf896d826af63d6d6b6ddb616612966f3bb5de3bd3ae0e692c47de85672f64b8ab6d3a1c45899dc25ba46990b5ec7
@random-zebra random-zebra mentioned this pull request Sep 9, 2021
Merged
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sipa sipa sipa approved these changes

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@practicalswift @laanwj @sipa @fanquake @maflcko