Android: Stack Memory Corruption in BnBluetoothGattServer and BnBluetoothGatServerCallback IPC
Platform: Based on current master in AOSP
Class: Elevation of Privilege

This is in pre-release code and might not actually be vulnerable on a real device. While it’s probably only available in Brillo atm there are indications that it might become the default BT stack on later versions of Android so fixing it now would be good. I’ve not been able to test it directly but I have verified the code is vulnerable by building a copy for another device. 

Summary:
The SEND_RESPONSE_TRANSACTION and SEND_NOTIFICATION_TRANSACTION IPC calls in BnBluetoothGattServer::onTransact are vulnerable to stack corruption which could allow an attacker to locally elevate privileges to the level of the bluetooth service. 

Description:
The system/bt/service/common/bluetooth/binder/IBluetoothGattServer.cpp file which is part of a new Bluetooth stack for Brillo contains a binder service which has SEND_RESPONSE_TRANSACTION and SEND_NOTIFICATION_TRANSACTION calls. The handlers for these calls have a vulnerability where it’s possible to move the stack pointer out of bounds and get selective memory corruption on the stack. Note that BnBluetoothGattServerCallback also has similar code patterns in its ON_CHARACTERISTIC_WRITE_REQUEST_TRANSACTION and ON_DESCRIPTOR_WRITE_REQUEST_TRANSACTION calls.

The code pattern is the following:

std::vector<uint8_t> value;
int value_len = data.readInt32(); <- Read in an int32 from the Binder, attacker controlled.
if (value_len != -1) {            <- Only check for -1
   uint8_t bytes[value_len];      <- Will adjust stack by value_len (plus stack alignment)
   data.read(bytes, value_len);   <- Will corrupt here
   value.insert(value.begin(), bytes, bytes + value_len);
}

The compiler could in theory make a check here to ensure the stack movement is valid (Visual C++ on Windows does for example) but looking at a compiled version of the code it seems to be blindly trusting the value.

.text:0000E756                 BLX             _ZNK7android6Parcel9readInt32Ev ; android::Parcel::readInt32(void)
.text:0000E75A                 MOV             R4, R0
.text:0000E75C value_len = R4                          ; int
.text:0000E75C                 CMP.W           value_len, #0xFFFFFFFF <- Compare to -1
.text:0000E760                 BEQ             loc_E792
.text:0000E762                 ADDS            R0, value_len, #7
.text:0000E764                 STR.W           SP, [R6,#8] <- Store old stack value
.text:0000E768                 BIC.W           R0, R0, #7 <- Align stack value
.text:0000E76C                 SUB.W           R9, SP, R0 <- Subtract untrusted value from stack
.text:0000E770                 MOV             SP, R9  <- Replace stack
.text:0000E772                 MOV             R0, data ; this
.text:0000E774                 MOV             R1, R9  ; void *
.text:0000E776                 MOV             R2, value_len ; unsigned int
.text:0000E778                 BLX             _ZNK7android6Parcel4readEPvj ; android::Parcel::read(void *,uint)
.text:0000E77C                 LDR.W           R1, [R8,#4] ; __position
.text:0000E780                 ADD.W           R3, R9, value_len ; __last
.text:0000E784                 ADD.W           R0, R6, #0xB4 ; this
.text:0000E788                 MOV             R2, R9  ; __first 
.text:0000E78A                 BL              _ZNSt3__16vectorIhNS_9allocatorIhEEE6insertIPhEENS_9enable_ifIXaasr21__is_forward_iteratorIT_EE5valuesr16is_constructibleIhNS_15iterator_traitsIS7_E9referenceEEE5valueENS_11__wrap_iterIS5_EEE4typeENSB_IPKhEES7_S7_
.text:0000E78E                 LDR.W           SP, [R6,#8] <- Restore original stack

This can be exploited in two ways, either specifying a negative value which means the stack will move up and any writes to the stack (say in the prolog of Parcel::read) will corrupt an earlier frame, or you can move the stack down past the base of the stack and potentially corrupt heap or other memory below the stack. It might not be possible to control the exact data being written through Parcel::read but it’s likely that control over written registers would be possible. This would bypass stack cookies as the move could be aligned specifically to jump over the cookie and corrupt a stored PC or register.

There’s no real need to use this code pattern when the vector has already been allocated. Instead you could use &value[0] to get a pointer to a mutable buffer to pass to Parcel::read. And the value should be checked for < 0, and the result of Parcel::read should be checked.

I’ve not provided a PoC as I don’t have a working system to test the code on. 

This bug is subject to a 90 day disclosure deadline. If 90 days elapse without a broadly available patch, then the bug report will automatically become visible to the public.