Hi Everyone, *** Background ***
A question was asked on Stack Overflow that caused us to look at SecBlock's append operations. The SO question is at http://stackoverflow.com/q/34479894. The SO question also triggered a bug report on GitHub. The bug report is at https://github.com/weidai11/cryptopp/issues/92. The append operations are 'operator +=' and 'operator +'. The old code is at http://www.cryptopp.com/docs/ref562/secblock_8h_source.html#l00331, and the updated code is at https://www.cryptopp.com/docs/ref563/secblock_8h_source.html#l00563. They are effectively the same code modulo a size check and an assert. It appears 'operator +=' does not hand self-concatenation well, and it triggers an assert in misc.h for debug build. That is, the following code: SecByteBlock t; ... t += t; ... The code will break because when 'this' is grown, both 'this' and 't' is grown. After t is grown, we attempt to copy too many bytes: memcpy_s <https://www.cryptopp.com/docs/ref563/misc_8h.html#a5f6fcbaaf6f72fe46a6d911b033dfea0>(result.m_ptr+m_size, (t.m_size-m_size)*sizeof(T), t.m_ptr, t.m_size*sizeof(T)); That is 't.m_size' has effectively doubled in size *after* we performed the Grow(). *** memcpy_s *** The mitigating circumstance is 'memcpy_s'. You can see the source at https://www.cryptopp.com/docs/ref563/misc_8h_source.html#l00278 . On non-Window machines, it will throw an InvalidArgument <https://www.cryptopp.com/docs/ref563/class_invalid_argument.html>. Now, on Windows machines, Crypto++ memcpy_s or Windows memcpy_s may be called, depending on the user's configuration. That means the exception may not be thrown in some configurations. If the exception is *not* thrown, then I believe it results in a silent truncation. We don't know if anyone is actually using the code and possibly suffering a silent truncation. Or I have not seen a report of it. *** Decisions *** Now we have some decisions to make. Do we: (1) just check-in the patch? (2) check-in the patch and release a 5.6.4? Memory errors make me nervous, but that's a personal position. However, Crypto++ is a security library and I think we should exercise an abundance of caution, so I'm kind of leaning towards (2). *** Any thoughts or suggestions? Are there other alternatives we should consider? Jeff -- -- You received this message because you are subscribed to the "Crypto++ Users" Google Group. To unsubscribe, send an email to [email protected]. More information about Crypto++ and this group is available at http://www.cryptopp.com. --- You received this message because you are subscribed to the Google Groups "Crypto++ Users" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
