Re: Possibly out of sync/broken openssl code

[nisarg joshi <njnisarg%gmail.com@localhost>]

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 <christos%zoulas.com@localhost> 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 <njnisarg%gmail.com@localhost> 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