-
Notifications
You must be signed in to change notification settings - Fork 546
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
common_args is not included in preprocessor caching #2038
common_args is not included in preprocessor caching #2038
Comments
IMO this bug presents a potential security concern. It can be used to poison a cache with incorrect build artifacts. We should file a CVE and mark 0.7.4 and 0.7.5 as vulnerable. |
It only affects preprocessor mode, which only works locally, and for poisoning to happen, it would have to happen from compiling the same code from the same paths with different flags, which is unlikely to happen maliciously. An attacker that could pull that off could just as well poison the cache by writing into it directly. |
I believe an attacker could exploit this by combining it with other attacks. If multiple builds of the same project are taking place with different compile flags, there could be a race condition where objects that are built with some compile flag are combined with objects that are not. Consider the following:
int doStuffInFile1()
{
#ifdef HAVE_FEATURE
return 2;
#else
return 1;
#endif
}
int doStuffInFile2()
{
int value = doStuffInFile1();
#ifdef HAVE_FEATURE
std::cout << "This code is acting on the assumption that `value` is 2, and may have unforeseen/hidden harmful effects if it is not\n";
#endif
} If This bug could also be used to amplify the hit rate on an already-poisoned cache object. This assumes the attacker was able to introduce a single poisoned object into the cache, either through the front door or the back door. As a front door example, an attacker could submit the following in a PR: void doStuff()
{
#ifdef HAVE_FEATURE
std::cout << "This feature looks innocent enough, but actually has a hidden vulnerability that's not obvious to reviewers\n";
#endif
} If the first build of this file has Given the potential exploits above, I think it would be wise to file a CVE, and I think the consequences of not doing so and being wrong are far greater than the consequences of sounding a false alarm (which is to say, none). Even if we discount the attack potential and don't file a CVE, I think it's pretty uncontroversial to say that 0.7.4 and 0.7.5 should not be used under any circumstances, because they are known to produce incorrect builds. We got hit by the above race condition yesterday, and the end result was that we had to dump our entire S3 cache. |
Repro file:
Run
sccache gcc -c main.cpp -DHELLO
. Then runsccache gcc -c main.cpp
. You will see that the second command registers a cache hit when it clearly shouldn't.This happens because as shown here,
preprocessor_and_arch_args
doesn't takecommon_args
into account, and since-DHELLO
belongs tocommon_args
, removing it doesn't affect the hash ofpreprocessor_and_arch_args
, hence registering a cache hit.The text was updated successfully, but these errors were encountered: