Only build bundled symbols if the user wants to use them

[str4d]

The secp256k1-sys README currently says:

Linking to external symbols

If you want to compile this library without using the bundled symbols (which may
be required for integration into other build systems), you can do so by adding
--cfg=rust_secp_no_symbol_renaming' to your RUSTFLAGS variable.

However, all this does is not use the bundled symbols by not renaming the Rust references to them. It doesn't prevent those symbols from being built, which becomes pointless work in this user-configured context.

Instead, the build.rs could check RUSTFLAGS for the presence of --cfg=rust_secp_no_symbol_renaming', and simply exit if it is present, since that is a clear indication from the user that they do not want these bundled symbols.

As a stretch goal, having this be controlled by a feature flag would enable the cc dependency to be dropped as well if not using the bundled symbols, but cc is a relatively small and self-contained dependency, so that's less of a problem.

[TheBlueMatt]

I think we want this to be a separate cfg flag - users may want to use the built-in library with an external implementation, but, yes, we should support this.

[apoelstra]

This flag exists for rust-secp257k1-zkp, right? Are there other users?

I think for secp-zkp @str4d may be correct that we're better off just not building included libsecp at all.

[elichai]

Do we want to support this though? I remember in the past we made some decisions here due to the fact that we control the exact version of libsecp

EDIT: I'm not saying we shouldn't, just that if we do we need to make sure we don't have anything that assumes implementation-defined details of libsecp

[TheBlueMatt]

IIRC part of the reason for this was to be able to link rust-bitcoin as a part of Bitcoin Core (which of course includes secp256k1). Sadly that work never progressed, so I'm not sure it has a user currently.

[apoelstra]

We could remove it in a RC of the next major version rev, and see if it breaks rust-bitcoinconsensus or rust-secp-zkp.

[str4d]

IIRC part of the reason for this was to be able to link rust-bitcoin as a part of Bitcoin Core (which of course includes secp256k1). Sadly that work never progressed, so I'm not sure it has a user currently.

This is in fact almost precisely my new use case. zcashd (based on Bitcoin Core) has a large Rust component, and until now that's only handled shielded parts of the protocol. But we've just started moving some of the address parsing logic into Rust, and need to validate secp256k1 compressed point encodings. I'm now using the secp256k1 crate with this flag to ensure that we consistently use the libsecp256k1 code vendored in the main repo; this issue was for optimising the resulting build flow.

[apoelstra]

Thanks :) ok we won't remove it. Let's give your "don't build libsecp" proposa a shot.

I wonder why we didn't try using this for secp-zkp rather than doing the crazy symbol-renaming thing that we did do..

[elichai]

I wonder why we didn't try using this for secp-zkp rather than doing the crazy symbol-renaming thing that we did do..

That fixes other problems too, let's say I'm using rust-secp256k1 regularly, but in my dependency tree it appears twice with 2 separate major versions (or once as rust-secp and the other as rust-secp-zkp)

[real-or-random]

I wonder why we didn't try using this for secp-zkp rather than doing the crazy symbol-renaming thing that we did do..

cc @jonasnick, you may know the answer here.

[apoelstra]

IIRC the secp-zkp solution pre-dated the rust-bitcoinconsensus solution ... we had originally intended to use renaming for rust-bitcoinconsensus, but our initial renaming solution was really shitty and didn't work (and this was at the time when Tamas was maintaining rust-bitcoinconsensus but no longer really active). Then Matt came up with this no-symbol-export solution, and then independently Elichai un-shittified the renaming script.

But my memory could be wrong on every one of those points :).

[thomaseizinger]

This flag exists for rust-secp257k1-zkp, right?

Not that I am aware.

In rust-secp256k1-zkp, we actually rely on the fact the contexts of libsecp256k1 and libsecp256k1zkp are the same IF we depend on a version of the libsecp256k1zkp code that was updated to the corresponding upstream version of libsecp256k1. Quite horrendous and results in an entire copy of the library that is effectively unused but it works 😅

Here for example we pass a Context that actually comes from secpk256k1-sys and thus libsecp256k1.

We did this so that rust-secp256k1-zkp essentially becomes a drop-in replacement for rust-secp256k1 and you can use the generated keys and everything with APIs in rust-bitcoin etc.

See discussions here and here.

[thomaseizinger]

entire copy of the library that is effectively unused

Actually not 100% about this. We don't create any bindings for it so I guess it depends on LTO doing its job to figure out that this C code is unused.

[apoelstra]

Unfortunately it is not unused. It is used by rust-secp, and we take context objects created by libsecp256k1 (through rust-secp) then pass them to libsecp256k1-zkp functions, which is questionably-defined behavior.

