Hi,
On Tue, Apr 19, 2016 at 9:01 PM, Jens <openvpn-de...@neuhalfen.name> wrote:
> The AUTH-PAM plugin contains the function `searchandreplace`. The buffer
> allocated there can be overflown if the parameter `replace_with` is to long
> (depending on the format string).
>
> E.g.:
>
> searchandreplace(to search :=“X”, searchfor := “X”, replacewith :=
> “0123456789!”)
>
> would overflow `temp` with the character ‘!’.
Good find!
> --- a/src/plugins/auth-pam/auth-pam.c
> +++ b/src/plugins/auth-pam/auth-pam.c
> @@ -123,13 +123,15 @@ static void pam_server (int fd, const char *service,
> int verb, const struct name
> static char *
> searchandreplace(const char *tosearch, const char *searchfor, const char
> *replacewith)
> {
> + if (!tosearch || !searchfor || !replacewith) return 0;
> + if (!strlen(tosearch) || !strlen(searchfor) || !strlen(replacewith))
> return 0;
> +
> const char *searching=tosearch;
> char *scratch;
> - char temp[strlen(tosearch)*10];
> +
> + char temp[strlen(tosearch) * strlen(replacewith) + 1];
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
SIZE_MAX/10. While we're fixing this code, let's fix that too. For
example:
size_t tosearchlen = strlen(tosearch);
size_t replacewithlen = strlen(replacewith);
size_t templen = tosearchlen * replacewithlen;
if ((tosearchlen != 0 && templen / tosearchlen != replacewithlen)
|| templen == SIZE_MAX) {
return NULL;
}
char temp[templen+1];
-Steffan
