[Bug 3983] New: UMAC corruption for messages >1024 bytes due to strict-aliasing violation

[bugzilla-daemon at mindrot.org]

https://bugzilla.mindrot.org/show_bug.cgi?id=3983

            Bug ID: 3983
           Summary: UMAC corruption for messages >1024 bytes due to
                    strict-aliasing violation
           Product: Portable OpenSSH
           Version: 9.6p1
          Hardware: Other
                OS: Linux
            Status: NEW
          Severity: normal
          Priority: P5
         Component: ssh
          Assignee: unassigned-bugs at mindrot.org
          Reporter: talaberp at gmail.com

Hi,

I have run into a strange issue. If for SSH connection UMAC is
negotiated I got `Corrupted MAC on input` when I try to send a few KB
of data.
I managed to narrow down the issue that above 1024 bytes of data passed
for umac calculation, the issue occurs, but only in case the compiler
uses optimization. This limit comes from `L1_KEY_LEN` in umac.c which
decides to pass code execution on "long message path" towards
poly_hash() instead of "short path" using ip_short() in uhash_final.
The problem was on "long path" as `UINT64 poly_key_8[STREAMS];` already
contained corrupted data, because `endian_convert()` called earlier by
`uhash_init()` used a code which violates strict-aliasing rule when
manipulating poly_key_8: a U64[] data via an U32* pointer.
When compiled in debug mode, without optimization it was fine.
Otherwise, it caused the issue, unless `-fno-strict-aliasing` was
passed as well, which fixed the violation above. Switching this code
part to use memcpy instead fixed the issue for me:
```
[umac.c#L556](https://github.com/openssh/openssh-portable/blob/master/umac.c#L556)
     } else if (bpw == 8) {
-        UINT32 *p = (UINT32 *)buf;
-        UINT32 t;
+        UINT8 *p = (UINT8 *)buf;
+        UINT32 hi, lo;
         do {
-            t = LOAD_UINT32_REVERSED(p+1);
-            p[1] = LOAD_UINT32_REVERSED(p);
-            p[0] = t;
-            p += 2;
+            hi = LOAD_UINT32_REVERSED(p);
+            lo = LOAD_UINT32_REVERSED(p + 4);
+            memcpy(p, &lo, sizeof(lo));
+            memcpy(p + 4, &hi, sizeof(hi));
+            p += 8;
         } while (--iters);
     }
```
Since OpenSSH uses `-fno-strict-aliasing` by default, issue remained
hidden for a while.
[configure.ac#L218](https://github.com/openssh/openssh-portable/blob/master/configure.ac#L218)

The fix applies to both the umac-64* and umac-128* processing paths,
though when I was testing it, it was not triggered for umac-64*.

BR,
Peter

-- 
You are receiving this mail because:
You are watching the assignee of the bug.