If we could just avoid complinig and using libsecp256k1 in rust-secp-zkp that would be a huge improvement.

[thomaseizinger]

Unfortunately it is not unused.

I was referring to the copy of libsecp256k1zkp that is mostly unused. (rust-secp256k1-zkp has its own depend directory with a copy of the vendor script, etc)
All the functions defined in there (duplicated from libsecp256k1) plus the static contexts etc.

[apoelstra]

Ah, I remember why we do not use this in rust-secp-zkp:

• It is not a feature flag, but a bare --cfg option, and therefore cannot be used by dependencies (needs to be integrated into non-cargo build processes)
• It cannot be a feature flag, for security reasons (it would be easy for a malicious crate, parallel to rust-secp in a dependency tree, to enable this feature, replace the symbols with their own compromised signing functionality, and there would be no way to prevent this)

Perhaps there are some launch-code-like tricks we could use to manually whitelist projects who are allowed to replace symbols ... but probably symbol renaming is easier than that.

[TheBlueMatt]

Hmmm, a malicious crate that gets pulled in as a dep can always create a build script that exports the relevant RUSTFLAGS environment variable, too, though - I've seen people demo this to use unstable features from "stable" crates.

[str4d]

• It cannot be a feature flag, for security reasons (it would be easy for a malicious crate, parallel to rust-secp in a dependency tree, to enable this feature, replace the symbols with their own compromised signing functionality, and there would be no way to prevent this)

