Seed hashing doesn't assume/enforce seed size

[Fang-]

Latest and final spec allows us to assume that all seeds are always 32 bytes/256 bits. The hashing implementation (sha256()) doesn't acknowledge this, and happily takes shorter seeds. Arguably, it should add as many leading zeroes as necessary to make the seed (excluding the salt) 32 bytes long.

This is currently a source of discrepancy between keygen-js and keygen-hoon. I'm arguing here that keygen-hoon is doing it right, but do fight me on this if you disagree.

[flowerornament]

Zero-padding seems right!

On Oct 29, 2018 at 7:22 PM, <Fang (mailto:notifications@github.com)> wrote:

Latest and final spec allows us to assume that all seeds are always 32 bytes/256 bits. The hashing implementation (sha256()) doesn't acknowledge this, and happily takes shorter seeds. Arguably, it should add as many leading zeroes as necessary to make the seed (excluding the salt) 32 bytes long.

This is currently a source of discrepancy between keygen-js and keygen-hoon. I'm arguing here that keygen-hoon is doing it right, but do fight me on this if you disagree.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub (https://github.com/urbit/keygen-js/issues/35), or mute the thread (https://github.com/notifications/unsubscribe-auth/AAB8HpgMKMjmZC4TvzpjFol4TdRHXrWQks5up0dogaJpZM4X_yWQ).

[jtobin]

draws cutlass en garde! 😄

Should it though? I checked the SHA-256 spec -- the padding scheme is a tad different, I assume node/browsers implement it (please don't let this be a stupid assumption, please don't let this be a stupid assumption, please..), and I am content to leave it at that. I.e., we define our wallet in terms of SHA-256, not SHA-256 with custom padding.

For 256-bit seeds, which we use, the implementations match regardless, non?

prepares to parry thrust

[Fang-]

Who do you think you are, bringing a sword to a fistfight?

Oh, this isn't about the padding that happens inside of SHA-256, but rather how we treat our input to it.

If you go purely by the Urbit Wallet spec, you can assume that all input to SHA-256 is always 32 bytes. This is indeed always the case in the real world (I assume JS SHA-256 to spit out leading zeroes if they're there), but our test cases currently break that very logical assumption, suddenly forcing SHA-256 to take an input that isn't of the expected 32-byte length.

I believe we agreed on fixed-length seeds, and since only seeds go into SHA-256, they need to be treated as if they were of that fixed length. The only place where this is not true is our test cases. Either those should be fixed to only provide appropriate-length seeds, or the hashing function needs to left-pad with zeroes prior to SHA-256.

This might seem like a stupid, trivial thing. And it is! But it matters to the hoon implementation as it currently stands, so I care about it.

(Strictly enforcing seed size for arbitrary inputs does add a bit of complexity to the spec, but then I don't think this functionality is supposed to be exposed in the first place?)

[jtobin]

Oh, this isn't about the padding that happens inside of SHA-256, but rather how we treat our input to it.

Right, but it seems to me that one would be redefining the hash function from SHA-256 to if bitlength(input) >= 256 then SHA-256(input) else SHA-256(pad-input-to-256-bits(input)), which circumvents whatever padding that SHA-256 would otherwise get up to.

I think your suggestion to change the change the seed test inputs to 256-bits is a good one, though, and is the more appropriate route to take. Want to do that?

sheathes cutlass

Before this day, I had never met my match in battle!

[Fang-]

Sure, that sounds like a fine compromise. And then we just label <256 bit/<32 byte seeds unsupported? We don't officially expose these interfaces anyway.

[Fang-]

Actually, I lied. We use 64-byte seeds when we're deriving the network keys from the management mnemonic, since the seed-from-mnemonic is always 512-bits... Fug.

[jtobin]

Derp. I thought I had resolved this in my commit. Back open with thee!

The non-256-bit test seeds are all 256-bit now, anyway.

[jtobin]

I'm gonna close this again as it seems it's been cleared up. Feel free to reopen again otherwise, cutlasses optional.