Skip to content
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.

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

Add soundness bug in secp256k1 API #1480

Merged
merged 2 commits into from
Dec 7, 2022

Conversation

Kixunil
Copy link
Contributor

@Kixunil Kixunil commented Dec 7, 2022

Summary: Unsound API in secp256k1 allows use-after-free and invalid deallocation from safe code. This was fixed and backported to multiple versions.

I'm not sure whether the date field should have todays date, will change if it needs something else.

Cc @apoelstra @tcharding

HCastano added a commit to use-ink/cargo-contract that referenced this pull request Dec 7, 2022
The `v0.24.1` release has been yanked due to a soundness bug. See here
for more: rustsec/advisory-db#1480
HCastano added a commit to use-ink/ink that referenced this pull request Dec 7, 2022
Versions `>= 0.24.0` have a soundness bug, so we need to ensure people
are using `>= 0.24.2`

See here for more: rustsec/advisory-db#1480
HCastano added a commit to use-ink/cargo-contract that referenced this pull request Dec 7, 2022
The `v0.24.1` release has been yanked due to a soundness bug. See here
for more: rustsec/advisory-db#1480
HCastano added a commit to use-ink/ink that referenced this pull request Dec 7, 2022
Versions `>= 0.24.0` have a soundness bug, so we need to ensure people
are using `>= 0.24.2`

See here for more: rustsec/advisory-db#1480
Copy link
Contributor

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

I have zero experience with filing RUSTSEC advisories but FWIW I checked all links are valid and read over for grammar and correctness.

crates/secp256k1/RUSTSEC-0000-0000.md Outdated Show resolved Hide resolved
Summary: Unsound API in `secp256k1` allows use-after-free and invalid
deallocation from safe code. This was fixed and backported to multiple
versions.
@Kixunil Kixunil force-pushed the secp256k1_preallocated_gen_new branch from fd6920c to 0e0f303 Compare December 7, 2022 21:54
@Shnatsel
Copy link
Member

Shnatsel commented Dec 7, 2022

Looks good to me, thank you!

Is this easy to trigger unintentionally? If so, we might want to drop informational = "unsound" which would promote it from a warning to an error. What do you think?

@Kixunil
Copy link
Contributor Author

Kixunil commented Dec 7, 2022

@Shnatsel hard to say, it's possible that the specific function isn't even used at all in any outside code. Neither github search nor grep.app show anything interesting so if it's used anywhere it's probably closed source.

However it kinda bypasses borrow checking so the consequences are probably quite bad. If cargo-audit can already filter based on function and does not error if the function is not called, I'd keep unsound. If it unconditionally errors for everyone I'd drop it.

@apoelstra
Copy link

Is this easy to trigger unintentionally?

I don't think so -- you need to be using a special no-alloc version of our API (which already excludes the vast majority of users), and then you need to pass it a backing store with a finite lifetime (which would be a little weird for a backing store), and then you'd need to use the object from our API beyond the backing store's lifetime (which would likely be caught in code review -- unless the reviewer was working from a "Rust will protect us from such things" mental model -- and if not, would likely reliably cause a double-free crash).

@Kixunil
Copy link
Contributor Author

Kixunil commented Dec 7, 2022

unless the reviewer was working from a "Rust will protect us from such things" mental model

I think not wasting brain power on reviewing those things is part of the point of Rust so my guess is most people do. (Including myself; and you know how crazy my reviews are... ;))

@tarcieri
Copy link
Member

tarcieri commented Dec 7, 2022

Based on @apoelstra's comment this sounds like informational = "unsound" to me

@Shnatsel
Copy link
Member

Shnatsel commented Dec 7, 2022

Yep, sounds good. Thanks for the explanation. Merging!

@Shnatsel Shnatsel merged commit 3be728d into rustsec:main Dec 7, 2022
@Kixunil Kixunil deleted the secp256k1_preallocated_gen_new branch December 8, 2022 07:19
HCastano added a commit to use-ink/ink that referenced this pull request Jan 23, 2023
Versions `>= 0.24.0` have a soundness bug, so we need to ensure people
are using `>= 0.24.2`

See here for more: rustsec/advisory-db#1480
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants