feat(crypto) add crypto.memcmp bindings#221
Conversation
| local _M = {} | ||
|
|
||
| function _M.memcmp(a, b, len) | ||
| if type(a) ~= "string" or type(b) ~= "string" or type(len) ~= "number" or len < 1 then |
There was a problem hiding this comment.
Thank you for the contribution!
However if we are only supporting comparing strings, why don't we have a for loop in lua land to compare it. It can also be done in a way to avoid timing attack right 🤔
There was a problem hiding this comment.
Fair point, I defaulted to using openssl because it's a battle tested implementation and to avoid writing a potentially unsafe version myself. I should be able to make it more generic and support more types.
There was a problem hiding this comment.
Yes, support some specific types of cdata pointer should also be fine.
There was a problem hiding this comment.
Done. Should support all lua types buffer.encode supports. strings and cdata are treated as is.
There was a problem hiding this comment.
Tests should be fixed now.
There was a problem hiding this comment.
ah actually, by cdata i was referring to passing them as-is without encoding it.
the function would accepts two string, or two cdata (treated as opaque pointer) and a third argument size.
user can always encode abitrary data as string as pass in so we shouldn't do the encode on our side.
There was a problem hiding this comment.
Yes It's already doing it that way. strings and cdata are not encoded but passed as-is. Everything else is encoded using the buffer built-in lib. You can also technically compare a cdata with a string, altough I'm not sure It could be useful.
There was a problem hiding this comment.
okay, that's good, i misread sorry.
let's do remove the buffer encode part, and only keep string and cdata, anything else we can return an error.
let's also add in the test case to compare two cdata pointer, we can ffi.new a pair of char* and compare them.
|
thank you @anvouk this is good to go ! the CI looks broken, i will fix it and then merge your PR. |
|
actually let me merge this first, will fix it on master |
2 participants
#220