[2/2] smsutil: check status report fits in buffer

Commit Message

Sicelo Dec. 4, 2024, 8:18 a.m. UTC

Fixes CVE-2023-4232
---
 src/smsutil.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Marcel Holtmann Dec. 4, 2024, 9:55 a.m. UTC | #1

Hi Sicelo,

> Fixes CVE-2023-4232
> ---
> src/smsutil.c | 3 +++
> 1 file changed, 3 insertions(+)
> 
> diff --git a/src/smsutil.c b/src/smsutil.c
> index bdb1d04f..8c1aaad3 100644
> --- a/src/smsutil.c
> +++ b/src/smsutil.c
> @@ -1077,6 +1077,9 @@ static gboolean decode_status_report(const unsigned char *pdu, int len,
> if ((len - offset) < expected)
> return FALSE;
> 
> + if (expected > (int)sizeof(out->status_report.ud))
> + return FALSE;
> +

every time we do casting, I would asked myself if the variable really has the right type and if casting could be avoided.

For example, what is the reason that sms_udl_in_bytes() returns int instead of size_t or unsigned int? Denis?

Regards

Marcel

Denis Kenzior Dec. 4, 2024, 6:06 p.m. UTC | #2

Hi Marcel,

On 12/4/24 3:55 AM, Marcel Holtmann wrote:
> Hi Sicelo,
> 
>> Fixes CVE-2023-4232
>> ---
>> src/smsutil.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/src/smsutil.c b/src/smsutil.c
>> index bdb1d04f..8c1aaad3 100644
>> --- a/src/smsutil.c
>> +++ b/src/smsutil.c
>> @@ -1077,6 +1077,9 @@ static gboolean decode_status_report(const unsigned char *pdu, int len,
>> if ((len - offset) < expected)
>> return FALSE;
>>
>> + if (expected > (int)sizeof(out->status_report.ud))
>> + return FALSE;
>> +
> 
> every time we do casting, I would asked myself if the variable really has the right type and if casting could be avoided.
> 
> For example, what is the reason that sms_udl_in_bytes() returns int instead of size_t or unsigned int? Denis?
> 

No good reason.  It should be returning a size_t.

Regards,
-Denis

Patch

diff --git a/src/smsutil.c b/src/smsutil.c
index bdb1d04f..8c1aaad3 100644
--- a/src/smsutil.c
+++ b/src/smsutil.c
@@ -1077,6 +1077,9 @@  static gboolean decode_status_report(const unsigned char *pdu, int len,
 		if ((len - offset) < expected)
 			return FALSE;
 
+		if (expected > (int)sizeof(out->status_report.ud))
+			return FALSE;
+
 		memcpy(out->status_report.ud, pdu + offset, expected);
 	}