[PATCH v2 1/3] msvcrt: Verify format string before reading past end of it
From: Michał Janiszewski <janisozaur(a)gmail.com> In some cases, e.g. unterminated selection specifier (%[]) could make scanf() family of functions could keep reading from the format string past end of it. Add a check to verify when format string ends, rather than blindly expect the termination to happen. Signed-off-by: Michał Janiszewski <janisozaur(a)gmail.com> --- dlls/msvcrt/scanf.h | 2 +- dlls/msvcrt/tests/scanf.c | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/dlls/msvcrt/scanf.h b/dlls/msvcrt/scanf.h index 0903d6909a..68585468fe 100644 --- a/dlls/msvcrt/scanf.h +++ b/dlls/msvcrt/scanf.h @@ -637,7 +637,7 @@ _FUNCTION_ { while(*format && (*format != ']')) { /* According to msdn: * "Note that %[a-z] and %[z-a] are interpreted as equivalent to %[abcde...z]." */ - if((*format == '-') && (*(format + 1) != ']')) { + if((*format == '-') && *(format + 1) && (*(format + 1) != ']')) { if ((*(format - 1)) < *(format + 1)) RtlSetBits(&bitMask, *(format - 1) +1 , *(format + 1) - *(format - 1)); else diff --git a/dlls/msvcrt/tests/scanf.c b/dlls/msvcrt/tests/scanf.c index b7244835ac..e1e351e0bb 100644 --- a/dlls/msvcrt/tests/scanf.c +++ b/dlls/msvcrt/tests/scanf.c @@ -294,6 +294,12 @@ static void test_sscanf_s(void) ret = psscanf_s("123", "%3c", buf, 3); ok(!strcmp("123a", buf), "buf = %s\n", buf); + /* Test to verify how unterminated and invalid sequence gets handled */ + memset(buf, 'a', sizeof(buf)); + ret = psscanf_s(" ", "%[-", buf, 2); + ok(ret == 1, "Wrong number of arguments read: %d\n", ret); + ok(!strcmp(" ", buf), "buf = %s\n", buf); + i = 1; ret = psscanf_s("123 123", "%s %d", buf, 2, &i); ok(ret == 0, "Wrong number of arguments read: %d\n", ret); -- 2.18.0
From: Michał Janiszewski <janisozaur(a)gmail.com> An unexpected format string of form "%" can cause scanf() family of functions to read past end of it. Signed-off-by: Michał Janiszewski <janisozaur(a)gmail.com> --- dlls/msvcrt/scanf.h | 2 +- dlls/msvcrt/tests/scanf.c | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/dlls/msvcrt/scanf.h b/dlls/msvcrt/scanf.h index 68585468fe..138d4351a2 100644 --- a/dlls/msvcrt/scanf.h +++ b/dlls/msvcrt/scanf.h @@ -686,7 +686,7 @@ _FUNCTION_ { * use %%." */ while ((nch!=_EOF_) && _ISSPACE_(nch)) nch = _GETC_(file); - if ((_CHAR_)nch == *format) { + if (*format && (_CHAR_)nch == *format) { suppress = 1; /* whoops no field to be read */ st = 1; /* but we got what we expected */ nch = _GETC_(file); diff --git a/dlls/msvcrt/tests/scanf.c b/dlls/msvcrt/tests/scanf.c index e1e351e0bb..06165558c5 100644 --- a/dlls/msvcrt/tests/scanf.c +++ b/dlls/msvcrt/tests/scanf.c @@ -300,6 +300,12 @@ static void test_sscanf_s(void) ok(ret == 1, "Wrong number of arguments read: %d\n", ret); ok(!strcmp(" ", buf), "buf = %s\n", buf); + memset(buf, 'a', sizeof(buf)); + buf[4] = 0; + ret = psscanf_s(" ", "%", buf, 2); + ok(ret == EOF, "Wrong number of arguments read: %d\n", ret); + ok(!strcmp("aaaa", buf), "buf = %s\n", buf); + i = 1; ret = psscanf_s("123 123", "%s %d", buf, 2, &i); ok(ret == 0, "Wrong number of arguments read: %d\n", ret); -- 2.18.0
On 07/29/18 23:25, janisozaur(a)gmail.com wrote:
@@ -686,7 +686,7 @@ _FUNCTION_ { * use %%." */ while ((nch!=_EOF_) && _ISSPACE_(nch)) nch = _GETC_(file); - if ((_CHAR_)nch == *format) { + if (*format && (_CHAR_)nch == *format) { suppress = 1; /* whoops no field to be read */ st = 1; /* but we got what we expected */ nch = _GETC_(file); Shouldn't this check be done before whitespaces are read?
Thanks, Piotr
From: Michał Janiszewski <janisozaur(a)gmail.com> Some unexpected sequences can buffer overrun due to insufficient format string verification. This patch fixes buffer overrun for format string of form "%[^" Signed-off-by: Michał Janiszewski <janisozaur(a)gmail.com> --- dlls/msvcrt/scanf.h | 2 +- dlls/msvcrt/tests/scanf.c | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/dlls/msvcrt/scanf.h b/dlls/msvcrt/scanf.h index 138d4351a2..04e06f6882 100644 --- a/dlls/msvcrt/scanf.h +++ b/dlls/msvcrt/scanf.h @@ -704,7 +704,7 @@ _FUNCTION_ { nch = _GETC_(file); } else break; } - format++; + if (*format) format++; } if (nch!=_EOF_) { _UNGETC_(nch, file); diff --git a/dlls/msvcrt/tests/scanf.c b/dlls/msvcrt/tests/scanf.c index 06165558c5..7c005ae14f 100644 --- a/dlls/msvcrt/tests/scanf.c +++ b/dlls/msvcrt/tests/scanf.c @@ -306,6 +306,12 @@ static void test_sscanf_s(void) ok(ret == EOF, "Wrong number of arguments read: %d\n", ret); ok(!strcmp("aaaa", buf), "buf = %s\n", buf); + memset(buf, 'a', sizeof(buf)); + buf[4] = 0; + ret = psscanf_s(" ", "%[^", buf, 2); + ok(ret == 0, "Wrong number of arguments read: %d\n", ret); + ok(!strcmp("aaaa", buf), "buf = %s\n", buf); + i = 1; ret = psscanf_s("123 123", "%s %d", buf, 2, &i); ok(ret == 0, "Wrong number of arguments read: %d\n", ret); -- 2.18.0
Hi Michał, The test is not working with wine: scanf.c:300: Test failed: Wrong number of arguments read: 0 scanf.c:301: Test failed: buf = make: *** [Makefile:498: scanf.ok] Error 2 The tests in your other patches are also introducing some new failures. Thanks, Piotr
Thanks, I'll re-check with wine proper, as I only did so with my copy of the code. Just to make sure: is the test in question already running with updated scanf.h? On 30 July 2018 at 13:48, Piotr Caban <piotr.caban(a)gmail.com> wrote:
Hi Michał,
The test is not working with wine: scanf.c:300: Test failed: Wrong number of arguments read: 0 scanf.c:301: Test failed: buf = make: *** [Makefile:498: scanf.ok] Error 2
The tests in your other patches are also introducing some new failures.
Thanks, Piotr
-- Michal Janiszewski
participants (3)
-
janisozaur＠gmail.com -
Michał Janiszewski
-
Piotr Caban