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

illegal-callback called with message that says "secp256k1_ecmult_context_is_built(&ctx->ecmult_ctx)" #573

Closed
jimhansson opened this issue Nov 12, 2018 · 10 comments · Fixed by #587

Comments

@jimhansson
Copy link

trying to wrap secp256k1 in lisp and I am getting this message when trying to derive a public child key of a public parent key.
my code looks something like this(I have used cl-autowrap, have not given everthing nice names yet)

(with-context (ctx secp256k1.ffi:+secp256k1-context-sign+)
    (secp256k1.ffi:secp256k1-context-set-illegal-callback ctx
                                                          (autowrap:callback 'illegal-callback)
                                                          'nil)
    (autowrap:with-many-alloc ((pubkey 'secp256k1.ffi:secp256k1-pubkey)
                               (serialized ':unsigned-char 65)
                               (size-serialized ':unsigned-long 1))
      (unless (secp256k1.ffi:secp256k1-ec-pubkey-parse ctx pubkey key_ (length key))
        (error "when parsing pubkey"))
      (unless (secp256k1.ffi:secp256k1-ec-pubkey-tweak-add ctx pubkey hash_)
        (error "error when tweaking pubkey"))
      (setf (autowrap:c-aref size-serialized 0 :unsigned-long) 65)
      (unless (secp256k1.ffi:secp256k1-ec-pubkey-serialize ctx
                                                           serialized
                                                           size-serialized
                                                           pubkey
                                                           secp256k1.ffi:+secp256k1-ec-compressed+)
        (error "could not serialize new public key"))
      (values (cffi:foreign-array-to-lisp serialized `(:array :unsigned-char
                                                              ,(autowrap:c-aref size-serialized
                                                                                0
                                                                                :unsigned-long)))
              (subseq hash 32 64))))

i get the call to my illegal-callback after the call to tweak-add function and before the call to the ec-pubkey-serialize.

Questions:
what does this message mean? do i need to configure the context in some specific way? do I need to have a scratch-space set up? i have tried with one.

I am running my lisp code in SBCL.

@apoelstra
Copy link
Contributor

In C you would need to call secp256k1_context_create with the SECP256K1_CONTEXT_SIGN flag before you can call secp256k1_sign.

@jimhansson
Copy link
Author

Sorry forgot to write that my (with-context .....) does setup and teardown of a context with the parameters given on the same line, so yes i have a context setup with SECP256K1_CONTEXT_SIGN before doing anything more.

@apoelstra
Copy link
Contributor

Can you post the source of the with-context macro?

@real-or-random
Copy link
Contributor

Try SECP256K1_CONTEXT_VERIFY instead

@apoelstra
Copy link
Contributor

apoelstra commented Nov 12, 2018

Oh, yeah, @real-or-random is right. I was confusing the ecmult_impl (verification) context with the ecmult_gen_impl (signing) context. secp256k1.ffi:secp256k1-ec-pubkey-tweak-add requires a verification context.

By the way, most crypto operations take 10s of microseconds, while context creation takes 10s of milliseconds, so you are paying a huge performance cost to use the with-X idiom this way.

@jimhansson
Copy link
Author

I tried to use a SECP256K1_CONTEXT_VERIFY before writting this, but it did not work. tried again and I got the something similier to "[libsecp256k1] illegal argument: secp256k1_ecmult_gen_context_is_built(&ctx->ecmult_gen_ctx)"

I then remembered that my (with-context) macro do call secp256k1_context_randomize when setting up the context..

(defmacro with-context ((name options) &body body)
	(with-gensyms (result-of-body)
		`(let ((,name (secp256k1-context-create ,options)))
			 ;; better way to seed this, don't wanna depend on ironclad
			 (cffi:with-foreign-array (seed_ (ironclad:make-random-salt 32) '(:array :unsigned-char 32))
				 (secp256k1-context-randomize ,name seed_) ; <- randomize
				 (let ((,result-of-body ,@body))
					 (secp256k1-context-destroy ,name)
					 ,result-of-body)))))

commenting out the line that does the randomization fixes the crash.

Things i would like to know/understand

  1. why should not a SECP256K1_CONTEXT_VERIFY context be randomized?
  2. If i have a context setup both for VERIFY and SIGN should it be randomized?

@apoelstra
Copy link
Contributor

apoelstra commented Nov 12, 2018

Oh, neat. secp256k1_context_randomize requires a context initialized for signing (if it's initialized for both that's fine). The reason is that randomization only applies to signing -- it's a sidechannel-resistance measure, and verification doesn't require any sidechannel protection. So it's not that it "shouldn't" be randomized, it's just that we have no notion of randomization for verification precomp tables, and don't need one.

It turns out that secp256k1_context_randomize has an ARG_CHECK checking that its context is initialized for signing, and will abort if the context isn't. This is a documentation bug; it's not listed in the header file and actually I had no idea that this behaviour existed. (I should check rust-secp256k1 to see if this abort condition is exposed by my bindings in safe code..)

Edit: Yep, this is a bug in rust-secp too. Thanks for the report! To work around your problem, just don't call randomize on a verification-only context.

@real-or-random
Copy link
Contributor

Yes this should be documented. But maybe randomizing a non-supported context should be a no-op instead of failing. I'm not sure.

@real-or-random
Copy link
Contributor

real-or-random commented Nov 15, 2018

But maybe randomizing a non-supported context should be a no-op instead of failing. I'm not sure.

@gmaxwell @sipa opinions?
An argument for a no-op is that it's future-proof. Maybe a verification context will need some randomization too. It sounds weird at first glance because verification is inherently a public operation but something like tweaking keys could involve secrets for example.

@sipa
Copy link
Contributor

sipa commented Jan 26, 2019

@real-or-random Agree, we should make randomizing a non-signing context a no-op.

Batch validation needs randomness, despite being a fully public operation.

real-or-random added a commit to real-or-random/secp256k1 that referenced this issue Jan 27, 2019
Before this commit secp256k1_context_randomize called illegal_callback
when called on a context not initialized for signing. This is not
documented. Moreover, it is not desirable because non-signing contexts
may use randomization in the future.

This commit makes secp256k1_context_randomize a noop in this case. This
is safe because the context cannot be used for signing anyway.

This fixes bitcoin-core#573 and it fixes rust-bitcoin/rust-secp256k1#82.
gmaxwell added a commit that referenced this issue Feb 21, 2019
6198375 Make randomization of a non-signing context a noop (Tim Ruffing)

Pull request description:

  Before this commit secp256k1_context_randomize called illegal_callback
  when called on a context not initialized for signing. This is not
  documented. Moreover, it is not desirable because non-signing contexts
  may use randomization in the future.

  This commit makes secp256k1_context_randomize a noop in this case. This
  is safe because the context cannot be used for signing anyway.

  This fixes #573 and it fixes rust-bitcoin/rust-secp256k1#82.

Tree-SHA512: 34ddfeb004d9da8f4a77c739fa2110544c28939378e779226da52f410a0e36b3aacb3ebd2e3f3918832a9027684c161789cfdc27a133f2f0e0f1c47e8363029c
real-or-random added a commit to real-or-random/secp256k1 that referenced this issue May 31, 2019
Before this commit secp256k1_context_randomize called illegal_callback
when called on a context not initialized for signing. This is not
documented. Moreover, it is not desirable because non-signing contexts
may use randomization in the future.

This commit makes secp256k1_context_randomize a noop in this case. This
is safe because the context cannot be used for signing anyway.

This fixes bitcoin-core#573 and it fixes rust-bitcoin/rust-secp256k1#82.
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 a pull request may close this issue.

4 participants