search

named-data / YaNFD

Yet another Named Data Networking Forwarding Daemon
https://pkg.go.dev/github.com/named-data/YaNFD
MIT License
12 stars 10 forks source link

ndn+security: compare digests in constant time #21

Closed yoursunny closed 2 years ago

Pesa commented 2 years ago

Why? What's the attack scenario? There are no secrets here.

yoursunny commented 2 years ago

Compare digests in non-constant-time == bad. Moreover, bytes.EqualFold would treat 0x41 and 0x61 as equal (it's Unicode case insensitive comparison), which is definitely wrong.

Pesa commented 2 years ago

Compare digests in non-constant-time == bad.

In some cases, yes. Not here, afaics. There is no secret information to be leaked. But you haven't answered the question.

Moreover, bytes.EqualFold would treat 0x41 and 0x61 as equal (it's Unicode case insensitive comparison), which is definitely wrong.

Well, that's irrelevant to the "constant time" discussion. Just use Equal instead of EqualFold.

yoursunny commented 2 years ago

What's the argument against using a constant time algorithm? Performance difference is negligible.

This PR is an improvement because it fixes bugs and adds tests.

Pesa commented 2 years ago

What's the argument against using a constant time algorithm? Performance difference is negligible.

Right. So let's add constant time comparisons EVERYWHERE. Let me open a PR for that...

Pesa commented 2 years ago

yay! 🎉 cargo cult ""security""! 🤦

zjkmxy commented 2 years ago

Well, I don't really care side channel attack. But since this fixes EqualFold problem and the performance cost is negligible, I think it should be merged. This does not mean I want to do constant time comparisons everywhere.

Pesa commented 2 years ago

The EqualFold bug has been there for months... now you suddenly felt the urge to fix it this very second, even though a better fix had already been suggested? Even then, the commit message is wrong, as it misrepresents the nature of the problem.

zjkmxy commented 2 years ago

I think I shouldn't discourage people by rejecting a useful PR, given that we do lack of human powers. If ConstantTimeCompare is harmful, I can patch it with another commit changing it back to Equal. YaNFD has not reached its first stable release, so I think this is acceptable. Actually just several months back, I merged a commit from Yash that even does not compile (the TCP unicast face one). I added a patch to make it work.

Pesa commented 2 years ago

Who said anything about rejecting... PRs can be amended though...

zjkmxy commented 2 years ago

OK. Thank you for your comments. I will consider that next time.