Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

8 bytes written into heap buffer of size 4 #11421

Closed
mstyura opened this issue Jun 10, 2023 · 4 comments
Closed

8 bytes written into heap buffer of size 4 #11421

mstyura opened this issue Jun 10, 2023 · 4 comments
Assignees
@htcgh
Labels
api: analytics

Comments

@mstyura
Copy link

mstyura commented Jun 10, 2023

Description

TLDR: 4 bytes buffer allocated from apmpb_copy_measurement_bundle + 1900 is being written 8 bytes using nano_set_uint32 from APMUpdateConsentSignalsAndIdentifiers + 236

Full description:
Here is more detailed steps how corruption happen:

  1. A buffer of size 4 bytes allocated by apmpb_copy_measurement_bundle + 1900:
apmpb_copy_measurement_bundle:
...
: mov w0, #0x4
: bl 0x10e20a894 ; symbol stub for: malloc
...

backtrace of 4 bytes block allocation:

register read --format hex x0
x0 = 0x0000000000000004 # malloc size
bt
thread #28, queue = 'com.google.fira.worker', stop reason = breakpoint 6.1
frame #0: App`apmpb_copy_measurement_bundle + 1904
frame #1: App`-[APMPBMeasurementBundle initWithMessageInfo:] + 96
frame #2: App`-[APMPBMeasurementBundle initWithBuffer:] + 120
frame #3: App`APMBundleRowIDSinglePair + 272
frame #4: App`-[APMDatabase queryBundlesWithCountLimit:sizeLimit:error:] + 252
frame #5: App`-[APMMeasurement uploadData] + 264
frame #6: App`__32-[APMMeasurement updateSchedule]_block_invoke_2 + 28
frame #7: App`__51-[APMScheduler scheduleOnWorkerQueueBlockID:block:]_block_invoke + 44
frame #8: libdispatch.dylib`_dispatch_call_block_and_release + 24
frame #9: libdispatch.dylib`_dispatch_client_callout + 16
frame #10: libdispatch.dylib`_dispatch_lane_serial_drain + 924
frame #11: libdispatch.dylib`_dispatch_lane_invoke + 424
frame #12: libdispatch.dylib`_dispatch_workloop_worker_thread + 1716
frame #13: libsystem_pthread.dylib`_pthread_wqthread + 284
(lldb) register read --format hex x0
x0 = 0x0000025df01125d0 # address returned by malloc

at that point write watchpoint is added for memory location $x0+4 (outside of the buffer)

  1. Then write watchpoint catch that 8 bytes are written to the buffer by nano_set_uint32 called from APMUpdateConsentSignalsAndIdentifiers + 236 via intermediate call to NANOSetInt32 + 140:
nano_set_uint32:
...
mov w8, w19
-> str x8, [x0] # write outsize of buffer range
...

backtrace of 8 bytes write to 4 byte buffer:

* thread #28, queue = 'com.google.fira.worker', stop reason = EXC_BREAKPOINT (code=258, subcode=0x25df01125d0)
* frame #0: 0x000000010961426c App`nano_set_uint32 + 48
frame #1: 0x00000001095c3944 App`APMUpdateConsentSignalsAndIdentifiers + 240
frame #2: 0x00000001095bcb60 App`__28-[APMMeasurement uploadData]_block_invoke + 84
frame #3: 0x0000000131f4d934 CoreFoundation`__NSARRAY_IS_CALLING_OUT_TO_A_BLOCK__ + 16
frame #4: 0x0000000131ed429c CoreFoundation`-[__NSArrayM enumerateObjectsWithOptions:usingBlock:] + 192
frame #5: 0x00000001095bc28c App`-[APMMeasurement uploadData] + 448
frame #6: 0x00000001095bbc5c App`__32-[APMMeasurement updateSchedule]_block_invoke_2 + 28
frame #7: 0x00000001096079b8 App`__51-[APMScheduler scheduleOnWorkerQueueBlockID:block:]_block_invoke + 44
frame #8: 0x000000014022c528 libdispatch.dylib`_dispatch_call_block_and_release + 24
frame #9: 0x000000014022dd50 libdispatch.dylib`_dispatch_client_callout + 16
frame #10: 0x0000000140236014 libdispatch.dylib`_dispatch_lane_serial_drain + 924
frame #11: 0x0000000140236d6c libdispatch.dylib`_dispatch_lane_invoke + 424
frame #12: 0x0000000140244b74 libdispatch.dylib`_dispatch_workloop_worker_thread + 1716
frame #13: 0x00000001b1834878 libsystem_pthread.dylib`_pthread_wqthread + 284

