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

Key Translation Duplicate Check Safety Hole #238

Open
JeremyRubin opened this issue Mar 5, 2021 · 8 comments · May be fixed by #556
Open

Key Translation Duplicate Check Safety Hole #238

JeremyRubin opened this issue Mar 5, 2021 · 8 comments · May be fixed by #556

Comments

@JeremyRubin
Copy link
Contributor

When translating keys in a given policy/miniscript, translating:

and(pk(A), pk(B))

with F: { A -> X, B -> X }

creates

and(pk(X), pk(X))

bypassing the check that no duplicate/repeated keys are used.

Fixing this requires changing from a function to a bijective map of some kind, either by tracing the function as it's used or switching the type externally to some kind of map.

cc @apoelstra @sanket1729, creating an issue for this to track fixing it

@sanket1729
Copy link
Member

A simpler but slightly inefficient solution might be to retraverse the tree again. I am not against doing this because we are already traversing the tree once when translating.

@JeremyRubin
Copy link
Contributor Author

@sanket1729 I advise caching because the function could have interior mutable state and lie to you...

@sanket1729
Copy link
Member

@JeremyRubin, We can call the translate function only once and save the output policy/miniscript which possibly contains duplicate keys. Do the checks again on the translated miniscript/policy.

Am I missing something or are you looking for an efficient way to it while creating(one traversal) while aborting at the first duplicate?

@JeremyRubin
Copy link
Contributor Author

Ah, certainly doing full checks after translating is the safest choice, but I think given that most functions are probably just getters around a map might make sense to do something just passing a map in. I'm for whatever works and closes the safety hole.

When you said 'retraverse' I thought you meant while translating in a manner that calls the function more than once per key. (Interestingly this sort of trick could be used to translate a duplicated-key script to non duplicated if it uses e.g. sequential HD derivation per call per key)

@sanket1729
Copy link
Member

Planning to cleanly address this after #493.

@benma
Copy link
Contributor

benma commented Jun 14, 2023

@sanket1729 reminder now that #493 is merged. I just experimented with this library and was quite confused why there was no error with duplicate keys present.

@sanket1729 sanket1729 linked a pull request Jun 15, 2023 that will close this issue
@sanket1729
Copy link
Member

@benma. Are you facing the no error when translating keys using TranslatePk? This is fixed in #556. All regular methods from_str, parse, compile etc should currently be fail on duplicated keys.

@benma
Copy link
Contributor

benma commented Jun 15, 2023

@benma. Are you facing the no error when translating keys using TranslatePk?

Yes exactly. Thanks for the fix.

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.

3 participants