1180096 - UBSAN store to misaligned address 0x000003032f4e for type 'HALF'

Status: RESOLVED WONTFIX

(NSS :: Libraries, defect, P2)

Description

[Tyson Smith [:tsmith]]

This was found by running the test suite with an Undefined Behavior Sanitizer (UBSAN) build.

des.c:650:5: runtime error: store to misaligned address 0x000003032f4e for type 'HALF' (aka 'unsigned int'), which requires 4 byte alignment
0x000003032f4e: note: pointer points here
 dd b3 68 5b 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00
             ^
#0 0x7fac21f3554d in DES_Do1Block /home/user/Desktop/nss_ubsan/nss/lib/freebl/des.c:650 (discriminator 2)
#1 0x7fac21f26c52 in DES_ECB /home/user/Desktop/nss_ubsan/nss/lib/freebl/desblapi.c:55
#2 0x7fac21f29808 in DES_Encrypt /home/user/Desktop/nss_ubsan/nss/lib/freebl/desblapi.c:252
#3 0x5917d4 in DES_Encrypt /home/user/Desktop/nss_ubsan/nss/lib/freebl/loader.c:459
#4 0x428a35 in des_Encrypt /home/user/Desktop/nss_ubsan/nss/cmd/bltest/blapitest.c:1064
#5 0x44924b in cipherDoOp /home/user/Desktop/nss_ubsan/nss/cmd/bltest/blapitest.c:2334
#6 0x4633ec in blapi_selftest /home/user/Desktop/nss_ubsan/nss/cmd/bltest/blapitest.c:3068
#7 0x458018 in main /home/user/Desktop/nss_ubsan/nss/cmd/bltest/blapitest.c:3610
#8 0x7fac22d0eec4 in __libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:287
#9 0x405f10 in _start ??:?

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Mass cc to get some NSS eyes on these bugs.

Comment 2

[Tyson Smith [:tsmith]]

Found this issue in a few more places:

des.c:576:13: runtime error: load of misaligned address 0x000003875f51 for type 'HALF' (aka 'unsigned int'), which requires 4 byte alignment
des.c:577:13: runtime error: load of misaligned address 0x000003875f55 for type 'HALF' (aka 'unsigned int'), which requires 4 byte alignment
des.c:649:5: runtime error: store to misaligned address 0x000001f1cf69 for type 'HALF' (aka 'unsigned int'), which requires 4 byte alignment
des.c:650:5: runtime error: store to misaligned address 0x000001f1cf6d for type 'HALF' (aka 'unsigned int'), which requires 4 byte alignment

Comment 3

[Ryan Sleevi]

Fairly sure this is legit. wtc had to do a pass on the RC4 code to make the *SAN tools happy for similar reasons. This is the 'undefined but generally leads to slower execution' issue, but I don't think represents a security bug, just a correctness bug.

Comment 4

[Wan-Teh Chang]

Ryan: the RC4 issue was different. The RC4 code used to read beyond the end of
the input buffer, but each read is aligned.

The DES code intentionally does unaligned reads and writes on processor
architectures that we know automatically fix up unaligned memory access,
such as x86 and x64. This is why all the errors UBSAN reported are inside
#if defined(NSS_X86_OR_X64) blocks.

So I think we can mark this bug WONTFIX.

Comment 5

[Tyson Smith [:tsmith]]

(In reply to Wan-Teh Chang from comment #4)

> The DES code intentionally does unaligned reads and writes on processor
> architectures that we know automatically fix up unaligned memory access

Should comments be added to explain this?

Comment 6

[Wan-Teh Chang]

Adding comments is a good idea. In fact, I was surprised to
find no comments when I searched for "align" in that file.
It's obvious to someone familiar with unaligned memory
access:

#if defined(NSS_X86_OR_X64)
    left  = HALFPTR(key)[0];
    right = HALFPTR(key)[1];
    BYTESWAP(left, temp);
    BYTESWAP(right, temp);
#else
    if (((ptrdiff_t)key & 0x03) == 0) {
        left  = HALFPTR(key)[0];
        right = HALFPTR(key)[1];
#if defined(IS_LITTLE_ENDIAN)
        BYTESWAP(left, temp);
        BYTESWAP(right, temp);
#endif
    } else {
        left    = ((HALF)key[0] << 24) | ((HALF)key[1] << 16) |
                  ((HALF)key[2] << 8)  | key[3];
        right   = ((HALF)key[4] << 24) | ((HALF)key[5] << 16) |
                  ((HALF)key[6] << 8)  | key[7];
    }
#endif

The NSS_X86_OR_X64 code is the same as the code in the the 4-byte
aligned case. So someone experienced in this area will immediately
know why the NSS_X86_OR_X64 code doesn't check for alignment.

Do you think these comments clarify it?

diff --git a/lib/freebl/des.c b/lib/freebl/des.c
--- a/lib/freebl/des.c
+++ b/lib/freebl/des.c
@@ -417,29 +417,33 @@ DES_MakeSchedule( HALF * ks, const BYTE 
 {
     register HALF left, right;
     register HALF c0, d0;
     register HALF temp;
     int           delta;
     unsigned int  ls;
 
 #if defined(NSS_X86_OR_X64)
+    /* These processors automatically fix up unaligned memory access. So they
+     * can simply use the code used for the 4-byte aligned case below. */
     left  = HALFPTR(key)[0]; 
     right = HALFPTR(key)[1]; 
     BYTESWAP(left, temp);
     BYTESWAP(right, temp);
 #else
     if (((ptrdiff_t)key & 0x03) == 0) {
+        /* If |key| is a 4-byte aligned address, read a HALF at a time. */
 	left  = HALFPTR(key)[0]; 
 	right = HALFPTR(key)[1]; 
 #if defined(IS_LITTLE_ENDIAN)
 	BYTESWAP(left, temp);
 	BYTESWAP(right, temp);
 #endif
     } else {
+        /* Otherwise, read a BYTE at a time. */
 	left    = ((HALF)key[0] << 24) | ((HALF)key[1] << 16) | 
 		  ((HALF)key[2] << 8)  | key[3];
 	right   = ((HALF)key[4] << 24) | ((HALF)key[5] << 16) | 
 		  ((HALF)key[6] << 8)  | key[7];
     }
 #endif
 
     PC1(left, right, c0, d0, temp);

Comment 7

[Wan-Teh Chang]

Attachment: Add comments, and add the feature-test macro HAVE_UNALIGNED_ACCESS

I decided to also add the feature-test macro HAVE_UNALIGNED_ACCESS
to make the code more self-explanatory.

Since HAVE_UNALIGNED_ACCESS is unrelated to the endianness, I had
to add #if defined(IS_LITTLE_ENDIAN) tests.

Comment 8

[Tyson Smith [:tsmith]]

Comment on attachment 8662586 [details] [diff] [review]
Add comments, and add the feature-test macro HAVE_UNALIGNED_ACCESS

Review of attachment 8662586 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah that looks good to me.

Comment 9

[Wan-Teh Chang]

Comment on attachment 8662586 [details] [diff] [review]
Add comments, and add the feature-test macro HAVE_UNALIGNED_ACCESS

Review of attachment 8662586 [details] [diff] [review]:
-----------------------------------------------------------------

https://hg.mozilla.org/projects/nss/rev/8e7a71347706

Comment 10

[Wan-Teh Chang]

Marked the bug WONTFIX. Added comments to document the intentional
unaligned memory access. Surrounded the unaligned memory access with
the HAVE_UNALIGNED_ACCESS feature tests, which have a self-explanatory
name and lead to the comments that explain why the unaligned memory
access is OK.