Would the inverse be reasonable? Have a default-enabled feature flag that performs the rename and builds/bundles the library, and then rely on downstream users to disable it as they need. It would make transitive disabling harder (intermediate projects would need to expose an equivalent default-enabled feature flag), but has a safer fallback (since cargo takes the union of enabled flags, so as long as one dependency on secp256k1 doesn't disable the flag, every dependency ends up using the vendored symbols).

[apoelstra]

I think in practice this would usually break the build -- downsteam users can't unilaterally disable features, in case any of their dependencies enable it (and crate trying to, say, depend on rust-secp without the default features, would be forced to explicitly enable it).

@TheBlueMatt Can a downstream crate actually change the RUSTFLAGS that other crates are built with?

[TheBlueMatt]

@TheBlueMatt Can a downstream crate actually change the RUSTFLAGS that other crates are built with?

Intuitive I'd guess its race-y, but I have seen demos of crates that definitely change the environment enough to use unstable rust features with a stable compiler (ie via RUSTC_BOOTSTRAP). I assume if you happen to get run before the other crate in the build order you could somehow? In any case, on most platforms you can also register dlopen hooks and do other nasty stuff so I'm not sure how much of a concern this can really be?

[apoelstra]

Yeah, okay, I'm starting to be convinced that we could make this a cargo feature, and that the resulting attack surface would be no worse than the existing "if downstream users depend on malicious crates they are in trouble" surface.

[real-or-random]

Without knowing must about the specifics of rustc and cargo here, it seems reasonable to assume that a malicious crate has control over the entire build process, just because the build itself already can run arbitrary code. (I guess this is true for almost every non-trivial build system.) If I wanted to attack this crate specifically, I could even set the CC variable and attack it at the C level...

[str4d]

I think in practice this would usually break the build -- downsteam users can't unilaterally disable features, in case any of their dependencies enable it (and crate trying to, say, depend on rust-secp without the default features, would be forced to explicitly enable it).

I'm thinking of this problem as being analogous to no-std mode that is implemented additively via a std flag. We see similar kinds of build breakage there, and the same "need to re-enable the rest of the default features of you want only one disabled" behavior. As such, users are more likely to already be exposed to, and familiar with, that kind of feature flag usage.

But here it has the added advantage that builds wouldn't actually fail to compile in many cases - they would just use the secp256k1 vendored symbols (which are always guaranteed to be correct for a particular secp256k1 version) instead of the ones the user wanted. There will be scenarios in which this could still cause build breakage (if the user is modifying the underlying library to do fun things, like I presume rust-secp-zkp is), but that's functionally similar to the no-std situation, and would require similar resolution.

The other argument I'd make is that a feature flag that adds bundled symbols makes sense given that feature flags are additive; a feature flag that removes them both doesn't fit the additive model, and leaves downstream crates open to "spooky action at a distance": a user depends on two libraries that depend on this one, one of which enables that flag and provides its own symbols, and then the other crate is unexpectedly using an unrelated sibling's symbols. If the crate providing its own symbols legitimately needs them and can't use the default vendored symbols at all, it seems reasonable to have that crate detect this situation and break the build rather than silently altering other crates' expected symbols.

[apoelstra]

@str4d the build would fail to build if one dependency needed include-symbols to be off but another crate turned it on, because the former would build conflicting symbols, leading to a link error. (If it didn't include conflicting symbols, it would lead to a build failure without another dependency, since the symbols would then be missing.)

In any case, I am leaning toward putting an additive disable-bundled-symbols feature.

[str4d]

Ah, sorry for the confusion; I'd expected that this feature flag would retain the current renaming behavior (or rather, the opposite of the current behavior: enabling the flag would have this crate look for the renamed symbols instead of the originals). So in your first scenario above, two sets of symbols would be built, but only the renamed vendored ones would be linked.

[Kixunil]

Maybe I'm missing something here but aren't you folks trying to reinvent the native cargo feature that can override build scripts?

[daira]

I think in practice this would usually break the build -- downsteam users can't unilaterally disable features, in case any of their dependencies enable it (and crate trying to, say, depend on rust-secp without the default features, would be forced to explicitly enable it).

I think that for the cases where this is likely to be needed, the downstream user controls the whole dependency tree. So if one of their dependencies enabled the feature then that would be a bug they would be in a position to fix, e.g. by vendoring that dependency. Note that if --cfg=rust_secp_no_symbol_renaming is retained, then disabling the feature flag is effectively an optimization: that copy of libsecp256k1 won't be used even if it is built. (To be clear, what I'm suggesting is that the "no symbol renaming" behaviour would occur if either --cfg=rust_secp_no_symbol_renaming is set or if an include-symbols feature is not enabled.)

[apoelstra]

Interesting @Kixunil ... this seems very close to what we want, but not quite there. Could we use this in rust-secp256k1-zkp to say "ignore the rust-secp256k1 build script, use ours instead" and have this do the right thing even if rust-secp and rust-secp-zkp are parallel dependencies of some third crate?

[Kixunil]

@apoelstra this is only usable from binaries, so it can't be used out-of-the-box. Before I start to figure out some horrible hack I'd like to understand why there are even two versions of the library.

• Why is C implementation forked? Could -zkp be merged or implemented using only public API?
• If above is impossible, do we really need two rust -sys libraries? They are already modified (symbol renaming), so why not do any other modifications (mainly ifdefs, I think) that would basically translate to zkp feature?
• If we really need to have everything duplicated is it even a good idea to try merging the libraries? I worry about problems with API/ABI breaks if we can't enforce versions.

[apoelstra]

• Why is C implementation forked?

Because -zkp implements a lot of bleeding-edge crypto which is not appropriate for libsecp and is not useful for Bitcoin Core.

• Could -zkp be merged or implemented using only public API?

No.

• If above is impossible, do we really need two rust -sys libraries? They are already modified (symbol renaming), so why not do any other modifications (mainly ifdefs, I think) that would basically translate to zkp feature?

I don't understand this question. We have two C libraries so we need two sets of FFI bindings.

• If we really need to have everything duplicated is it even a good idea to try merging the libraries?

We need to do this so that we can share context objects, which are megabytes in size and take many milliseconds to create. There is no risk as long as the libsecp functions have the same signatures in both libraries, which we (the maintainers of both libsecp and libsecp-zkp) can ensure, because (a) we are (mostly) the same people, (b) libsecp-zkp frequently merges in upstream changes from libsecp, (c) libsecp-zkp never messes with the upstream functionality.

I wouldn't characterize this as "merging libraries". The idea is that libsecp256k1 is replaced wholesale with libsecp256k1-zkp when rust-secp256k1-zkp is in the dependency tree.

• I worry about problems with API/ABI breaks if we can't enforce versions.

We can enforce versions, using symbol renaming.

[Kixunil]

We have two C libraries so we need two sets of FFI bindings.

Didn't check but AFAIU zkp is superset of thenon-zkp. Therefore we could just vendor zkp one and disable added zkp stuff by default - enable with feature. Am I missing something?

We need to do this so that we can share context objects, which are megabytes in size and take many milliseconds to create.

Great motivation! However did a quick look at zkp crate and it seems completely disconnected from upstream. Thus this seems to be a problem of Rust code, not C code because the context type is not interchangeable.

[apoelstra]

Didn't check but AFAIU zkp is superset of thenon-zkp. Therefore we could just vendor zkp one and disable added zkp stuff by default - enable with feature. Am I missing something?

I am not completely opposed to this, but I think it would be scary and surprising for users if the "Rust bindings to libsecp256k1" actually were bindings to a different project with far fewer eyes on it and a much lower threshold for acceptance of cryptosystems.

Yes, it is a superset, so this shouldn't matter ... but "shouldn't matter" isn't enough here, in my opinion.

Great motivation! However did a quick look at zkp crate and it seems completely disconnected from upstream. Thus this seems to be a problem of Rust code, not C code because the context type is not interchangeable.

rust-secp-zkp uses the rust-secp Context type. This means that we are using a context object from libsecp256k1, and passing it to functions from libsecp256k1-zkp, which definitely is "the C code's problem". (In fact I am quite certain that this is undefined behavior, hence my strong desire to resolve this issue in a better way..)

[daira]

It seems odd to me that libsecp256k1-zkp tries to be a replacement for libsecp256k1, rather than just a separate library that has libsecp256k1 as a dependency. I realize it might be difficult to change that design now, but it appears to be what is causing these problems. In the alternative design where -zkp is a separate library, its context object can just hold a pointer to a libsecp256k1 context object.

[real-or-random]

It seems odd to me that libsecp256k1-zkp tries to be a replacement for libsecp256k1, rather than just a separate library that has libsecp256k1 as a dependency. I realize it might be difficult to change that design now, but it appears to be what is causing these problems. In the alternative design where -zkp is a separate library, its context object can just hold a pointer to a libsecp256k1 context object.

It's certainly odd. The main reason for this decision is that libsecp256k1 doesn't expose low-level data types and functions (operation on group or field elements). So if you want to reuse the low-level implementation for your scheme, the natural way is to fork the project and have access to all the internal low-level functionality (simply because it's not internal to your library).

The reasons for not exposing low-level functionality include:

• it would be hard for libsecp256k1 to guarantee stability of a low-level API because internals are changed very often
• the desire to make user-facing crypto APIs as safe as possible
• it wouldn't be unreasonable for libsecp256k1 maintainers not to care too much about extensions because libsecp256k1 is a sub-project of Bitcoin Core and

Now how can we do better? The goal is to expose the internal functionality somehow.

The obvious idea is to split libsecp256k1 into a low-level library and a high-level library. But that seems hard without the willingness to guarantee API stability. Moreover, libsecp256k1 currently uses unity builds (single compilation unit) to help the compiler optimizing. Nowadays, LTO is more common than it was when the library was created but maybe not everyone (e.g., embedded) has great compilers and we don't really checked if there's a performance penalty even with LTO enabled. (Maybe everyone has a decent compiler and this is not an issue -- but we don't have the data.)

Another idea is to expose the internal functionality differently, e.g., have a switch that just gives you access to everything. This would be intended to enable "add-ons" such as -zkp, and the switch should not used by end users, and clearly marked as such. That sounds reasonable to me personally because I think it may be better than the current model because then libsecp256k1 could be a real dependency of -zkp. @jonasnick created a PoC of a similar idea here BlockstreamResearch/secp256k1-zkp#153 (It seems again odd that this is a PR to -zkp but I think this just because it's not ready to show yet.) But then there are some questions to answer: Is this a compile-time or a run-time switch? Will we still have LTO issues? Is it then possible to write add-ons in other languages, e.g., Rust? Can we support more than one add-on?

Happy to hear any thoughts or other suggestions. We often think about this problem.

[Kixunil]

@apoelstra docs.rs isn't showing those types as reexports but looking at the code they are. Weird.

Thinking about it again, those megabytes/milliseconds are only spent if the contexts are created and should be unrelated to the library being linked twice. I don't see how it could be UB if the vendored libraries share exactly same base code but I suspect this can't be guaranteed without severely restricting dependencies. Thus I agree with resolving this.

If I understand correctly what people said here it should be possible to make zkp source code split into two parts: non-zkp and the add-on. They can still be joined at build time using #include. Separating them should make it trivial to prove that non-zkp part is the same as upstream lib (modulo renamings) and the zkp, default-off feature would turn on the add-on if needed. This should be easy to check for anyone concerned as I expect such concerned people to also audit their dependencies.

[apoelstra]

@Kixunil the #includes are repeated in secp256k1.c, tests.c, tests_exhaustive,c, configure.ac, Makefile,am, and in all the CI stuff (and possibly elsewhere, this is off the top of my head). I would say it's "nearly" trivial to vet that the code is actually the same, but still not trivial enough.

In the past, we have also had some nontrivial changes in the base code -- our old rangeproof code modified the context objects to include another precomp table, for example. I can't guarantee we'll never do that again. (Though if if we did do that again, it would break our current context-object-sharing hack..)

Another concern I have is that secp256k1-zkp is ideally not the only library that tries to add cool/interesting cryptosystems to libsecp. So if we come up with a solution that works for -zkp but not for anybody else, that's not great.

[Kixunil]

I would say it's "nearly" trivial to vet that the code is actually the same, but still not trivial enough.

Do you know anyone who would complain about it? Is the difference worth adding hacks into these libraries?

Another concern I have is that secp256k1-zkp is ideally not the only library that tries to add cool/interesting cryptosystems to libsecp. So if we come up with a solution that works for -zkp but not for anybody else, that's not great.

In that case I see no solution other than exporting internals. If there are two orthogonal libraries then you can't use one to replace another.

[apoelstra]

Do you know anyone who would complain about it? Is the difference worth adding hacks into these libraries?

I would complain. It makes me deeply uncomfortable to be pulling -zkp stuff into something which claims to be non-zkp bindings, even if they are not linked and even if it's possible to manually check that the C code is consistent. And there are no hacks in rust-secp, only in rust-secp-zkp.

In that case I see no solution other than exporting internals. If there are two orthogonal libraries then you can't use one to replace another.

This is a good point.

[Kixunil]

Why? Is the issue possibly misleading name or something else?

[apoelstra]

@Kixunil yes, because of the misleading name, and beacuse it would then mean that there are no longer standard Rust bindings for libsecp.

libsecp256k1 has much higher standards for code quality, QA/review, and cryptographic soundness than does libsecp256k1-zkp. The fact that secp-zkp is a superset seems to me like an accident of history/ease of maintenance thing for the libsecp-zkp developers, and even if they promised that the libraries would always be compatible in this way (which we do not) and if we were exactly the same set of people (we are not), such promises aren't what users expect when they look for libsecp256k1 bindings.

[Kixunil]

Although I prefer to look at actual merits of things and verifying stuff I think I can see that some people would find it fishy. I wonder if it'd be also bad to still include the whole library but provide zkp feature that just switches the internal C library and exports new stuff. With all disclaimers about it being lower-quality and unstable. (Maybe call it unstable-zkp instead.)

[apoelstra]

I might be okay with that, but I still dislike the idea of rust-secp-zkp stuff leaking into rust-secp.

[Kixunil]

FTR I don't strictly like it either, I just don't see any better solution.

[tcharding]

Hmmm, a malicious crate that gets pulled in as a dep can always create a build script that exports the relevant RUSTFLAGS environment variable, too, though - I've seen people demo this to use unstable features from "stable" crates.

Hi @TheBlueMatt, do you still stand by this statement? I spent some time recently thinking about this and discussing it with @apoelstra (over here) and we came to the conclusion that a malicious dependency could not set RUSTFLAGS. The unstable thing demo'd was probably using regular cargo features? I'm asking you because just because I couldn't work out how to set it does not mean its impossible. Thanks

[Kixunil]

If you have a malicious dependency, you're already completely screwed. There's so much stuff malicious dependencies can do that it makes no sense to worry about them setting env vars. Review your deps, verify signatures or whatever you need.

[apoelstra]

I need to re-load this discussion into my head, but I've become convinced that malicious deps basically mean that you're screwed. So the reasons to use cfg flags over Cargo features are basically

• To make it much harder to enable some flag by accident, eg by using --all-features and not looking
• For things that really shouldn't be controllable by other dependencies, only by the final build config (e.g. our fuzz harness crap which should only be turned on by a fuzz harness, which has the ability to set RUSTFLAGS)
• If we needed a "feature" that couldn't be made additive or which would otherwise violate cargo constraints.

There are some parallel discussions about the nature of libsecp vs libsecp-zkp but I don't think that has anything to do with this. (At least, there are other places to discuss it, such as on one of those two repos). I'll just reiterate that I think it's totally reasonable for somebody to trust/whitelist libsecp but not libsecp-zkp, so we can't pull libsecp-zkp directly into this library.

So at first glance, I think:

• The existing rust_secp_no_symbol_renaming cfg flag needs to be a cfg flag because it's not additive -- either enabling it or disabling it would break compilation for anyone who needs it.
• A rust_secp_do_build_external_symbols_at_all Cargo feature would be additive, but would need to be on by default ... so things like bitcoinconsensus or maybe rust-secp-zkp which need it to be off would be screwed.
• A rust_secp_dont_build_external_symbols Cargo feature would get us the behavior we want, but wouldn't be additive.

All this to say that I think our best choice, which is still not great, would be to replace rust_secp_no_symbol_renaming with rust_secp_dont_build_external_symbols, keep this as a cfg flag rather than a Cargo feature, and then downstream users of libbitcoinconsensus or secp-zkp or whatever would need to be explicitly instructed to enable it. (Alternately, if it turned out there was a way for other deps to hack RUSTFLAGS, I wouldn't mind if they did so. But I think we've established they can't.)