Author: JF <jf@codingfield.com>
Fix buffer overflow opportunities in AlertNotificationService & AlertNotificationClient.
src/Components/Ble/AlertNotificationClient.cpp | 25 ++++++++++--- src/Components/Ble/AlertNotificationService.cpp | 34 ++++++++++-------- src/Components/Ble/NotificationManager.cpp | 7 ++- src/Components/Ble/NotificationManager.h | 4 +-
diff --git a/src/Components/Ble/AlertNotificationClient.cpp b/src/Components/Ble/AlertNotificationClient.cpp index bf4d851ca23630b606accbcf0194fe4513983b88..b65e019da5e5e403c6e8a43f238cad133d551e08 100644 --- a/src/Components/Ble/AlertNotificationClient.cpp +++ b/src/Components/Ble/AlertNotificationClient.cpp @@ -105,14 +105,25 @@ } void AlertNotificationClient::OnNotification(ble_gap_event *event) { if(event->notify_rx.attr_handle == newAlertHandle) { - size_t notifSize = OS_MBUF_PKTLEN(event->notify_rx.om); - uint8_t data[notifSize + 1]; - data[notifSize] = '\0'; - os_mbuf_copydata(event->notify_rx.om, 0, notifSize, data); - char *s = (char *) &data[2]; - NRF_LOG_INFO("DATA : %s", s); + // TODO implement this with more memory safety (and constexpr) + static const size_t maxBufferSize{21}; + static const size_t maxMessageSize{18}; + size_t bufferSize = min(OS_MBUF_PKTLEN(event->notify_rx.om), maxBufferSize); + + uint8_t data[bufferSize]; + os_mbuf_copydata(event->notify_rx.om, 0, bufferSize, data); + + char *s = (char *) &data[3]; + auto messageSize = min(maxMessageSize, (bufferSize-3)); + + for (int i = 0; i < messageSize-1; i++) { + if (s[i] == 0x00) { + s[i] = 0x0A; + } + } + s[messageSize-1] = '\0'; - notificationManager.Push(Pinetime::Controllers::NotificationManager::Categories::SimpleAlert, s, notifSize + 1); + notificationManager.Push(Pinetime::Controllers::NotificationManager::Categories::SimpleAlert, s, messageSize); systemTask.PushMessage(Pinetime::System::SystemTask::Messages::OnNewNotification); } } diff --git a/src/Components/Ble/AlertNotificationService.cpp b/src/Components/Ble/AlertNotificationService.cpp index fd69bda316259d3bb2556de7c482c370a9f81ff4..0faa17c806ac6482cb0a2871be19fcc11021af86 100644 --- a/src/Components/Ble/AlertNotificationService.cpp +++ b/src/Components/Ble/AlertNotificationService.cpp @@ -4,6 +4,7 @@ #include "NotificationManager.h" #include <SystemTask/SystemTask.h> #include "AlertNotificationService.h" +#include <cstring> using namespace Pinetime::Controllers; @@ -55,23 +56,26 @@ int AlertNotificationService::OnAlert(uint16_t conn_handle, uint16_t attr_handle, struct ble_gatt_access_ctxt *ctxt) { if (ctxt->op == BLE_GATT_ACCESS_OP_WRITE_CHR) { - size_t notifSize = OS_MBUF_PKTLEN(ctxt->om); - uint8_t data[notifSize + 1]; - data[notifSize] = '\0'; - os_mbuf_copydata(ctxt->om, 0, notifSize, data); - char *s = (char *) &data[3]; - NRF_LOG_INFO("DATA : %s", s); + // TODO implement this with more memory safety (and constexpr) + static const size_t maxBufferSize{21}; + static const size_t maxMessageSize{18}; + size_t bufferSize = min(OS_MBUF_PKTLEN(ctxt->om), maxBufferSize); + + uint8_t data[bufferSize]; + os_mbuf_copydata(ctxt->om, 0, bufferSize, data); + + char *s = (char *) &data[3]; + auto messageSize = min(maxMessageSize, (bufferSize-3)); - for(int i = 0; i <= notifSize; i++) - { - if(s[i] == 0x00) - { - s[i] = 0x0A; - } - } + for (int i = 0; i < messageSize-1; i++) { + if (s[i] == 0x00) { + s[i] = 0x0A; + } + } + s[messageSize-1] = '\0'; - m_notificationManager.Push(Pinetime::Controllers::NotificationManager::Categories::SimpleAlert, s, notifSize + 1); - m_systemTask.PushMessage(Pinetime::System::SystemTask::Messages::OnNewNotification); + m_notificationManager.Push(Pinetime::Controllers::NotificationManager::Categories::SimpleAlert, s, messageSize); + m_systemTask.PushMessage(Pinetime::System::SystemTask::Messages::OnNewNotification); } return 0; } diff --git a/src/Components/Ble/NotificationManager.cpp b/src/Components/Ble/NotificationManager.cpp index 2e02cb158895fc71fbc87fb92561f47cb85d3e07..0aea0697358482a966e6c158e525751c9c4bc156 100644 --- a/src/Components/Ble/NotificationManager.cpp +++ b/src/Components/Ble/NotificationManager.cpp @@ -4,11 +4,12 @@ using namespace Pinetime::Controllers; void NotificationManager::Push(Pinetime::Controllers::NotificationManager::Categories category, - const char *message, uint8_t messageSize) { + const char *message, uint8_t currentMessageSize) { // TODO handle edge cases on read/write index + auto checkedSize = std::min(currentMessageSize, uint8_t{18}); auto& notif = notifications[writeIndex]; - std::memcpy(notif.message.data(), message, messageSize); - notif.message[messageSize] = '\0'; + std::memcpy(notif.message.data(), message, checkedSize); + notif.message[checkedSize] = '\0'; notif.category = category; writeIndex = (writeIndex + 1 < TotalNbNotifications) ? writeIndex + 1 : 0; diff --git a/src/Components/Ble/NotificationManager.h b/src/Components/Ble/NotificationManager.h index 8edd6828c76122457eaf8d8d81769483eb618654..daa1571b3d6feeafcc80287854d265b9ab9b5c4b 100644 --- a/src/Components/Ble/NotificationManager.h +++ b/src/Components/Ble/NotificationManager.h @@ -7,10 +7,10 @@ namespace Controllers { class NotificationManager { public: enum class Categories {Unknown, SimpleAlert, Email, News, IncomingCall, MissedCall, Sms, VoiceMail, Schedule, HighProriotyAlert, InstantMessage }; - static constexpr uint8_t MessageSize = 18; + static constexpr uint8_t MessageSize{18}; struct Notification { - std::array<char, MessageSize> message; + std::array<char, MessageSize+1> message; Categories category = Categories::Unknown; };