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

fix soundness issue with preallocated_gen_new #548

Merged

Conversation

apoelstra
Copy link
Member

@apoelstra apoelstra commented Nov 30, 2022

Stop this from being a generic function over all contexts, to only a function generic over contexts where we can bound the lifetime precisely. Introduces a new unsafe trait. I believe the only code this breaks was already unsound:

  • code that tried to use one of the *Preallocated context markers with an incorrect lifetime
  • code that tried to use preallocated_gen_new with a non-*Preallocated marker, which I believe we allowed before (I just noticed this now) and almost certainly would've led to UB on drop

Fixes #543

@apoelstra
Copy link
Member Author

I'm not thrilled with this solution but it's a start so we can at least discuss what a better one might look like.

Copy link
Member

@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 tested this with the code from @Kixunil, and with this applied we get

error[E0597]: `*array` does not live long enough
 --> src/main.rs:6:87
  |
6 |     secp256k1::Secp256k1::<secp256k1::AllPreallocated<'static>>::preallocated_gen_new(&mut *array).unwrap()
  |                                                                                       ^^^^^^^^^^^
  |                                                                                       |
  |                                                                                       borrowed value does not live long enough
  |                                                                                       cast requires that `*array` is borrowed for `'static`
7 | }
  | - `*array` dropped here while still borrowed

Which is exactly what we were after, right?

src/context.rs Outdated
@@ -318,9 +318,9 @@ unsafe impl<'buf> Context for AllPreallocated<'buf> {
}
}

impl<'buf, C: Context + 'buf> Secp256k1<C> {
impl<'buf> PreallocatedContext<'buf> for Secp256k1<AllPreallocated<'buf>> {
Copy link
Member

@tcharding tcharding Dec 1, 2022

Choose a reason for hiding this comment

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

Two comments; we can macroize this and also the trait needs to be public.

macro_rules! impl_preallocated_gen_new {
	($ty:ty) => {
		/// Lets you create a context with a preallocated buffer in a generic manner (sign/verify/all).
		fn preallocated_gen_new(buf: &'buf mut [AlignedType]) -> Result<Self, Error> {
			#[cfg(target_arch = "wasm32")]
			ffi::types::sanity_checks_for_wasm();

			if buf.len() < Self::preallocate_size_gen() {
				return Err(Error::NotEnoughMemory);
			}
			Ok(Secp256k1 {
				ctx: unsafe {
					NonNull::new_unchecked(ffi::secp256k1_context_preallocated_create(
						buf.as_mut_c_ptr() as *mut c_void,
						<$ty>::FLAGS,
					))
				},
				phantom: PhantomData,
			})
		}
	}
}
impl<'buf> PreallocatedContext<'buf> for Secp256k1<AllPreallocated<'buf>> {
	impl_preallocated_gen_new!(AllPreallocated<'buf>);
}
impl<'buf> PreallocatedContext<'buf> for Secp256k1<VerifyOnlyPreallocated<'buf>> {
	impl_preallocated_gen_new!(VerifyOnlyPreallocated<'buf>);
}
impl<'buf> PreallocatedContext<'buf> for Secp256k1<SignOnlyPreallocated<'buf>> {
	impl_preallocated_gen_new!(SignOnlyPreallocated<'buf>);
}

And, on a lighter note, this PR shows who round here is the C ludite that puts definitions above where they are used. I say that with much endearment and kindness as one often called a ludite to another :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

100% agree. You could also macroize the impl... line :)

Also notably we could keep inherent method just forwarding to trait. This would keep the currently-sound client code working after upgrade and avoid annoying imports.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I also realized none of this duplication/macro stuff would be needed if PreallocatedContext was an unsafe marker trait. Any specific reason you didn't choose it @apoelstra ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I figured, making stuff unsafe that didn't use to be would be a more annoying change for users than doing something ugly. I don't feel strongly about this.

We could actually leave the code alone except to mark the existing preallocated_gen_new funciton unsafe. And/or we could just make it private, and insist users go through the more specific Secp256k1::<AllPreallocated<'a>>::preallocated_new etc methods.

Copy link
Collaborator

Choose a reason for hiding this comment

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

