Hi Steffan,
[…]
>
> This is without a doubt an improvement, but it still leaves an
> opportunity open to achieve a buffer overflow through an integer
> overflow. Consider a tosearch with len 11, and a replacewith with len
[…]
Good point. Patch attached.
Cheers
Jens
From 7d76d224096d26a6d1933856e03f5c42387d2f31 Mon Sep 17 00:00:00 2001
From: Jens Neuhalfen <j...@neuhalfen.name>
List-Post: openvpn-devel@lists.sourceforge.net
Date: Tue, 19 Apr 2016 20:42:55 +0200
Subject: [PATCH] Fix buffer overflow by user supplied data
Passing very long usernames/passwords for pam authentication could possibly
lead to a stack based buffer overrun in the auth-pam plugin.
Adds a dependency to C99 (includes stdbool.h)
Signed-off-by: Jens Neuhalfen <j...@neuhalfen.name>
---
src/plugins/auth-pam/auth-pam.c | 29 +++++++++++++++++++++++++----
1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-pam.c
index 95692ab..159fd07 100644
--- a/src/plugins/auth-pam/auth-pam.c
+++ b/src/plugins/auth-pam/auth-pam.c
@@ -39,6 +39,7 @@
#include <stdio.h>
#include <string.h>
#include <ctype.h>
+#include <stdbool.h>
#include <unistd.h>
#include <stdlib.h>
#include <sys/types.h>
@@ -119,17 +120,37 @@ static void pam_server (int fd, const char *service, int
verb, const struct name
* a pointer to the NEW string. Does not modify the input strings. Will not
enter an
* infinite loop with clever 'searchfor' and 'replacewith' strings.
* Daniel Johnson - progman2...@usa.net / djohn...@progman.us
+ *
+ * Retuns NULL when
+ * - any parameter is NULL
+ * - the worst-case result is to large ( >= SIZE_MAX)
*/
static char *
searchandreplace(const char *tosearch, const char *searchfor, const char
*replacewith)
{
+ if (!tosearch || !searchfor || !replacewith) return NULL;
+
+ size_t tosearchlen = strlen(tosearch);
+ size_t replacewithlen = strlen(replacewith);
+ size_t templen = tosearchlen * replacewithlen;
+
+ if (tosearchlen == 0 || strlen(searchfor) == 0 || replacewithlen == 0) {
+ return NULL;
+ }
+
+ bool is_potential_integer_overflow = (templen == SIZE_MAX) || (templen /
tosearchlen != replacewithlen);
+
+ if (is_potential_integer_overflow) {
+ return NULL;
+ }
+
+ // state: all parameters are valid
+
const char *searching=tosearch;
char *scratch;
- char temp[strlen(tosearch)*10];
- temp[0]=0;
- if (!tosearch || !searchfor || !replacewith) return 0;
- if (!strlen(tosearch) || !strlen(searchfor) || !strlen(replacewith)) return
0;
+ char temp[templen+1];
+ temp[0]=0;
scratch = strstr(searching,searchfor);
if (!scratch) return strdup(tosearch);
--
2.6.4 (Apple Git-63)
>
> -Steffan
