common_args is not included in preprocessor caching

[GZGavinZhao]

Repro file:

#include <bits/stdc++.h>

using namespace std;

int main()
{
#ifdef HELLO
	cout << "Hello World" << endl;
#else
	cout << "Hi World" << endl;
#endif
}

Run sccache gcc -c main.cpp -DHELLO. Then run sccache 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 take common_args into account, and since -DHELLO belongs to common_args, removing it doesn't affect the hash of preprocessor_and_arch_args, hence registering a cache hit.

[KyleFromNVIDIA]

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.

[glandium]

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.

[KyleFromNVIDIA]

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:

file1.cpp:

int doStuffInFile1()
{
#ifdef HAVE_FEATURE
  return 2;
#else
  return 1;
#endif
}

file2.cpp:

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 file1.cpp is built without -DHAVE_FEATURE, and file2.cpp is built with -DHAVE_FEATURE, the unforeseen/hidden harmful effects will take place. An attacker could easily submit a PR with the above and get it accepted without controversy. Even if they didn't submit the code themselves, they could still exploit it if it was written by someone else in good faith but built improperly by sccache.

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 -DHAVE_FEATURE, it's added to the cache, and the vulnerability now has a 100% hit rate, as opposed to 50% or less.

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.