unsafe trait is `unsafe to implement but safe to use. The API would be almost identical to the current, it'd just disallow unsound usage. So if someone used it soundly, without any generic crazyness it would work without change. Guaranteed UB would be guaranteed to fail to compile. Funny generics would have to be adjusted slightly but work as before.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah! I'm sorry, I misunderstood what you were suggesting.

Now I understand you to be suggesting: (1) add a new unsafe marker trait PreallocatedContext<'a>; (2) impl this for the three preallocated context markes e.g. AllPreallocated<'a>; (3) change the bound on preallocated_gen_new from C: Context + 'buf to C: Context + PreallocatedContext<'buf>.

I believe this would work. I'll try it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Exactly.

Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

At minimum the trait really needs to be pub.

Also are we going to backport this? (I vote yes and also yank old versions.) This is API-breaking but I suggest we adopt Rust policy of allowing API breakages for soundness fixes.

I also plan to file RustSec entry once the fix is released.

src/context.rs Outdated
@@ -318,9 +318,9 @@ unsafe impl<'buf> Context for AllPreallocated<'buf> {
}
}

impl<'buf, C: Context + 'buf> Secp256k1<C> {
impl<'buf> PreallocatedContext<'buf> for Secp256k1<AllPreallocated<'buf>> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

100% agree. You could also macroize the impl... line :)

Also notably we could keep inherent method just forwarding to trait. This would keep the currently-sound client code working after upgrade and avoid annoying imports.

@apoelstra
Copy link
Member Author

Also are we going to backport this? (I vote yes and also yank old versions.) This is API-breaking but I suggest we adopt Rust policy of allowing API breakages for soundness fixes.

I don't feel strongly about it. I think it's obscure enough and "only if you use the method in a clearly wrong way" enough that it's not a big deal. But I get your argument and I won't say no if you want to backport-and-yank.

Especially if we decide to fix it just by tacking unsafe and some extra docs on the generic preallocated_gen_new method.

@Kixunil
Copy link
Collaborator

Kixunil commented Dec 1, 2022

I feel like if I had a satoshi for every "use in a clearly wrong way" I'd cause another overflow bug...

So yes please, especially if we just fix the bound using unsafe trait. :)

@apoelstra
Copy link
Member Author

Ok, pushed to use @Kixunil's unsafe solution. Much much better!

Fixes unsoundness in `preallocated_gen_new` which previously did not
properly constrain the lifetime of the buffer used to back the context
object. We introduce an unsafe marker trait, and impl it for our
existing preallocated-context markers.

Annoyingly the trait has to be public even though it should never be
used directly, and is only used alongside the sealed `Context` trait,
so it is de-facto sealed itself.

Fixes rust-bitcoin#543
@apoelstra
Copy link
Member Author

lol, we should move the fmt check out of the ASAN CI job ... it freaks me out every time that one fails

Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 1e6eb6c

I also tested that my example doesn't compile.

We might make the trait sealed but I'm not sure if it'd interfere with other crates extending secp256k1 (e.g. zkp fork).

Copy link
Member

@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.

ACK 1e6eb6c

@Kixunil
Copy link
Collaborator

Kixunil commented Dec 3, 2022

FYI I have RustSec report prepared assuming we will backport the fix to 0.24.2. Feel free to review it including complaining about grammar.

@Kixunil
Copy link
Collaborator

Kixunil commented Dec 3, 2022

Also new piece of information: yanked crates don't show on docs.rs which makes inspection extra hard (e.g. yanked version 0.13 is the last one that didn't have the affected method). Therefore I'm now against yanking all affected versions of the crate. I think it could be good to yank 0.24.1 as a simple way to signal the issue to users not using cargo audit (cargo warns when you're trying to build a yanked crate) but I wouldn't go beyond that.

@Kixunil
Copy link
Collaborator

Kixunil commented Dec 3, 2022

code that tried to use preallocated_gen_new with a non-*Preallocated marker, which I believe we allowed before (I just noticed this now) and almost certainly would've led to UB on drop

Oh crap, only noticed it now. Good point! I tested it and it is the case. So I now have to update the report. 😄
Edit: report updated.

@apoelstra
Copy link
Member Author

@Kixunil thanks, your rustsec advisory looks great!

Agreed about yanking 0.24.1 as a signal. I think we should also backport to 0.23 and 0.22, using the heuristic that rust-bitcoin 0.28 uses 0.22. So then users of the most recent and second-most recent rust-bitcoin would get a signal. (rust-bitcoin doesn't call preallocated_gen_new anywhere of course, but I'm using it as a prototypical rust-secp-dependent project)

@apoelstra apoelstra merged commit 29c1363 into rust-bitcoin:master Dec 4, 2022
@apoelstra apoelstra deleted the 2022-11--prealloc-soundness branch December 4, 2022 17:46
@Kixunil
Copy link
Collaborator

Kixunil commented Dec 4, 2022

Oh, good point, agree!

apoelstra added a commit that referenced this pull request Dec 7, 2022
12e67f1 Bump version to 0.22.2 (Tobin C. Harding)
7604f32 Add saftey docs for PreallocatedContext trait (Tobin C. Harding)
357e800 context: introduce unsafe `PreallocatedContext` trait (Andrew Poelstra)

Pull request description:

  Backport #548 and bump version ready for release.

ACKs for top commit:
  Kixunil:
    ACK 12e67f1

Tree-SHA512: 6e2e700ecede75b00a6629e314f814946d4a929e5a5b1540b663cf5b338dae4fa7eda293dd6877888976892c834eb63a53ce29938d451e6062ed3d6db3ad6429
apoelstra added a commit that referenced this pull request Dec 7, 2022
70e8cbe Bump version to 0.23.5 (Tobin C. Harding)
989bf05 Add saftey docs for PreallocatedContext trait (Tobin C. Harding)
2b9a5b6 context: introduce unsafe `PreallocatedContext` trait (Andrew Poelstra)
f2ba29f Remove deref on an immutable reference (Tobin C. Harding)

Pull request description:

  Backport #548 and bump version ready for release.

  ### Note

  Patch one fixes a trivial clippy issue so we lint cleanly on every patch.

ACKs for top commit:
  Kixunil:
    ACK 70e8cbe

Tree-SHA512: 3551b798e89724ab06cfcc3be71689cd96f514faab2bbf2791ecd90fe5246321becb4aa141c95c9f086a190fd19197c5470f2ff765ef74a61d52ed6e3899cb02
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.

preallocated_gen_new is unsound
3 participants