[PATCH] msvcrt: fix character/byte confusion in buffer overflow branch
The first memcpy() call in puts_clbk_str_w() confuses character count and byte count. It uses the number of characters (out->len) as number of bytes. This leaves half of the buffer undefined. Interestingly, the second memcpy() call in the same function is correct. This bug potentially makes applications expose internal (secret) data. Usually, the destination buffer is on the stack, and the stack often contains secrets. Therefore, one could argue that this bug constitutes a security vulnerability. --- dlls/msvcrt/printf.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/msvcrt/printf.h b/dlls/msvcrt/printf.h index cfba4b7..8b749bc 100644 --- a/dlls/msvcrt/printf.h +++ b/dlls/msvcrt/printf.h @@ -48,7 +48,7 @@ static int FUNC_NAME(puts_clbk_str)(void *ctx, int len, const APICHAR *str) return len; if(out->len < len) { - memcpy(out->buf, str, out->len); + memcpy(out->buf, str, out->len*sizeof(APICHAR)); out->buf += out->len; out->len = 0; return -1;
In general, I think you want to send this to wine-patches, not here. On Mon, May 6, 2013 at 12:26 PM, Max Kellermann <max(a)duempel.org> wrote:
The first memcpy() call in puts_clbk_str_w() confuses character count and byte count. It uses the number of characters (out->len) as number of bytes. This leaves half of the buffer undefined.
Interestingly, the second memcpy() call in the same function is correct.
This bug potentially makes applications expose internal (secret) data. Usually, the destination buffer is on the stack, and the stack often contains secrets. Therefore, one could argue that this bug constitutes a security vulnerability.
It'd be hard to make that argument convincingly. That's neither here nor there, I suppose, but... ---
dlls/msvcrt/printf.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/msvcrt/printf.h b/dlls/msvcrt/printf.h index cfba4b7..8b749bc 100644 --- a/dlls/msvcrt/printf.h +++ b/dlls/msvcrt/printf.h @@ -48,7 +48,7 @@ static int FUNC_NAME(puts_clbk_str)(void *ctx, int len, const APICHAR *str) return len;
if(out->len < len) { - memcpy(out->buf, str, out->len); + memcpy(out->buf, str, out->len*sizeof(APICHAR)); out->buf += out->len;
If the memcpy was incorrect, the += is also incorrect. I'm not sure which is the case, but either way, your patch can't be correct as is. --Juan
On 2013/05/07 17:46, Juan Lang <juan.lang(a)gmail.com> wrote:
In general, I think you want to send this to wine-patches, not here.
True, I resent it to wine-patches yesterday already, when I noticed that.
if(out->len < len) { - memcpy(out->buf, str, out->len); + memcpy(out->buf, str, out->len*sizeof(APICHAR)); out->buf += out->len;
If the memcpy was incorrect, the += is also incorrect. I'm not sure which is the case, but either way, your patch can't be correct as is.
Sure? out->buf is an "APICHAR*" (see printf.h), and adding out->len advances the pointer by "out->len * sizeof(APICHAR)" bytes. Am I missing something? Max
On 05/07/13 17:46, Juan Lang wrote:
In general, I think you want to send this to wine-patches, not here. This patch was also sent to wine-patches.
On Mon, May 6, 2013 at 12:26 PM, Max Kellermann <max(a)duempel.org <mailto:max(a)duempel.org>> wrote:
--- dlls/msvcrt/printf.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/msvcrt/printf.h b/dlls/msvcrt/printf.h index cfba4b7..8b749bc 100644 --- a/dlls/msvcrt/printf.h +++ b/dlls/msvcrt/printf.h @@ -48,7 +48,7 @@ static int FUNC_NAME(puts_clbk_str)(void *ctx, int len, const APICHAR *str) return len;
if(out->len < len) { - memcpy(out->buf, str, out->len); + memcpy(out->buf, str, out->len*sizeof(APICHAR)); out->buf += out->len;
If the memcpy was incorrect, the += is also incorrect. I'm not sure which is the case, but either way, your patch can't be correct as is. out->buf is of APICHAR* type, so it's updated correctly. The patch looks good for me.
On Tue, May 7, 2013 at 9:10 AM, Piotr Caban <piotr.caban(a)gmail.com> wrote:
On 05/07/13 17:46, Juan Lang wrote:
In general, I think you want to send this to wine-patches, not here.
This patch was also sent to wine-patches.
On Mon, May 6, 2013 at 12:26 PM, Max Kellermann <max(a)duempel.org
<mailto:max(a)duempel.org>> wrote:
--- dlls/msvcrt/printf.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/msvcrt/printf.h b/dlls/msvcrt/printf.h index cfba4b7..8b749bc 100644 --- a/dlls/msvcrt/printf.h +++ b/dlls/msvcrt/printf.h @@ -48,7 +48,7 @@ static int FUNC_NAME(puts_clbk_str)(void *ctx, int len, const APICHAR *str) return len;
if(out->len < len) { - memcpy(out->buf, str, out->len); + memcpy(out->buf, str, out->len*sizeof(APICHAR)); out->buf += out->len;
If the memcpy was incorrect, the += is also incorrect. I'm not sure which is the case, but either way, your patch can't be correct as is.
out->buf is of APICHAR* type, so it's updated correctly. The patch looks good for me.
Thanks, Piotr. Max, my apologies for the sloppy review. --Juan
participants (3)
-
Juan Lang -
Max Kellermann -
Piotr Caban