From c0e34d1a9ed0511d87c9b9aa618d796503e294ea Mon Sep 17 00:00:00 2001 From: William Elwood Date: Fri, 1 Nov 2019 01:23:32 +0000 Subject: [PATCH] Verify signature immediately after reading from cache or URL This allows a large number of tests to be enabled and pass now that the behaviour is expected. The main fix here is that a download with an invalid signature will always fall back on using a properly signed cache, no matter how old it is. Additionally, downloads will never be written to the cache unless they are properly signed (both at startup and prefetching). --- dnscrypt-proxy/sources.go | 35 ++++++++++++++----------- dnscrypt-proxy/sources_test.go | 48 +++++++++++++++++----------------- 2 files changed, 44 insertions(+), 39 deletions(-) diff --git a/dnscrypt-proxy/sources.go b/dnscrypt-proxy/sources.go index cf5967bd..82e26213 100644 --- a/dnscrypt-proxy/sources.go +++ b/dnscrypt-proxy/sources.go @@ -51,18 +51,23 @@ func (source *Source) checkSignature(bin, sig []byte) (err error) { // timeNow can be replaced by tests to provide a static value var timeNow = time.Now -func (source *Source) fetchFromCache() (bin, sig []byte, delayTillNextUpdate time.Duration, err error) { +func (source *Source) fetchFromCache() (delayTillNextUpdate time.Duration, err error) { delayTillNextUpdate = 0 - var fi os.FileInfo - if fi, err = os.Stat(source.cacheFile); err != nil { - return - } + var bin, sig []byte if bin, err = ioutil.ReadFile(source.cacheFile); err != nil { return } if sig, err = ioutil.ReadFile(source.cacheFile + ".minisig"); err != nil { return } + if err = source.checkSignature(bin, sig); err != nil { + return + } + source.in = bin + var fi os.FileInfo + if fi, err = os.Stat(source.cacheFile); err != nil { + return + } if elapsed := timeNow().Sub(fi.ModTime()); elapsed < source.cacheTTL { dlog.Debugf("Cache file [%s] is still fresh", source.cacheFile) delayTillNextUpdate = source.prefetchDelay - elapsed @@ -100,8 +105,8 @@ func fetchFromURL(xTransport *XTransport, u *url.URL) (bin []byte, err error) { return } -func (source *Source) fetchWithCache(xTransport *XTransport, urlStr string) (bin, sig []byte, delayTillNextUpdate time.Duration, err error) { - if bin, sig, delayTillNextUpdate, err = source.fetchFromCache(); err != nil { +func (source *Source) fetchWithCache(xTransport *XTransport, urlStr string) (delayTillNextUpdate time.Duration, err error) { + if delayTillNextUpdate, err = source.fetchFromCache(); err != nil { if len(urlStr) == 0 { dlog.Errorf("Cache file [%s] not present and no URL given to retrieve it", source.cacheFile) return @@ -122,12 +127,17 @@ func (source *Source) fetchWithCache(xTransport *XTransport, urlStr string) (bin sigURL := &url.URL{} *sigURL = *srcURL // deep copy to avoid parsing twice sigURL.Path += ".minisig" + var bin, sig []byte if bin, err = fetchFromURL(xTransport, srcURL); err != nil { return } if sig, err = fetchFromURL(xTransport, sigURL); err != nil { return } + if err = source.checkSignature(bin, sig); err != nil { + return + } + source.in = bin source.writeToCache(bin, sig) // ignore error: not fatal delayTillNextUpdate = source.prefetchDelay return @@ -150,13 +160,12 @@ func NewSource(xTransport *XTransport, urls []string, minisignKeyStr string, cac } now := timeNow() - var bin, sig []byte var delayTillNextUpdate time.Duration if len(urls) <= 0 { - bin, sig, delayTillNextUpdate, err = source.fetchWithCache(xTransport, "") + delayTillNextUpdate, err = source.fetchWithCache(xTransport, "") } else { for _, url := range urls { - bin, sig, delayTillNextUpdate, err = source.fetchWithCache(xTransport, url) + delayTillNextUpdate, err = source.fetchWithCache(xTransport, url) if err == nil { break } @@ -168,11 +177,7 @@ func NewSource(xTransport *XTransport, urls []string, minisignKeyStr string, cac return } - if err = source.checkSignature(bin, sig); err != nil { - return - } dlog.Noticef("Source [%s] loaded", cacheFile) - source.in = bin return } @@ -188,7 +193,7 @@ func PrefetchSources(xTransport *XTransport, sources []*Source) time.Duration { continue } dlog.Debugf("Prefetching [%s]", u) - _, _, delay, err := source.fetchWithCache(xTransport, u) + delay, err := source.fetchWithCache(xTransport, u) if err != nil { dlog.Debugf("Prefetching [%s] failed: %v", u, err) continue diff --git a/dnscrypt-proxy/sources_test.go b/dnscrypt-proxy/sources_test.go index 35e92304..cd86301d 100644 --- a/dnscrypt-proxy/sources_test.go +++ b/dnscrypt-proxy/sources_test.go @@ -197,32 +197,32 @@ func setupSourceTest(t *testing.T) (func(), *SourceTestData) { d.cacheTests = map[string]SourceTestState{ // determines cache files written to disk before each call "correct": TestStateCorrect, //"expired": TestStateExpired, // TODO: an expired cache should be used if no download passes signature verification - //"partial": TestStatePartial, // TODO: failed signature verification should not abort before trying additional URLs - //"partial-sig": TestStatePartialSig, // TODO: failed signature verification should not abort before trying additional URLs - "missing": TestStateMissing, - //"missing-sig": TestStateMissingSig, // TODO: a cache without a signature should cause an attempt to download both files (not just the signature) - "open-err": TestStateOpenErr, - //"open-sig-err": TestStateOpenSigErr, // TODO: a cache without a signature should cause an attempt to download both files (not just the signature) + "partial": TestStatePartial, + "partial-sig": TestStatePartialSig, + "missing": TestStateMissing, + "missing-sig": TestStateMissingSig, + "open-err": TestStateOpenErr, + "open-sig-err": TestStateOpenSigErr, } d.downloadTests = map[string][]SourceTestState{ // determines the list of URLs passed in each call and how they will respond - "correct": {TestStateCorrect}, - //"partial": {TestStatePartial}, // TODO: failed signature verification should not count as successfully prefetched - //"partial-sig": {TestStatePartialSig}, // TODO: failed signature verification should not count as successfully prefetched - "missing": {TestStateMissing}, - //"missing-sig": {TestStateMissingSig}, // TODO: failed signature verification should not count as successfully prefetched - "read-err": {TestStateReadErr}, - //"read-sig-err": {TestStateReadSigErr}, // TODO: failed signature verification should not count as successfully prefetched - "open-err": {TestStateOpenErr}, - //"open-sig-err": {TestStateOpenSigErr}, // TODO: failed signature verification should not count as successfully prefetched + "correct": {TestStateCorrect}, + "partial": {TestStatePartial}, + "partial-sig": {TestStatePartialSig}, + "missing": {TestStateMissing}, + "missing-sig": {TestStateMissingSig}, + "read-err": {TestStateReadErr}, + "read-sig-err": {TestStateReadSigErr}, + "open-err": {TestStateOpenErr}, + "open-sig-err": {TestStateOpenSigErr}, //"path-err": {TestStatePathErr}, // TODO: invalid URLs should not be included in the prefetch list - //"partial,correct": {TestStatePartial, TestStateCorrect}, // TODO: all URLs should be included in the prefetch list, failed signature verification should not abort before trying additional URLs - //"partial-sig,correct": {TestStatePartialSig, TestStateCorrect}, // TODO: all URLs should be included in the prefetch list, failed signature verification should not abort before trying additional URLs - //"missing,correct": {TestStateMissing, TestStateCorrect}, // TODO: all URLs should be included in the prefetch list - //"missing-sig,correct": {TestStateMissingSig, TestStateCorrect}, // TODO: all URLs should be included in the prefetch list, failed signature verification should not abort before trying additional URLs - //"read-err,correct": {TestStateReadErr, TestStateCorrect}, // TODO: all URLs should be included in the prefetch list - //"read-sig-err,correct": {TestStateReadSigErr, TestStateCorrect}, // TODO: all URLs should be included in the prefetch list, failed signature verification should not abort before trying additional URLs - //"open-err,correct": {TestStateOpenErr, TestStateCorrect}, // TODO: all URLs should be included in the prefetch list - //"open-sig-err,correct": {TestStateOpenSigErr, TestStateCorrect}, // TODO: all URLs should be included in the prefetch list, failed signature verification should not abort before trying additional URLs + "partial,correct": {TestStatePartial, TestStateCorrect}, + "partial-sig,correct": {TestStatePartialSig, TestStateCorrect}, + "missing,correct": {TestStateMissing, TestStateCorrect}, + "missing-sig,correct": {TestStateMissingSig, TestStateCorrect}, + "read-err,correct": {TestStateReadErr, TestStateCorrect}, + "read-sig-err,correct": {TestStateReadSigErr, TestStateCorrect}, + "open-err,correct": {TestStateOpenErr, TestStateCorrect}, + "open-sig-err,correct": {TestStateOpenSigErr, TestStateCorrect}, //"path-err,correct": {TestStatePathErr, TestStateCorrect}, // TODO: invalid URLs should not be included in the prefetch list "no-urls": {}, } @@ -252,7 +252,7 @@ func prepSourceTestCache(t *testing.T, d *SourceTestData, e *SourceTestExpect, s case TestStatePartial, TestStatePartialSig: e.err = "signature" case TestStateMissing, TestStateMissingSig: - e.err = "stat" + e.err = "open" case TestStateOpenErr, TestStateOpenSigErr: e.err = os.ErrPermission.Error() }