search

arduino-libraries / ArduinoECCX08

78 stars 49 forks source link

Possible crash on signatureLength() and appendSignature() #13

Closed guillep2k closed 5 years ago

guillep2k commented 5 years ago

Hi. There's a bug in ASN1Utils.cpp, in functions that deal with the creation of the signatures (signatureLength, appendSignature) where a value consisting of only zeros can cause problems.

while (*r == 0x00 && rLength) { should be changed to: while (rLength && *r == 0x00) {

and

if (*r & 0x80) { should be changed to: if (rLength && *r & 0x80) {

Similar changes should be granted to s and sLength.

Of course the probability of an all zeros signature is remote in a real-case scenario, but not in tests.

sandeepmistry commented 5 years ago

Hi @guillep2k,

Thanks for bringing this up, would you like to open a pull request with your suggested changes?

guillep2k commented 5 years ago

Hi, sorry, I don't currently have an Arduino environment. I just converted the library code into my project and realized this bug. My code is in C language.

sandeepmistry commented 5 years ago

Hi @guillep2k, I've prepared a pull request https://github.com/arduino-libraries/ArduinoECCX08/pull/14 for the zero condition, could you please take a look?

guillep2k commented 5 years ago

Hi, @sandeepmistry. It doesn't look good: while (*r == 0x00 && rLength > 1) The order is important: r is accessed before checking it's pointing to valid data (rLength>0); that's why it should be `while (rLength / first / && r == 0 / second /). Also, 1 is a valid value for rLength, it should not be skipped, sorLength > 1should berLengthorrLength > 0or evenrLength != 0`. In my opinion, the code changes should be made just as suggested in my first post.

sandeepmistry commented 5 years ago

I tried your suggestion, with a signature of all zero's but it resulted in an invalid ASN.1 self signed cert.

guillep2k commented 5 years ago

I tried your suggestion, with a signature of all zero's but it resulted in an invalid ASN.1 self signed cert.

Hi, @sandeepmistry,, sorry I've misinterpreted your comment. Yes! This is another bug I didn't notice before but you've caught correctly. Integers in BER need at least 1 byte of length. So, your correction (rLength > 1) is correct. However, you've missed a couple of places where this happens, plus there's a typo in the original mixing sLength with rLength.

Finally, both appendSerialNumber() and appendSignature() returned the wrong size when the values were negative. I guess it was working because the default serial number is "0x01".

I've made a new pull request above. Please check it out.

sandeepmistry commented 5 years ago

Closed via #16.