So the type MD32_REG_T is defined as either long or int in md32_common.h, and it is being used in md4_dgst.c and rmd_dgst.c for carrying out digest calculations. These calculations have shift operations and UBSan is reporting overflows for these operations because MD32_REG_T is int or long. I checked upstream openssl/openssl and they have used prefixed MD32_REG_T with unsigned as in ==> "unsigned MD32_REG_T" which we should have been following. I also noticed that the NetBSD implementation of other crypto functions might diverge because of such minor differences(which is what I tried to convey in the previous mail along with the commits that are involved).
I have created a patch for md4_dgst.c and rmd_dgst.c which will tackle the UBSan reports by using "unsigned MD32_REG_T" which is in line with upstream implementation(openssl). I will attach the patch with this mail. Let me know if there are any issues with the same or if I am missing something. Thank You Nisarg S. Joshi On Sun, Feb 16, 2020 at 6:07 AM Christos Zoulas <chris...@zoulas.com> wrote: > Yes, because "unsigned long" on _LP64 is not 32bits. What is the undefined > behavior you are trying to fix? > > christos > > On Feb 15, 2020, at 7:03 PM, nisarg joshi <njnis...@gmail.com> wrote: > > Hi community, > > > While trying to fix UBSan Undefined behavior reports for md4_dgst.c and > rmd_dgst.c files which reported issues for the overflow of signed ints etc, > I came across a possible out of sync tree of openssl crypto implementations. > > > The UBSan report required usage of unsigned int(or long) for MD32_REG_T > type and upon checking the upstream openssl/openssl, it seemed to be using > the correct data type that would not throw that particular error. Upon > tracing the changes in NetBSD openssl code, I found that there was a commit > that made changes to the openssl version imported from upstream in 2009. > The commits are: > > 1.) Original codebase ==> > https://github.com/NetBSD/src/commit/df8082221a1522cb9f9711f39f81796132552e1d > > 2.) Changes made ==> > https://github.com/NetBSD/src/commit/309c6b7ae7a7de3f477f9f707d08a4fc12f01b62 > > > The 2nd commit listed above made changes to the original codebase in such > a way that it changed MD32_REG_T to uint32_t and diverged from the > upstream. But later MD32_REG_T has been changed to int or long and has > become out of sync with the upstream implementation. These changes not only > affect the files mentioned earlier but also affect a lot of the files. > > > I would request someone to look into the matter and try to sync the > implementation and our local modifications to the upstream. > > > Thank You > > Nisarg S. Joshi > > <sanitizer.log> > > >
diff --git a/crypto/external/bsd/openssl/dist/crypto/md4/md4_dgst.c b/crypto/external/bsd/openssl/dist/crypto/md4/md4_dgst.c index a3c92f7..5319618 100644 --- a/crypto/external/bsd/openssl/dist/crypto/md4/md4_dgst.c +++ b/crypto/external/bsd/openssl/dist/crypto/md4/md4_dgst.c @@ -37,10 +37,10 @@ int MD4_Init(MD4_CTX *c) void md4_block_data_order(MD4_CTX *c, const void *data_, size_t num) { const unsigned char *data = data_; - register MD32_REG_T A, B, C, D, l; + register unsigned MD32_REG_T A, B, C, D, l; # ifndef MD32_XARRAY /* See comment in crypto/sha/sha_locl.h for details. */ - MD32_REG_T XX0, XX1, XX2, XX3, XX4, XX5, XX6, XX7, + unsigned MD32_REG_T XX0, XX1, XX2, XX3, XX4, XX5, XX6, XX7, XX8, XX9, XX10, XX11, XX12, XX13, XX14, XX15; # define X(i) XX##i # else diff --git a/crypto/external/bsd/openssl/dist/crypto/ripemd/rmd_dgst.c b/crypto/external/bsd/openssl/dist/crypto/ripemd/rmd_dgst.c index aa4787d..a1670c7 100644 --- a/crypto/external/bsd/openssl/dist/crypto/ripemd/rmd_dgst.c +++ b/crypto/external/bsd/openssl/dist/crypto/ripemd/rmd_dgst.c @@ -36,11 +36,11 @@ int RIPEMD160_Init(RIPEMD160_CTX *c) void ripemd160_block_data_order(RIPEMD160_CTX *ctx, const void *p, size_t num) { const unsigned char *data = p; - register MD32_REG_T A, B, C, D, E; - MD32_REG_T a, b, c, d, e, l; + register unsigned MD32_REG_T A, B, C, D, E; + unsigned MD32_REG_T a, b, c, d, e, l; # ifndef MD32_XARRAY /* See comment in crypto/sha/sha_locl.h for details. */ - MD32_REG_T XX0, XX1, XX2, XX3, XX4, XX5, XX6, XX7, + unsigned MD32_REG_T XX0, XX1, XX2, XX3, XX4, XX5, XX6, XX7, XX8, XX9, XX10, XX11, XX12, XX13, XX14, XX15; # define X(i) XX##i # else