From 1f540e497d69b74b729873df400d89cf4f929d9a Mon Sep 17 00:00:00 2001 From: Kraemii Date: Fri, 26 May 2023 10:34:47 +0200 Subject: [PATCH] Pointer overflow check When checking for integer overflow, you may often write tests like `p + i < p`. This works fine if `p` and `i` are unsigned integers, since any overflow in the addition will cause the value to simply "wrap around." However, using this pattern when `p` is a pointer is problematic because pointer overflow has undefined behavior according to the C and C++ standards. If the addition overflows and has an undefined result, the comparison will likewise be undefined; it may produce an unintended result, or may be deleted entirely by an optimizing compiler. In this case I removed the pointer overflow check completely and replaced it with a check of the actual pointer size, as everything outside would be a segmentation fault anyway. --- samba/auth/ntlmssp/ntlmssp_parse.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/samba/auth/ntlmssp/ntlmssp_parse.c b/samba/auth/ntlmssp/ntlmssp_parse.c index 06ae82a9..c1b57c58 100644 --- a/samba/auth/ntlmssp/ntlmssp_parse.c +++ b/samba/auth/ntlmssp/ntlmssp_parse.c @@ -233,7 +233,7 @@ BOOL msrpc_parse(TALLOC_CTX *mem_ctx, const DATA_BLOB *blob, /* if odd length and unicode */ return False; } - if (blob->data + ptr < (uint8_t *)ptr || blob->data + ptr < blob->data) + if (ptr > blob->length) return False; if (0 < len1) { @@ -263,7 +263,7 @@ BOOL msrpc_parse(TALLOC_CTX *mem_ctx, const DATA_BLOB *blob, return False; } - if (blob->data + ptr < (uint8_t *)ptr || blob->data + ptr < blob->data) + if (ptr > blob->length) return False; if (0 < len1) { @@ -293,7 +293,7 @@ BOOL msrpc_parse(TALLOC_CTX *mem_ctx, const DATA_BLOB *blob, return False; } - if (blob->data + ptr < (uint8_t *)ptr || blob->data + ptr < blob->data) + if (head_ofs > blob->length) return False; *b = data_blob_talloc(mem_ctx, blob->data + ptr, len1); @@ -304,7 +304,7 @@ BOOL msrpc_parse(TALLOC_CTX *mem_ctx, const DATA_BLOB *blob, len1 = va_arg(ap, uint_t); /* make sure its in the right format - be strict */ NEED_DATA(len1); - if (blob->data + head_ofs < (uint8_t *)head_ofs || blob->data + head_ofs < blob->data) + if (head_ofs > blob->length) return False; *b = data_blob_talloc(mem_ctx, blob->data + head_ofs, len1); @@ -318,8 +318,8 @@ BOOL msrpc_parse(TALLOC_CTX *mem_ctx, const DATA_BLOB *blob, case 'C': s = va_arg(ap, char *); - if (blob->data + head_ofs < (uint8_t *)head_ofs || blob->data + head_ofs < blob->data) - return False; + if (head_ofs > blob->length) + return False; head_ofs += pull_string(p, blob->data+head_ofs, sizeof(p), blob->length - head_ofs,