| Message ID | 20200331140638.16464-2-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qga: Restrict guest-file-read count to 10 MB to avoid crashes | expand |
On Tue, Mar 31, 2020 at 04:06:35PM +0200, Philippe Mathieu-Daudé wrote: > By using g_try_malloc() instead of g_malloc() the qemu-guest-agent > Denial-of-Service attack referred in commit 807e2b6fce is reduced, > but still triggerable: As explained previously, I believe there is *no* denial of service attack here. The described scenario is just a user hurting themselves by intentionally telling QEMU not to limit the amount of data returned. > > - bisect file size S until g_try_malloc(S) fails, > - use S - 1: > g_try_malloc(S - 1) succeeds, but g_new0() few lines later will > fail. > > 346 buf = g_try_malloc0(count + 1); > 347 if (!buf) { > 348 error_setg(errp, > 349 "failed to allocate sufficient memory " > 350 "to complete the requested service"); > 351 return NULL; > 352 } > 353 is_ok = ReadFile(fh, buf, count, &read_count, NULL); > 354 if (!is_ok) { > 355 error_setg_win32(errp, GetLastError(), "failed to read file"); > 356 slog("guest-file-read failed, handle %" PRId64, handle); > 357 } else { > 358 buf[read_count] = 0; > 359 read_data = g_new0(GuestFileRead, 1); > ^^^^^^ > > Instead we are going to put a low hard limit on 'count' in the next > commits. > This reverts commit 807e2b6fce022707418bc8f61c069d91c613b3d2. > > Suggested-by: Daniel P. Berrangé <berrange@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > qga/commands-win32.c | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) > > diff --git a/qga/commands-win32.c b/qga/commands-win32.c > index b49920e201..46cea7d1d9 100644 > --- a/qga/commands-win32.c > +++ b/qga/commands-win32.c > @@ -343,13 +343,7 @@ GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count, > } > > fh = gfh->fh; > - buf = g_try_malloc0(count + 1); > - if (!buf) { > - error_setg(errp, > - "failed to allocate sufficient memory " > - "to complete the requested service"); > - return NULL; > - } > + buf = g_malloc0(count + 1); > is_ok = ReadFile(fh, buf, count, &read_count, NULL); > if (!is_ok) { > error_setg_win32(errp, GetLastError(), "failed to read file"); > -- > 2.21.1 > Regards, Daniel
On 3/31/20 4:12 PM, Daniel P. Berrangé wrote: > On Tue, Mar 31, 2020 at 04:06:35PM +0200, Philippe Mathieu-Daudé wrote: >> By using g_try_malloc() instead of g_malloc() the qemu-guest-agent >> Denial-of-Service attack referred in commit 807e2b6fce is reduced, >> but still triggerable: > > As explained previously, I believe there is *no* denial of service > attack here. The described scenario is just a user hurting themselves > by intentionally telling QEMU not to limit the amount of data returned. Yes. Do you mind updating the BZ, eventually marking as NOTABUG? Then I can adapt the patch descriptions. https://bugzilla.redhat.com/show_bug.cgi?id=1594054 > >> >> - bisect file size S until g_try_malloc(S) fails, >> - use S - 1: >> g_try_malloc(S - 1) succeeds, but g_new0() few lines later will >> fail. >> >> 346 buf = g_try_malloc0(count + 1); >> 347 if (!buf) { >> 348 error_setg(errp, >> 349 "failed to allocate sufficient memory " >> 350 "to complete the requested service"); >> 351 return NULL; >> 352 } >> 353 is_ok = ReadFile(fh, buf, count, &read_count, NULL); >> 354 if (!is_ok) { >> 355 error_setg_win32(errp, GetLastError(), "failed to read file"); >> 356 slog("guest-file-read failed, handle %" PRId64, handle); >> 357 } else { >> 358 buf[read_count] = 0; >> 359 read_data = g_new0(GuestFileRead, 1); >> ^^^^^^ >> >> Instead we are going to put a low hard limit on 'count' in the next >> commits. >> This reverts commit 807e2b6fce022707418bc8f61c069d91c613b3d2. >> >> Suggested-by: Daniel P. Berrangé <berrange@redhat.com> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> qga/commands-win32.c | 8 +------- >> 1 file changed, 1 insertion(+), 7 deletions(-) >> >> diff --git a/qga/commands-win32.c b/qga/commands-win32.c >> index b49920e201..46cea7d1d9 100644 >> --- a/qga/commands-win32.c >> +++ b/qga/commands-win32.c >> @@ -343,13 +343,7 @@ GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count, >> } >> >> fh = gfh->fh; >> - buf = g_try_malloc0(count + 1); >> - if (!buf) { >> - error_setg(errp, >> - "failed to allocate sufficient memory " >> - "to complete the requested service"); >> - return NULL; >> - } >> + buf = g_malloc0(count + 1); >> is_ok = ReadFile(fh, buf, count, &read_count, NULL); >> if (!is_ok) { >> error_setg_win32(errp, GetLastError(), "failed to read file"); >> -- >> 2.21.1 >> > > Regards, > Daniel >
diff --git a/qga/commands-win32.c b/qga/commands-win32.c index b49920e201..46cea7d1d9 100644 --- a/qga/commands-win32.c +++ b/qga/commands-win32.c @@ -343,13 +343,7 @@ GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count, } fh = gfh->fh; - buf = g_try_malloc0(count + 1); - if (!buf) { - error_setg(errp, - "failed to allocate sufficient memory " - "to complete the requested service"); - return NULL; - } + buf = g_malloc0(count + 1); is_ok = ReadFile(fh, buf, count, &read_count, NULL); if (!is_ok) { error_setg_win32(errp, GetLastError(), "failed to read file");
By using g_try_malloc() instead of g_malloc() the qemu-guest-agent Denial-of-Service attack referred in commit 807e2b6fce is reduced, but still triggerable: - bisect file size S until g_try_malloc(S) fails, - use S - 1: g_try_malloc(S - 1) succeeds, but g_new0() few lines later will fail. 346 buf = g_try_malloc0(count + 1); 347 if (!buf) { 348 error_setg(errp, 349 "failed to allocate sufficient memory " 350 "to complete the requested service"); 351 return NULL; 352 } 353 is_ok = ReadFile(fh, buf, count, &read_count, NULL); 354 if (!is_ok) { 355 error_setg_win32(errp, GetLastError(), "failed to read file"); 356 slog("guest-file-read failed, handle %" PRId64, handle); 357 } else { 358 buf[read_count] = 0; 359 read_data = g_new0(GuestFileRead, 1); ^^^^^^ Instead we are going to put a low hard limit on 'count' in the next commits. This reverts commit 807e2b6fce022707418bc8f61c069d91c613b3d2. Suggested-by: Daniel P. Berrangé <berrange@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- qga/commands-win32.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)