CVE-2019-5736: runc container breakout

Posted Feb 12, 2019 19:23 UTC (Tue) by sorokin (guest, #88478)
Parent article: CVE-2019-5736: runc container breakout

> do {
> *output = malloc(sizeof(**output));
> } while (!*output);

Am I the only one person who consider these infinite loops a bad practice? I would say that the code should report the error. It also must preserve the state unchanged so the operation can be retried.

Having infinite loops like this especially in function that read whole file in memory looks very strange.

Apparently the authors are not consistent. They have asprintf() without looping in the same file.

to post comments

CVE-2019-5736: runc container breakout

Posted Feb 12, 2019 19:34 UTC (Tue) by excors (subscriber, #95769) [Link] (1 responses)

Reporting errors when out of memory seems difficult, unless you're extremely careful to implement the entire error-reporting path with no memory allocation at all (including in any third-party libraries you call into). More practical to just fail in a secure and obvious way (like abort() or (if you can expect the user to notice and debug/kill the process) loop forever).

CVE-2019-5736: runc container breakout

Posted Feb 12, 2019 20:26 UTC (Tue) by sorokin (guest, #88478) [Link]

> Reporting errors when out of memory seems difficult, unless you're extremely careful to implement the entire error-reporting path with no memory allocation at all (including in any third-party libraries you call into).

I completely agree. It is understandably difficult to be 100% exception-safe. That is why under "report the error" I meant reporting in general. "fprintf(stderr, ...); abort();" fine. "errno = ENOMEM; return -1;" fine. "throw std::bad_alloc();" fine. I would say that any kind of error reporting is OK as far as no data is lost and the operation can be retried.

> if you can expect the user to notice and debug/kill the proces

That is what I think is not reasonable to expect. When some KDE application hung using 100% CPU time, the last idea that will come to my mind is that it lacks memory. Also most users don't know how to use gdb.

CVE-2019-5736: runc container breakout

Posted Feb 12, 2019 23:57 UTC (Tue) by cyphar (subscriber, #110703) [Link]

The patch actually pushed is substantially cleaner[1], but the looping is mostly because the patch was co-developed with the LXC folks and they have must_realloc and family that do the exact same thing. "reporting an error" here would be a crash, by the way (in the context of "runc init" we currently don't have a way to report errors other than through the exit code). The first few iterations of the patch just aborted each time, but mimicking LXC's must_realloc seemed nicer.

> They have asprintf() without looping in the same file.

Yup, that is a mistake -- I will fix that when I update the fix to work on pre-3.11 kernels.

[1]: https://github.com/opencontainers/runc/commit/6635b4f0c6a...