bitshares / bsips

BitShares Improvement Proposals and Protocols. These technical documents describe the process of updating and improving the BitShares blockchain and technical ecosystem.
https://bitshares.github.io
63 stars 86 forks source link

Remove SHA1 #209

Closed jmjatlanta closed 5 years ago

jmjatlanta commented 5 years ago

This PR will request removal of the SHA1 algorithm from HTLCs as par of BSIP 64.

pmconrad commented 5 years ago

Are there any other chains out there that we'll no longer be able to HTLC with? IOW they have SHA1 but no other hash algorithm that is compatible with us?

Technical note: we cannot simply remove SHA1 from the htlc_hash static_variant. We'll have to forbid its usage in the hardfork, and if it isn't used until then we can later replace it with an invalid dummy. Code-wise this will be much uglier than simply keeping SHA1.

pmconrad commented 5 years ago

Also note that SHA1 is broken in that it is practical to produce two documents that have the same hash value. However, this would not allow an attack against HTLC. SHA1 in HTLC would be insecure if it was possible to produce a plaintext for any chosen SHA1 hash value. I mean, we still should discourage its use, but I don't see an urgent need to actually remove it.

ryanRfox commented 5 years ago

Perhaps it is best to leave it in. Suggest to note the risks in documentation. (Out of scope for this PR: Also suggest @nathanhourt not use SHA1 in the TNT design.)

nathanielhourt commented 5 years ago

Technical note: we cannot simply remove SHA1 from the htlc_hash static_variant. We'll have to forbid its usage in the hardfork, and if it isn't used until then we can later replace it with an invalid dummy. Code-wise this will be much uglier than simply keeping SHA1.

<slyly> Well now... that depends... if, by the time of the hardfork, no one ever used the SHA1 HTLC, then we could just quietly rewrite that type tag to reference HASH160... it would be an epic revisionist hack, but it would make the code prettier :smirk: </slyly>

pmconrad commented 5 years ago

if, by the time of the hardfork, no one ever used the SHA1 HTLC, then we could just quietly rewrite that type tag to reference HASH160

Hehe, yes, that would work. We could create a softfork in advance to prevent SHA1 from being used until the hardfork. Without the softfork this would be a risky undertaking.

abitmore commented 5 years ago

We could create a softfork in advance to prevent SHA1 from being used

This sounds like a good idea. Which version should we sneak it in?

pmconrad commented 5 years ago

The softfork would be a witness-only update. I believe they are running various 3.x versions so we'd have to create another set of patch versions.

ryanRfox commented 5 years ago

Who is willing to confirm SHA1 has not been used thus far? I support a soft fork in this case.

jmjatlanta commented 5 years ago

Who is willing to confirm SHA1 has not been used thus far? I support a soft fork in this case.

There have been at least 3 HTLCs that used SHA1 on mainnet.

pmconrad commented 5 years ago

Thanks. So SHA1 stays in, technically. No softfork. Next question is, do we forbid using SHA1 in the future? I don't see a reason to do so.

ryanRfox commented 5 years ago

Let's leave it in. Suggest we note the potential risks to users if using this hash function in our documentation. Ready for close.