Race condition on creating a full chain for a certificate

[aaomidi]

I've come across a race with how

certificate-transparency-go/submission/distributor.go

Lines 254 to 258 in a82fc3e

	compatibleLogsAndChain := func() (loglist3.LogList, []*x509.Certificate, error) {
	d.mu.RLock()
	defer d.mu.RUnlock()
	vOpts := ctfe.NewCertValidationOpts(d.rootPool, time.Time{}, false, false, nil, nil, false, nil)
	rootedChain, err := ctfe.ValidateChain(rawChain, vOpts)

builds the rootedChain.

ctfe.ValidateChain takes in the global root pool. This pool contains every single root that the application started with. Those get populated here:

certificate-transparency-go/submission/distributor.go

Lines 163 to 170 in a82fc3e

	// Merge individual root-pools into a unified one
	d.rootPool = x509util.NewPEMCertPool()
	for _, pool := range d.logRoots {
	for _, c := range pool.RawCertificates() {
	d.rootPool.AddCert(c)
	}
	}

The race condition is that PEMCertPool gets populated in an arbitrary order since the loop is over a map.

ctfe.ValidateChain builds various chains, and picks the first valid one:

certificate-transparency-go/trillian/ctfe/cert_checker.go

Lines 166 to 173 in a82fc3e

	// Verify might have found multiple paths to roots. Now we check that we have a path that
	// uses all the certs in the order they were submitted so as to comply with RFC 6962
	// requirements detailed in Section 3.1.
	for _, verifiedChain := range verifiedChains {
	if chainsEquivalent(chain, verifiedChain) {
	return verifiedChain, nil
	}
	}

However, not every CT log necessarily has the same trust store, and the RootCompatible method will filter that CT log out.

The fix for this would be to consider the root pool per log so the constructed chain can be different per log.

[roger2hk]

@aaomidi Is this issue fully addressed by #1258?

[aaomidi]

Not at all. That PR simply disabled this functionality to work around the bug.

The real solution requires making a trust anchor per log endpoint, or potentially getting rid of this feature entirely.