The suspicious thing is that nano_set_uint32 could allocate buffer by itself, but it uses 8 bytes as buffer size, so it is likely that provided buffer must be at least 8 bytes:

nano_set_uint32:
...
mov w0, #0x8
bl 0x10daca894 ; symbol stub for: malloc
...

It is interesting why function which has uint32 in name does allocate 8 bytes, instead of intuitevely guessable 4 bytes.
It also interesting why it does 8 bytes-wide write while function name suggest 32-bit wide argument.
Here is the disassembly of provided by lldb:

App`nano_set_uint32:
0x10961423c : cbz x0, 0x10961427c ;
0x109614240 : stp x20, x19, [sp, #-0x20]!
0x109614244 : stp x29, x30, [sp, #0x10]
0x109614248 : add x29, sp, #0x10
0x10961424c : mov x19, x1 # note: second func argument seems to be 64-bit integer
0x109614250 : mov x20, x0 # note: first func argument seems to be pointer
0x109614254 : ldr x0, [x0]
0x109614258 : cbnz x0, 0x109614268 ;
0x10961425c : mov w0, #0x8
0x109614260 : bl 0x10a2fe894 ; symbol stub for: malloc
0x109614264 : str x0, [x20]
0x109614268 : mov w8, w19 # note: looks like implementation of truncation of 64-bit integer down to 32-bit integer with zero-extension (https://devblogs.microsoft.com/oldnewthing/20220805-00/)
-> 0x10961426c : str x8, [x0] # note: 8 bytes write to [$x0] happen here
0x109614270 : ldp x29, x30, [sp, #0x10]
0x109614274 : ldp x20, x19, [sp], #0x20
0x109614278 : ret
0x10961427c : adrp x0, 4328
0x109614280 : add x0, x0, #0x7ee ; " Unable to set uint32. Destination pointer is NULL"
0x109614284 : b 0x10a2fef18 ; symbol stub for: puts

Using godbolt.org I've reconstructed the possible definitioon of nano_set_uint32 in C language:

void nano_set_uint32(uint64_t **ptr /*ptr to 64-bit wide data*/, int64_t value /*64-bit wide data*/) {
    if (NULL == ptr) {
        puts(" Unable to set uint32. Destination pointer is NULL");
        return;
    }
    if  (NULL == *ptr) {
        *ptr = (uint64_t*)malloc(sizeof(uint64_t));
    }
    **ptr = (uint32_t)value;
}

such that generated assembly by clang 16 with -O2 optimization produces almost the same assembly with the difference in one instruction which I believe effectively does the same thing - 64-bit to 32-bit integer truncation:

mov w8, w19 # instruction from Firebase binary
and x8, x19, #0xffffffff # instruction from Godbolt of re-constructed function

There is obviously something wrong here, either apmpb_copy_measurement_bundle should allocate larger buffer or nano_set_uint32 should not do 8 bytes write.
It also seems that somewhere (silent?) conversion from uint32_t* to uint64_t* done, leading to buffer overflow later.
Could you please inspect related part of Firebase SDK source code and fix buffer overflow problem?

This issue is probably harmful due to smallest malloc block allocated on iOS is actually 16 bytes, but this overrun mess with runtime diagnostic tools, in my case mimalloc with debug checks enabled.

Reproducing the issue

Use Firebase SDK in such a way that it uploads data to it's server.

Firebase SDK Version

10.10.0

Xcode Version

14.3.1

Installation Method

Swift Package Manager

Firebase Product(s)

All

Targeted Platforms

iOS

Relevant Log Output

No response

If using Swift Package Manager, the project's Package.resolved

No response

If using CocoaPods, the project's Podfile.lock

No response
@google-oss-bot
Copy link

I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.

@mstyura
Copy link
Author

mstyura commented Jun 27, 2023 •
edited

Hello, @paulb777 ! I was wondering if there might be an update available regarding the resolution of this issue. Do you need any additional information to facilitate the investigation and rectification of the bug? Thank you for your time and effort.

@htcgh
Copy link
Member

htcgh commented Jun 28, 2023

Thank you @mstyura for the thorough report. It'll take some time for us to look into this issue and fully test a fix. Unfortunately, the fix won't make it into our July release.

@htcgh
Copy link
Member

htcgh commented Aug 1, 2023

Firebase 10.13.0 includes the fix for this. Please reopen this ticket if the issue still occurs in 10.13.0 or later.

@htcgh htcgh closed this as completed Aug 1, 2023
@firebase firebase locked and limited conversation to collaborators Sep 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@htcgh htcgh

Labels
api: analytics
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@paulb777 @mstyura @htcgh @google-oss-bot