1243724 - firefox 44 crashes due to depending on undefined behavior with memcpy()

Status: RESOLVED DUPLICATE of bug 1224291

(Core :: Networking: HTTP, defect)

Description

[Natanael Copa]

Attachment: firefox-44.0-brotli-backtrace.txt

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/47.0.2526.106 Safari/537.36

Steps to reproduce:

built firefox 44.0 on x86_64 alpine linux (which uses musl libc) and opened http://streambuilder.pro/docs/how-tos/check-iqsv/


Actual results:

fortify check caught bad use of memcpy() and crashed.

This does not happen with firefox-43.

What happens is that brotli is built without BROTLI_BUILD_PORTABLE by default.

from: modules/brotli/dec/port.h:
#ifdef BROTLI_BUILD_PORTABLE
#define BROTLI_ALIGNED_READ 1
#define BROTLI_SAFE_MEMMOVE 1
#else
#define BROTLI_ALIGNED_READ 0
#define BROTLI_SAFE_MEMMOVE 0
#endif

So we get BROTLI_SAFE_MEMMOVE=0. This will select the following code in 
modules/brotli/dec/decode.c:

static BROTLI_INLINE BROTLI_NO_ASAN void memmove16(
    uint8_t* dst, uint8_t* src) {
#if BROTLI_SAFE_MEMMOVE
  /* For x86 this compiles to the same binary as signle memcpy.
     On ARM memcpy is not inlined, so it works slower.
     This implementation makes decompression 1% slower than regular one,
     and 2% slower than NEON implementation.
   */
  uint32_t buffer[4];
  memcpy(buffer, src, 16);
  memcpy(dst, buffer, 16);
#elif defined(__ARM_NEON__)
  vst1q_u8(dst, vld1q_u8(src));
#else
  /* memcpy is unsafe for overlapping regions and ASAN detects this.
     But, because of optimizations, it works exactly as memmove:
     copies data to registers first, and then stores them to dst. */
  memcpy(dst, src, 16);
#endif
}



and we end up using memcpy() in a way that depends on how implementation deals with overlapping memory regions. FORTIFY_SOURCE on alpine linux did not like this and went boom.




Expected results:

pages should have open without crashing, regardless of how memcpy() is implemented.

Comment 1

[Natanael Copa]

I think it would make sense to build brotli with BROTLI_BUILD_PORTABLE by default, and possibly have a whitelist of platforms where you know for sure that it can be disabled.

Comment 2

[Frederik Braun [:freddy]]

Moving to Core: Networking and marking this crash blocking the brotli implementation.

Comment 3

[Natanael Copa]

Credit to musl libc developer Szabolcs Nagy, who helped analyze the disassembly.

Comment 4

[Jonathan Kew [:jfkthame]]

This is basically the same report as bug 1224291, except that there it was reported by ASAN.

I guess we should go ahead and take the patch in that bug, to avoid this issue.

Comment 5

[Eugene Kliuchnikov]

Fixed in upstream