diff --git a/CHANGELOG.md b/CHANGELOG.md index 4adb443fefd..00f447fdbb0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,16 @@ See also the [v0.107.78 GitHub milestone][ms-v0.107.78]. NOTE: Add new changes BELOW THIS COMMENT. --> +### Security + +- The size of rulelists is limited. This is necessary to prevent a user's machine from becoming overloaded if the filter source misbehaves. + +### Changed + +#### Configuration changes + +- The `filtering` object of the YAML configuration now includes a new property, `max_http_size`, which defines the maximum size of the HTTP request for rulelists. To disable the limitation, set a large size, such as `1 TB`. + ### Fixed - Blocked services check on the Custom filtering rules page does not work properly without specifying of a client. diff --git a/internal/filtering/filter.go b/internal/filtering/filter.go index 167b8d1f128..6ff3b9a37ad 100644 --- a/internal/filtering/filter.go +++ b/internal/filtering/filter.go @@ -19,6 +19,7 @@ import ( "github.com/AdguardTeam/AdGuardHome/internal/filtering/rulelist" "github.com/AdguardTeam/golibs/container" "github.com/AdguardTeam/golibs/errors" + "github.com/AdguardTeam/golibs/ioutil" "github.com/AdguardTeam/golibs/logutil/slogutil" ) @@ -30,7 +31,8 @@ const filterDir = "filters" // // TODO(e.burkov): Investigate if the field ordering is important. type FilterYAML struct { - Enabled bool + Enabled bool + // TODO(m.kazantsev): Refactor. URL string // URL or a file path Name string `yaml:"name"` RulesCount int `yaml:"-"` @@ -495,7 +497,7 @@ func (d *DNSFilter) update(filter *FilterYAML) (b bool, err error) { } // updateIntl updates the flt rewriting it's actual file. It returns true if -// the actual update has been performed. +// the actual update has been performed. flt must not be nil. func (d *DNSFilter) updateIntl(ctx context.Context, flt *FilterYAML) (ok bool, err error) { d.logger.DebugContext(ctx, "downloading update for filter", "id", flt.ID, "url", flt.URL) @@ -503,24 +505,78 @@ func (d *DNSFilter) updateIntl(ctx context.Context, flt *FilterYAML) (ok bool, e tmpFile, err := aghrenameio.NewPendingFile(flt.Path(d.conf.DataDir), aghos.DefaultPermFile) if err != nil { + // Don't wrap the error because it's informative enough as is. return false, err } defer func() { err = d.finalizeUpdate(ctx, tmpFile, flt, res, err, ok) }() - r, err := d.reader(flt.URL) + if filepath.IsAbs(flt.URL) { + // Initialise this variable to avoid any confusion. + path := flt.URL + + res, err = d.readFromFile(tmpFile, path) + } else { + res, err = d.readFromHTTP(tmpFile, flt.URL) + } + if err != nil { - // Don't wrap the error since it's informative enough as is. + // Don't wrap the error because it's informative enough as is. return false, err } - defer func() { err = errors.WithDeferred(err, r.Close()) }() + + return res.Checksum != flt.checksum, nil +} + +// readFromHTTP reads filter data from urlStr via HTTP and parses it into the +// tmpFile file. tmpFile must not be nil. urlStr must be a valid URL. +func (d *DNSFilter) readFromHTTP( + tmpFile aghrenameio.PendingFile, + urlStr string, +) (res *rulelist.ParseResult, err error) { + resp, err := d.conf.HTTPClient.Get(urlStr) + if err != nil { + // Don't wrap the error because it's informative enough as is. + return nil, err + } + defer func() { err = errors.WithDeferred(err, resp.Body.Close()) }() + + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("got status code %d, want %d", resp.StatusCode, http.StatusOK) + } + + bufPtr := d.bufPool.Get() + defer d.bufPool.Put(bufPtr) + + p := rulelist.NewParser() + httpBody := ioutil.LimitReader(resp.Body, d.conf.MaxHTTPSize.Bytes()) + + return p.Parse(tmpFile, httpBody, *bufPtr) +} + +// readFromFile reads filter data from a file located at path and parses it into +// the tmpFile file. tmpFile must not be nil. path must be a valid filepath. +func (d *DNSFilter) readFromFile( + tmpFile aghrenameio.PendingFile, + path string, +) (res *rulelist.ParseResult, err error) { + path = filepath.Clean(path) + + if !pathMatchesAny(d.safeFSPatterns, path) { + return nil, fmt.Errorf("path %q does not match safe patterns", path) + } + + file, err := os.Open(path) + if err != nil { + return nil, fmt.Errorf("opening file: %w", err) + } + defer func() { err = errors.WithDeferred(err, file.Close()) }() bufPtr := d.bufPool.Get() defer d.bufPool.Put(bufPtr) p := rulelist.NewParser() - res, err = p.Parse(tmpFile, r, *bufPtr) - return res.Checksum != flt.checksum && err == nil, err + return p.Parse(tmpFile, file, *bufPtr) } // finalizeUpdate closes and gets rid of temporary file f with filter's content @@ -566,47 +622,6 @@ func (d *DNSFilter) finalizeUpdate( return nil } -// reader returns an io.ReadCloser reading filtering-rule list data form either -// a file on the filesystem or the filter's HTTP URL. -func (d *DNSFilter) reader(fltURL string) (r io.ReadCloser, err error) { - if !filepath.IsAbs(fltURL) { - r, err = d.readerFromURL(fltURL) - if err != nil { - return nil, fmt.Errorf("reading from url: %w", err) - } - - return r, nil - } - - fltURL = filepath.Clean(fltURL) - if !pathMatchesAny(d.safeFSPatterns, fltURL) { - return nil, fmt.Errorf("path %q does not match safe patterns", fltURL) - } - - r, err = os.Open(fltURL) - if err != nil { - return nil, fmt.Errorf("opening file: %w", err) - } - - return r, nil -} - -// readerFromURL returns an io.ReadCloser reading filtering-rule list data form -// the filter's URL. -func (d *DNSFilter) readerFromURL(fltURL string) (r io.ReadCloser, err error) { - resp, err := d.conf.HTTPClient.Get(fltURL) - if err != nil { - // Don't wrap the error since it's informative enough as is. - return nil, err - } - - if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("got status code %d, want %d", resp.StatusCode, http.StatusOK) - } - - return resp.Body, nil -} - // loads filter contents from the file in dataDir func (d *DNSFilter) load(ctx context.Context, flt *FilterYAML) (err error) { fileName := flt.Path(d.conf.DataDir) diff --git a/internal/filtering/filter_internal_test.go b/internal/filtering/filter_internal_test.go index 9e7165cae15..77cf95b675c 100644 --- a/internal/filtering/filter_internal_test.go +++ b/internal/filtering/filter_internal_test.go @@ -11,6 +11,7 @@ import ( "github.com/AdguardTeam/golibs/netutil/urlutil" "github.com/AdguardTeam/golibs/testutil" + "github.com/c2h5oh/datasize" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -76,6 +77,9 @@ func updateAndAssert( require.NoError(tb, err) } +// testFilterSize is a test size of filters. +const testFilterSize = 10 * datasize.MB + // newDNSFilter returns a new properly initialized DNS filter instance. func newDNSFilter(tb testing.TB) (d *DNSFilter) { tb.Helper() @@ -86,6 +90,7 @@ func newDNSFilter(tb testing.TB) (d *DNSFilter) { HTTPClient: &http.Client{ Timeout: testTimeout, }, + MaxHTTPSize: testFilterSize, }, nil) require.NoError(tb, err) diff --git a/internal/filtering/filtering.go b/internal/filtering/filtering.go index 645c4fe5389..159d216ca41 100644 --- a/internal/filtering/filtering.go +++ b/internal/filtering/filtering.go @@ -32,6 +32,7 @@ import ( "github.com/AdguardTeam/urlfilter" "github.com/AdguardTeam/urlfilter/filterlist" "github.com/AdguardTeam/urlfilter/rules" + "github.com/c2h5oh/datasize" "github.com/miekg/dns" ) @@ -154,6 +155,10 @@ type Config struct { // files can be added. SafeFSPatterns []string `yaml:"safe_fs_patterns"` + // MaxHTTPSize defines the maximum size of the HTTP body. The value must + // not be equal to zero. + MaxHTTPSize datasize.ByteSize `yaml:"max_http_size"` + SafeBrowsingCacheSize uint `yaml:"safebrowsing_cache_size"` // (in bytes) SafeSearchCacheSize uint `yaml:"safesearch_cache_size"` // (in bytes) ParentalCacheSize uint `yaml:"parental_cache_size"` // (in bytes) diff --git a/internal/filtering/http_internal_test.go b/internal/filtering/http_internal_test.go index 4ed66c74050..f6e82ae7159 100644 --- a/internal/filtering/http_internal_test.go +++ b/internal/filtering/http_internal_test.go @@ -119,6 +119,7 @@ func TestDNSFilter_handleFilteringSetURL(t *testing.T) { ConfModifier: confModifier, HTTPReg: aghhttp.EmptyRegistrar{}, DataDir: filtersDir, + MaxHTTPSize: testFilterSize, }, nil) require.NoError(t, err) t.Cleanup(d.Close) diff --git a/internal/filtering/rulelist/rulelist.go b/internal/filtering/rulelist/rulelist.go index 17e8c3cd67b..f185519cd26 100644 --- a/internal/filtering/rulelist/rulelist.go +++ b/internal/filtering/rulelist/rulelist.go @@ -21,7 +21,7 @@ import ( const DefaultRuleBufSize = 1024 // DefaultMaxRuleListSize is the default maximum filtering-rule list size. -const DefaultMaxRuleListSize = 64 * datasize.MB +const DefaultMaxRuleListSize = 256 * datasize.MB // APIID is the type for the rule-list IDs used in the HTTP API. type APIID int64 diff --git a/internal/home/config.go b/internal/home/config.go index ed4e6008fdc..31bfaee8a1f 100644 --- a/internal/home/config.go +++ b/internal/home/config.go @@ -20,6 +20,7 @@ import ( "github.com/AdguardTeam/AdGuardHome/internal/dhcpd" "github.com/AdguardTeam/AdGuardHome/internal/dnsforward" "github.com/AdguardTeam/AdGuardHome/internal/filtering" + "github.com/AdguardTeam/AdGuardHome/internal/filtering/rulelist" "github.com/AdguardTeam/AdGuardHome/internal/querylog" "github.com/AdguardTeam/AdGuardHome/internal/schedule" "github.com/AdguardTeam/AdGuardHome/internal/stats" @@ -564,6 +565,7 @@ var config = &configuration{ ParentalEnabled: false, SafeBrowsingEnabled: false, + MaxHTTPSize: rulelist.DefaultMaxRuleListSize, SafeBrowsingCacheSize: 1 * 1024 * 1024, SafeSearchCacheSize: 1 * 1024 * 1024, ParentalCacheSize: 1 * 1024 * 1024,