search

tillitis / tillitis-key1

Board designs, FPGA verilog, firmware for TKey, the flexible and open USB security key 🔑
https://www.tillitis.se
395 stars 24 forks source link

Idea: multiple inputs to `blake2s` in firmware #157

Open cobratbq opened 7 months ago

cobratbq commented 7 months ago

Note that I did have to investigate how blake2s in firmware works: I was looking for a way to hash multiple byte-arrays, which isn't possible now. The thing I run into is when I have several byte-arrays, so I want to pass those on, instead of concatenating them first. That blake2s in firmware is a one-shot function: init + update + finalize is not so much the problem, but being limited to a single byte-array of input sometimes is. Obviously I can use the monocypher mechanisms, obviously I can concatenate, also vararg isn't an option given that input is arbitrary-length therefore needs length separately specified.

One option to consider would be to simply provide parameters for 2nd, 3rd, 4th input and accept 0 length to ignore. I don't think one often needs tens of inputs, but more than one is quite common. For example: monocypher recommends performing a keyed hash with the X25519 shared secret as key, and both X25519 public keys as input. This step is a necessary follow-up step, in some cases such as this, to produce uniformly random bytes as secret (for example when used as a symmetric key).

I would suggest the extra inputs as something to consider. Especially if use is as simple as passing in 0 values for unwanted additional arguments/parameters, i.e. with conditional to skip 0-length input, a pointer address 0 is no problem. Which is straight-forward, relatively cheap and simple to use. (If I read the firmware function blake2s_update correctly, then you can even pass on 0 (value) arguments and they will be processed in same way as conditionally calling additional blake2s_update.)

update I just realized this might not be the appropriate place for this report.

mchack-work commented 7 months ago

Thanks for the idea. I moved the issue to the tillitis-key1 repo where the firmware lives for further consideration. I'm leaving this open for now.

If your use case is urgent I urge you to implement this outside of the firmware and simply not use the blake2s() "system call".

-- Michael "MC" Cardell Widerkrantz https://tillitis.se/

cobratbq commented 7 months ago

[..] If your use case is urgent I urge you to implement this outside of the firmware and simply not use the blake2s() "system call".

Not urgent. Merely feedback provided for consideration/evaluation of the use-case.

dehanj commented 6 months ago

I'm not sure if I'm getting the problem. Isn't it easier to just use as many blake2s_update() as you have inputs, instead of changing the API of blake2s()?

In other words, use it in a similar fashion as we are in compute_cdi(). Or am I missing something?

cobratbq commented 6 months ago

In other words, use it in a similar fashion as we are in compute_cdi(). Or am I missing something?

Are you looking at this from the perspective of a TKey application that accesses the exposed blake2s function through the function pointer in the memory map? AFAIK, I only have access to one function: blake2s.

dehanj commented 6 months ago

Okay, no I was not - it was not mentioned in the post either but now it makes much more sense. I thought you meant in firmware, where they are exposed.

Anyhow, I don't believe the API change is justified, and what should we do the day one needs five inputs instead of four? Since blake2s doesn't run in hardware (today at least) the benefit of it is minimal. My suggestion is instead that tkey-libs include the implementation of blake2s_init(), blake2s_update() and blake2s_final() in tkey-libs, and then one can choose to use them and include as many arrays as one might want.

The day it may run in hardware, it would be interesting to have those exposed via firmware - but we are not there yet.

cobratbq commented 6 months ago

Indeed, the description is not completely clear. It was transferred from the tkey-libs repository.

Anyhow, I don't believe the API change is justified, and what should we do the day one needs five inputs instead of four? Since blake2s doesn't run in hardware (today at least) the benefit of it is minimal. My suggestion is instead that tkey-libs include the implementation of blake2s_init(), blake2s_update() and blake2s_final() in tkey-libs, and then one can choose to use them and include as many arrays as one might want.

That's a fair point, but have you considered probabilities?

The solution I'm proposing, satisfies the use-cases of 1, 2, 3 and 4 inputs, hence combining the probability of each. My claim is that past 4 inputs, the need is significantly lower. Look at your mentioned use-case for cdi (2 or 3 inputs). Blake2s is already needed in firmware, so it is present. Offering more inputs means not having to waste application memory on duplicate code.

As a side-note: I proposed this solution to respect the decision (possibly by limitation) to expose a single one-shot function.

The day it may run in hardware, it would be interesting to have those exposed via firmware - but we are not there yet.

Sure, that makes sense.