Closed soh0ro0t closed 8 years ago
Thanks for the report @TheBeeMan. From the description above, even if not explicitly stated it looks like your code review is covering the logic in getAncestry() (please pinpoint if this covers something else instead).
In the above code, I agree that the current parent-handling behavior is buggy as it doesn't take into account cyclic dependencies.
However I'm not completely sure about any security implication of this. Triggering this infinite loop seems to require local access and only concern a single user process. This will just result in a never-ending conversion of a single malformed image, which shouldn't impact availability per-se as there is no single service to DoS here. As you pointed out, there will be indeed some over-usage of processing resources but this is something that is typically controlled by the OS (eg. via cgroups, priority-throttling, OOM-killing).
What's was your original impact analysis of this suggesting that it requires a CVE?
This is a typical denial of service vulnerability, which could result in over-usage of processing resources. I think it is a CVE worthy vulnerability, although it is a low severity issue but a crafted image could cause docker2aci fall into a never-ending conversion.
The way you think about security is different with me, denial of service is a grey area, Obviously if I send 10 gigabits of request traffic or an valid input files and the processor gets slow/non responsive the CVE response would be "No CVE for you" ,but if a single crafted file prevents the whole system from working in an expected manner, that may be a problem that is worth a CVE.
Just for reference, infinite loop is a well-known security issue (CWE-835)
Could you request a CVE for that ?
Sure! I've just forwarded this to CVE Assignment Team at MITRE and put you in copy. We'll be waiting for their reply regarding submission review and CVE ID allocation. Thanks again for following up on this.
After MITRE review, in the context of docker2aci usage as an embedded library, this bug has been recognized as a security issue and assigned CVE-2016-8579.
Here the reply from CVE Assignment Team:
docker2aci is apparently a library [...] and we almost always recognize
the potential for an unattended use case for any library.
[...]
Someone can call the ConvertSavedFile function from an arbitrary
application. [...] It might be automated with cron or a similar unattended
tool that runs in an unrestricted (non-container) environment. Thus,
there is an availability impact because no human is around to notice
the CPU usage.
Use CVE-2016-8579.
@TheBeeMan a proposed fix for this is up at https://github.com/appc/docker2aci/pull/204, adding additional validation on crafted images. Can you please take a look at it?
i reviewed the patch for CVE-2016-8579 and processed some tests with the previous malicious image, it addressed the issue. i agree.
Hi,
In code reviewing, i found an infinite loop vulnerability in retrieving images chain using docker2aci, it occurs during the corresponding json file parsing from user's image archive, fetching the parent image ID until ID is nil. There must be a possibility that the images chain may be a closed cycle, thus , docker2aci will fall into an infinite loop, that's indeed true by some interesting tests.
I think the core cause of this issue is lacking in essential check for duplicated image ID, such as the current image ID could not be equal to its parent image ID, most important, check whether the images chain is a closed cycle.
I processed some interesting test for this issue, building a crafted image whose top layer's parent ID points to itself, then an infinite loop occurred, this flaw caused excessive CPU cycles & resources consume on the host.
expecting subsequent discuss and fix the issue together, and could you request a CVE identifier for that ?