From 8ce81f918cc5cf43972e2045532a48c829257a2f Mon Sep 17 00:00:00 2001 From: Ainar Garipov Date: Wed, 2 Oct 2024 18:09:57 +0300 Subject: [PATCH] all: improve permissions, add safe_fs_patterns --- CHANGELOG.md | 39 ++++++ internal/aghos/os.go | 6 + internal/configmigrate/configmigrate.go | 2 +- .../configmigrate/migrations_internal_test.go | 1 + internal/configmigrate/migrator.go | 13 +- internal/configmigrate/migrator_test.go | 6 + .../TestMigrateConfig_Migrate/v29/input.yml | 117 +++++++++++++++++ .../TestMigrateConfig_Migrate/v29/output.yml | 120 ++++++++++++++++++ internal/configmigrate/v29.go | 63 +++++++++ internal/dhcpd/db.go | 3 +- internal/filtering/filter.go | 12 +- internal/filtering/filtering.go | 29 ++++- internal/filtering/http.go | 18 ++- internal/filtering/path.go | 37 ++++++ internal/filtering/path_unix_internal_test.go | 78 ++++++++++++ .../filtering/path_windows_internal_test.go | 73 +++++++++++ internal/filtering/rulelist/filter.go | 5 +- internal/home/auth.go | 3 +- internal/home/config.go | 18 ++- internal/home/controlinstall.go | 9 +- internal/home/dns.go | 7 +- internal/home/home.go | 25 +++- internal/next/configmgr/configmgr.go | 3 +- internal/permcheck/migrate.go | 93 ++++++++++++++ internal/permcheck/permcheck.go | 86 +++++++++++++ internal/querylog/qlogfile.go | 3 +- internal/querylog/querylogfile.go | 3 +- internal/stats/stats.go | 3 +- internal/updater/updater.go | 9 +- scripts/install.sh | 2 +- 30 files changed, 835 insertions(+), 51 deletions(-) create mode 100644 internal/configmigrate/testdata/TestMigrateConfig_Migrate/v29/input.yml create mode 100644 internal/configmigrate/testdata/TestMigrateConfig_Migrate/v29/output.yml create mode 100644 internal/configmigrate/v29.go create mode 100644 internal/filtering/path.go create mode 100644 internal/filtering/path_unix_internal_test.go create mode 100644 internal/filtering/path_windows_internal_test.go create mode 100644 internal/permcheck/migrate.go create mode 100644 internal/permcheck/permcheck.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 7eda9d3b..800a7c4f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,20 @@ NOTE: Add new changes BELOW THIS COMMENT. ### Security +- Previuos versions of AdGuard Home allowed users to add any system it had + access to as filters, exposing them to be world-readable. To prevent this, + AdGuard Home now allows adding filtering-rule list files only from files + matching the patterns enumerated in the `filtering.safe_fs_patterns` property + in the configuration file. + + We thank @itz-d0dgy for reporting this vulnerability, designated + CVE-2024-36814, to us. +- Additionally, AdGuard Home will now try to change the permissions of its files + and directories to more restrictive ones to prevent similar vulnerabilities + as well as limit the access to the configuration. + + We thank @go-compile for reporting this vulnerability, designated + CVE-2024-36586, to us. - Go version has been updated to prevent the possibility of exploiting the Go vulnerabilities fixed in [1.23.2][go-1.23.2]. @@ -42,6 +56,15 @@ NOTE: Add new changes BELOW THIS COMMENT. - Upstream server URL domain names requirements has been relaxed and now follow the same rules as their domain specifications. +#### Configuration changes + +In this release, the schema version has changed from 28 to 29. + +- The new array `filtering.safe_fs_patterns` contains glob patterns for paths of + files that can be added as local filtering-rule lists. The migration should + add list files that have already been added, as well as the default value, + `$DATA_DIR/userfilters/*`. + ### Fixed - Property `clients.runtime_sources.dhcp` in the configuration file not taking @@ -50,6 +73,22 @@ NOTE: Add new changes BELOW THIS COMMENT. - Enforce Bing safe search from Edge sidebar ([#7154]). - Text overflow on the query log page ([#7119]). +### Known issues + +- Due to the complexity of the Windows permissions architecture and poor support + from the standard Go library, we have to postpone the proper automated Windows + fix until the next release. + + **Temporary workaround:** Set the permissions of the `AdGuardHome` directory + to more restrictive ones manually. To do that: + + 1. Locate the `AdGuardHome` directory. + 2. Right-click on it and navigate to _Properties → Security → Advanced._ + 3. (You might need to disable permission inheritance to make them more + restricted.) + 4. Adjust to give the `Full control` access to only the user which runs + AdGuard Home. Typically, `Administrator`. + [#5009]: https://github.com/AdguardTeam/AdGuardHome/issues/5009 [#5704]: https://github.com/AdguardTeam/AdGuardHome/issues/5704 [#7119]: https://github.com/AdguardTeam/AdGuardHome/issues/7119 diff --git a/internal/aghos/os.go b/internal/aghos/os.go index f2933a98..2ac99464 100644 --- a/internal/aghos/os.go +++ b/internal/aghos/os.go @@ -19,6 +19,12 @@ import ( "github.com/AdguardTeam/golibs/log" ) +// Default file and directory permissions. +const ( + DefaultPermDir = 0o700 + DefaultPermFile = 0o600 +) + // Unsupported is a helper that returns a wrapped [errors.ErrUnsupported]. func Unsupported(op string) (err error) { return fmt.Errorf("%s: not supported on %s: %w", op, runtime.GOOS, errors.ErrUnsupported) diff --git a/internal/configmigrate/configmigrate.go b/internal/configmigrate/configmigrate.go index 6e8845e0..cba61247 100644 --- a/internal/configmigrate/configmigrate.go +++ b/internal/configmigrate/configmigrate.go @@ -2,4 +2,4 @@ package configmigrate // LastSchemaVersion is the most recent schema version. -const LastSchemaVersion uint = 28 +const LastSchemaVersion uint = 29 diff --git a/internal/configmigrate/migrations_internal_test.go b/internal/configmigrate/migrations_internal_test.go index 5349102f..34bbb847 100644 --- a/internal/configmigrate/migrations_internal_test.go +++ b/internal/configmigrate/migrations_internal_test.go @@ -19,6 +19,7 @@ func TestUpgradeSchema1to2(t *testing.T) { m := New(&Config{ WorkingDir: "", + DataDir: "", }) err := m.migrateTo2(diskConf) diff --git a/internal/configmigrate/migrator.go b/internal/configmigrate/migrator.go index ebdf6ba7..70a80f8b 100644 --- a/internal/configmigrate/migrator.go +++ b/internal/configmigrate/migrator.go @@ -10,20 +10,24 @@ import ( // Config is a the configuration for initializing a [Migrator]. type Config struct { - // WorkingDir is an absolute path to the working directory of AdGuardHome. + // WorkingDir is the absolute path to the working directory of AdGuardHome. WorkingDir string + + // DataDir is the absolute path to the data directory of AdGuardHome. + DataDir string } // Migrator performs the YAML configuration file migrations. type Migrator struct { - // workingDir is an absolute path to the working directory of AdGuardHome. workingDir string + dataDir string } // New creates a new Migrator. -func New(cfg *Config) (m *Migrator) { +func New(c *Config) (m *Migrator) { return &Migrator{ - workingDir: cfg.WorkingDir, + workingDir: c.WorkingDir, + dataDir: c.DataDir, } } @@ -120,6 +124,7 @@ func (m *Migrator) upgradeConfigSchema(current, target uint, diskConf yobj) (err 25: migrateTo26, 26: migrateTo27, 27: migrateTo28, + 28: m.migrateTo29, } for i, migrate := range upgrades[current:target] { diff --git a/internal/configmigrate/migrator_test.go b/internal/configmigrate/migrator_test.go index 442713a4..cc6672ad 100644 --- a/internal/configmigrate/migrator_test.go +++ b/internal/configmigrate/migrator_test.go @@ -4,6 +4,7 @@ import ( "io/fs" "os" "path" + "path/filepath" "testing" "github.com/AdguardTeam/AdGuardHome/internal/configmigrate" @@ -190,6 +191,10 @@ func TestMigrateConfig_Migrate(t *testing.T) { yamlEqFunc: require.YAMLEq, name: "v27", targetVersion: 27, + }, { + yamlEqFunc: require.YAMLEq, + name: "v29", + targetVersion: 29, }} for _, tc := range testCases { @@ -202,6 +207,7 @@ func TestMigrateConfig_Migrate(t *testing.T) { migrator := configmigrate.New(&configmigrate.Config{ WorkingDir: t.Name(), + DataDir: filepath.Join(t.Name(), "data"), }) newBody, upgraded, err := migrator.Migrate(body, tc.targetVersion) require.NoError(t, err) diff --git a/internal/configmigrate/testdata/TestMigrateConfig_Migrate/v29/input.yml b/internal/configmigrate/testdata/TestMigrateConfig_Migrate/v29/input.yml new file mode 100644 index 00000000..909fdd5f --- /dev/null +++ b/internal/configmigrate/testdata/TestMigrateConfig_Migrate/v29/input.yml @@ -0,0 +1,117 @@ +http: + address: 127.0.0.1:3000 + session_ttl: 3h + pprof: + enabled: true + port: 6060 +users: +- name: testuser + password: testpassword +dns: + bind_hosts: + - 127.0.0.1 + port: 53 + parental_sensitivity: 0 + upstream_dns: + - tls://1.1.1.1 + - tls://1.0.0.1 + - quic://8.8.8.8:784 + bootstrap_dns: + - 8.8.8.8:53 + edns_client_subnet: + enabled: true + use_custom: false + custom_ip: "" +filtering: + filtering_enabled: true + parental_enabled: false + safebrowsing_enabled: false + safe_search: + enabled: false + bing: true + duckduckgo: true + google: true + pixabay: true + yandex: true + youtube: true + protection_enabled: true + blocked_services: + schedule: + time_zone: Local + ids: + - 500px + blocked_response_ttl: 10 +filters: +- url: https://adaway.org/hosts.txt + name: AdAway + enabled: false +- url: /path/to/file.txt + name: Local Filter + enabled: false +clients: + persistent: + - name: localhost + ids: + - 127.0.0.1 + - aa:aa:aa:aa:aa:aa + use_global_settings: true + use_global_blocked_services: true + filtering_enabled: false + parental_enabled: false + safebrowsing_enabled: false + safe_search: + enabled: true + bing: true + duckduckgo: true + google: true + pixabay: true + yandex: true + youtube: true + blocked_services: + schedule: + time_zone: Local + ids: + - 500px + runtime_sources: + whois: true + arp: true + rdns: true + dhcp: true + hosts: true +dhcp: + enabled: false + interface_name: vboxnet0 + local_domain_name: local + dhcpv4: + gateway_ip: 192.168.0.1 + subnet_mask: 255.255.255.0 + range_start: 192.168.0.10 + range_end: 192.168.0.250 + lease_duration: 1234 + icmp_timeout_msec: 10 +schema_version: 28 +user_rules: [] +querylog: + enabled: true + file_enabled: true + interval: 720h + size_memory: 1000 + ignored: + - '|.^' +statistics: + enabled: true + interval: 240h + ignored: + - '|.^' +os: + group: '' + rlimit_nofile: 123 + user: '' +log: + file: "" + max_backups: 0 + max_size: 100 + max_age: 3 + compress: true + local_time: false + verbose: true diff --git a/internal/configmigrate/testdata/TestMigrateConfig_Migrate/v29/output.yml b/internal/configmigrate/testdata/TestMigrateConfig_Migrate/v29/output.yml new file mode 100644 index 00000000..e698551b --- /dev/null +++ b/internal/configmigrate/testdata/TestMigrateConfig_Migrate/v29/output.yml @@ -0,0 +1,120 @@ +http: + address: 127.0.0.1:3000 + session_ttl: 3h + pprof: + enabled: true + port: 6060 +users: +- name: testuser + password: testpassword +dns: + bind_hosts: + - 127.0.0.1 + port: 53 + parental_sensitivity: 0 + upstream_dns: + - tls://1.1.1.1 + - tls://1.0.0.1 + - quic://8.8.8.8:784 + bootstrap_dns: + - 8.8.8.8:53 + edns_client_subnet: + enabled: true + use_custom: false + custom_ip: "" +filtering: + filtering_enabled: true + parental_enabled: false + safebrowsing_enabled: false + safe_fs_patterns: + - TestMigrateConfig_Migrate/v29/data/userfilters/* + - /path/to/file.txt + safe_search: + enabled: false + bing: true + duckduckgo: true + google: true + pixabay: true + yandex: true + youtube: true + protection_enabled: true + blocked_services: + schedule: + time_zone: Local + ids: + - 500px + blocked_response_ttl: 10 +filters: +- url: https://adaway.org/hosts.txt + name: AdAway + enabled: false +- url: /path/to/file.txt + name: Local Filter + enabled: false +clients: + persistent: + - name: localhost + ids: + - 127.0.0.1 + - aa:aa:aa:aa:aa:aa + use_global_settings: true + use_global_blocked_services: true + filtering_enabled: false + parental_enabled: false + safebrowsing_enabled: false + safe_search: + enabled: true + bing: true + duckduckgo: true + google: true + pixabay: true + yandex: true + youtube: true + blocked_services: + schedule: + time_zone: Local + ids: + - 500px + runtime_sources: + whois: true + arp: true + rdns: true + dhcp: true + hosts: true +dhcp: + enabled: false + interface_name: vboxnet0 + local_domain_name: local + dhcpv4: + gateway_ip: 192.168.0.1 + subnet_mask: 255.255.255.0 + range_start: 192.168.0.10 + range_end: 192.168.0.250 + lease_duration: 1234 + icmp_timeout_msec: 10 +schema_version: 29 +user_rules: [] +querylog: + enabled: true + file_enabled: true + interval: 720h + size_memory: 1000 + ignored: + - '|.^' +statistics: + enabled: true + interval: 240h + ignored: + - '|.^' +os: + group: '' + rlimit_nofile: 123 + user: '' +log: + file: "" + max_backups: 0 + max_size: 100 + max_age: 3 + compress: true + local_time: false + verbose: true diff --git a/internal/configmigrate/v29.go b/internal/configmigrate/v29.go new file mode 100644 index 00000000..fcd04bef --- /dev/null +++ b/internal/configmigrate/v29.go @@ -0,0 +1,63 @@ +package configmigrate + +import ( + "fmt" + "path/filepath" +) + +// migrateTo29 performs the following changes: +// +// # BEFORE: +// 'filters': +// - 'enabled': true +// 'url': /path/to/file.txt +// 'name': My FS Filter +// 'id': 1234 +// +// # AFTER: +// 'filters': +// - 'enabled': true +// 'url': /path/to/file.txt +// 'name': My FS Filter +// 'id': 1234 +// # … +// 'filtering': +// 'safe_fs_patterns': +// - '/opt/AdGuardHome/data/userfilters/*' +// - '/path/to/file.txt' +// # … +func (m Migrator) migrateTo29(diskConf yobj) (err error) { + diskConf["schema_version"] = 29 + + filterVals, ok, err := fieldVal[[]any](diskConf, "filters") + if !ok { + return err + } + + paths := []string{ + filepath.Join(m.dataDir, "userfilters", "*"), + } + + for i, v := range filterVals { + var f yobj + f, ok = v.(yobj) + if !ok { + return fmt.Errorf("filters: at index %d: expected object, got %T", i, v) + } + + var u string + u, ok, _ = fieldVal[string](f, "url") + if ok && filepath.IsAbs(u) { + paths = append(paths, u) + } + } + + fltConf, ok, err := fieldVal[yobj](diskConf, "filtering") + if !ok { + return err + } + + fltConf["safe_fs_patterns"] = paths + + return nil +} diff --git a/internal/dhcpd/db.go b/internal/dhcpd/db.go index aa482f2e..d3ca53bf 100644 --- a/internal/dhcpd/db.go +++ b/internal/dhcpd/db.go @@ -12,6 +12,7 @@ import ( "strings" "time" + "github.com/AdguardTeam/AdGuardHome/internal/aghos" "github.com/AdguardTeam/AdGuardHome/internal/dhcpsvc" "github.com/AdguardTeam/golibs/errors" "github.com/AdguardTeam/golibs/log" @@ -185,7 +186,7 @@ func writeDB(path string, leases []*dbLease) (err error) { return err } - err = maybe.WriteFile(path, buf, 0o644) + err = maybe.WriteFile(path, buf, aghos.DefaultPermFile) if err != nil { // Don't wrap the error since it's informative enough as is. return err diff --git a/internal/filtering/filter.go b/internal/filtering/filter.go index 15bc14d9..0dd3471c 100644 --- a/internal/filtering/filter.go +++ b/internal/filtering/filter.go @@ -11,6 +11,7 @@ import ( "strings" "time" + "github.com/AdguardTeam/AdGuardHome/internal/aghos" "github.com/AdguardTeam/AdGuardHome/internal/aghrenameio" "github.com/AdguardTeam/AdGuardHome/internal/filtering/rulelist" "github.com/AdguardTeam/golibs/container" @@ -448,11 +449,7 @@ func (d *DNSFilter) updateIntl(flt *FilterYAML) (ok bool, err error) { var res *rulelist.ParseResult - // Change the default 0o600 permission to something more acceptable by end - // users. - // - // See https://github.com/AdguardTeam/AdGuardHome/issues/3198. - tmpFile, err := aghrenameio.NewPendingFile(flt.Path(d.conf.DataDir), 0o644) + tmpFile, err := aghrenameio.NewPendingFile(flt.Path(d.conf.DataDir), aghos.DefaultPermFile) if err != nil { return false, err } @@ -522,6 +519,11 @@ func (d *DNSFilter) reader(fltURL string) (r io.ReadCloser, err error) { 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) diff --git a/internal/filtering/filtering.go b/internal/filtering/filtering.go index 55404b74..8836515c 100644 --- a/internal/filtering/filtering.go +++ b/internal/filtering/filtering.go @@ -19,6 +19,7 @@ import ( "time" "github.com/AdguardTeam/AdGuardHome/internal/aghhttp" + "github.com/AdguardTeam/AdGuardHome/internal/aghos" "github.com/AdguardTeam/AdGuardHome/internal/filtering/rulelist" "github.com/AdguardTeam/golibs/container" "github.com/AdguardTeam/golibs/errors" @@ -130,6 +131,10 @@ type Config struct { // UserRules is the global list of custom rules. UserRules []string `yaml:"-"` + // SafeFSPatterns are the patterns for matching which local filtering-rule + // files can be added. + SafeFSPatterns []string `yaml:"safe_fs_patterns"` + SafeBrowsingCacheSize uint `yaml:"safebrowsing_cache_size"` // (in bytes) SafeSearchCacheSize uint `yaml:"safesearch_cache_size"` // (in bytes) ParentalCacheSize uint `yaml:"parental_cache_size"` // (in bytes) @@ -257,6 +262,8 @@ type DNSFilter struct { refreshLock *sync.Mutex hostCheckers []hostChecker + + safeFSPatterns []string } // Filter represents a filter list @@ -987,13 +994,22 @@ func New(c *Config, blockFilters []Filter) (d *DNSFilter, err error) { d = &DNSFilter{ idGen: newIDGenerator(int32(time.Now().Unix())), bufPool: syncutil.NewSlicePool[byte](rulelist.DefaultRuleBufSize), + safeSearch: c.SafeSearch, refreshLock: &sync.Mutex{}, safeBrowsingChecker: c.SafeBrowsingChecker, parentalControlChecker: c.ParentalControlChecker, confMu: &sync.RWMutex{}, } - d.safeSearch = c.SafeSearch + for i, p := range c.SafeFSPatterns { + // Use Match to validate the patterns here. + _, err = filepath.Match(p, "test") + if err != nil { + return nil, fmt.Errorf("safe_fs_patterns: at index %d: %w", i, err) + } + + d.safeFSPatterns = append(d.safeFSPatterns, p) + } d.hostCheckers = []hostChecker{{ check: d.matchSysHosts, @@ -1022,7 +1038,7 @@ func New(c *Config, blockFilters []Filter) (d *DNSFilter, err error) { err = d.prepareRewrites() if err != nil { - return nil, fmt.Errorf("rewrites: preparing: %s", err) + return nil, fmt.Errorf("rewrites: preparing: %w", err) } if d.conf.BlockedServices != nil { @@ -1037,11 +1053,16 @@ func New(c *Config, blockFilters []Filter) (d *DNSFilter, err error) { if err != nil { d.Close() - return nil, fmt.Errorf("initializing filtering subsystem: %s", err) + return nil, fmt.Errorf("initializing filtering subsystem: %w", err) } } - _ = os.MkdirAll(filepath.Join(d.conf.DataDir, filterDir), 0o755) + err = os.MkdirAll(filepath.Join(d.conf.DataDir, filterDir), aghos.DefaultPermDir) + if err != nil { + d.Close() + + return nil, fmt.Errorf("making filtering directory: %w", err) + } d.loadFilters(d.conf.Filters) d.loadFilters(d.conf.WhitelistFilters) diff --git a/internal/filtering/http.go b/internal/filtering/http.go index 3a13ccb3..e459cb2f 100644 --- a/internal/filtering/http.go +++ b/internal/filtering/http.go @@ -20,14 +20,22 @@ import ( ) // validateFilterURL validates the filter list URL or file name. -func validateFilterURL(urlStr string) (err error) { +func (d *DNSFilter) validateFilterURL(urlStr string) (err error) { defer func() { err = errors.Annotate(err, "checking filter: %w") }() if filepath.IsAbs(urlStr) { + urlStr = filepath.Clean(urlStr) _, err = os.Stat(urlStr) + if err != nil { + // Don't wrap the error since it's informative enough as is. + return err + } - // Don't wrap the error since it's informative enough as is. - return err + if !pathMatchesAny(d.safeFSPatterns, urlStr) { + return fmt.Errorf("path %q does not match safe patterns", urlStr) + } + + return nil } u, err := url.ParseRequestURI(urlStr) @@ -65,7 +73,7 @@ func (d *DNSFilter) handleFilteringAddURL(w http.ResponseWriter, r *http.Request return } - err = validateFilterURL(fj.URL) + err = d.validateFilterURL(fj.URL) if err != nil { aghhttp.Error(r, w, http.StatusBadRequest, "%s", err) @@ -225,7 +233,7 @@ func (d *DNSFilter) handleFilteringSetURL(w http.ResponseWriter, r *http.Request return } - err = validateFilterURL(fj.Data.URL) + err = d.validateFilterURL(fj.Data.URL) if err != nil { aghhttp.Error(r, w, http.StatusBadRequest, "invalid url: %s", err) diff --git a/internal/filtering/path.go b/internal/filtering/path.go new file mode 100644 index 00000000..a52a6ebe --- /dev/null +++ b/internal/filtering/path.go @@ -0,0 +1,37 @@ +package filtering + +import ( + "fmt" + "path/filepath" +) + +// pathMatchesAny returns true if filePath matches one of globs. globs must be +// valid. filePath must be absolute and clean. If globs are empty, +// pathMatchesAny returns false. +// +// TODO(a.garipov): Move to golibs? +func pathMatchesAny(globs []string, filePath string) (ok bool) { + if len(globs) == 0 { + return false + } + + clean, err := filepath.Abs(filePath) + if err != nil { + panic(fmt.Errorf("pathMatchesAny: %w", err)) + } else if clean != filePath { + panic(fmt.Errorf("pathMatchesAny: filepath %q is not absolute", filePath)) + } + + for _, g := range globs { + ok, err = filepath.Match(g, filePath) + if err != nil { + panic(fmt.Errorf("pathMatchesAny: bad pattern: %w", err)) + } + + if ok { + return true + } + } + + return false +} diff --git a/internal/filtering/path_unix_internal_test.go b/internal/filtering/path_unix_internal_test.go new file mode 100644 index 00000000..09139979 --- /dev/null +++ b/internal/filtering/path_unix_internal_test.go @@ -0,0 +1,78 @@ +//go:build unix + +package filtering + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestPathInAnyDir(t *testing.T) { + t.Parallel() + + const ( + filePath = "/path/to/file.txt" + filePathGlob = "/path/to/*" + otherFilePath = "/otherpath/to/file.txt" + ) + + testCases := []struct { + want assert.BoolAssertionFunc + filePath string + name string + globs []string + }{{ + want: assert.False, + filePath: filePath, + name: "nil_pats", + globs: nil, + }, { + want: assert.True, + filePath: filePath, + name: "match", + globs: []string{ + filePath, + otherFilePath, + }, + }, { + want: assert.False, + filePath: filePath, + name: "no_match", + globs: []string{ + otherFilePath, + }, + }, { + want: assert.True, + filePath: filePath, + name: "match_star", + globs: []string{ + filePathGlob, + }, + }} + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + tc.want(t, pathMatchesAny(tc.globs, tc.filePath)) + }) + } + + require.True(t, t.Run("panic_on_unabs_file_path", func(t *testing.T) { + t.Parallel() + + assert.Panics(t, func() { + _ = pathMatchesAny([]string{"/home/user"}, "../../etc/passwd") + }) + })) + + require.True(t, t.Run("panic_on_bad_pat", func(t *testing.T) { + t.Parallel() + + assert.Panics(t, func() { + _ = pathMatchesAny([]string{`\`}, filePath) + }) + })) +} diff --git a/internal/filtering/path_windows_internal_test.go b/internal/filtering/path_windows_internal_test.go new file mode 100644 index 00000000..cbabc243 --- /dev/null +++ b/internal/filtering/path_windows_internal_test.go @@ -0,0 +1,73 @@ +//go:build windows + +package filtering + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestPathInAnyDir(t *testing.T) { + t.Parallel() + + const ( + filePath = `C:\path\to\file.txt` + filePathGlob = `C:\path\to\*` + otherFilePath = `C:\otherpath\to\file.txt` + ) + + testCases := []struct { + want assert.BoolAssertionFunc + filePath string + name string + globs []string + }{{ + want: assert.False, + filePath: filePath, + name: "nil_pats", + globs: nil, + }, { + want: assert.True, + filePath: filePath, + name: "match", + globs: []string{ + filePath, + otherFilePath, + }, + }, { + want: assert.False, + filePath: filePath, + name: "no_match", + globs: []string{ + otherFilePath, + }, + }, { + want: assert.True, + filePath: filePath, + name: "match_star", + globs: []string{ + filePathGlob, + }, + }} + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + tc.want(t, pathMatchesAny(tc.globs, tc.filePath)) + }) + } + + require.True(t, t.Run("panic_on_unabs_file_path", func(t *testing.T) { + t.Parallel() + + assert.Panics(t, func() { + _ = pathMatchesAny([]string{`C:\home\user`}, `..\..\etc\passwd`) + }) + })) + + // TODO(a.garipov): See if there is anything for which filepath.Match + // returns ErrBadPattern on Windows. +} diff --git a/internal/filtering/rulelist/filter.go b/internal/filtering/rulelist/filter.go index 5f3fa6be..a29897de 100644 --- a/internal/filtering/rulelist/filter.go +++ b/internal/filtering/rulelist/filter.go @@ -11,6 +11,7 @@ import ( "path/filepath" "time" + "github.com/AdguardTeam/AdGuardHome/internal/aghos" "github.com/AdguardTeam/AdGuardHome/internal/aghrenameio" "github.com/AdguardTeam/golibs/errors" "github.com/AdguardTeam/golibs/ioutil" @@ -196,7 +197,7 @@ func (f *Filter) readFromHTTP( return "", nil, fmt.Errorf("got status code %d, want %d", resp.StatusCode, http.StatusOK) } - fltFile, err := aghrenameio.NewPendingFile(cachePath, 0o644) + fltFile, err := aghrenameio.NewPendingFile(cachePath, aghos.DefaultPermFile) if err != nil { return "", nil, fmt.Errorf("creating temp file: %w", err) } @@ -271,7 +272,7 @@ func parseIntoCache( filePath string, cachePath string, ) (parseRes *ParseResult, err error) { - tmpFile, err := aghrenameio.NewPendingFile(cachePath, 0o644) + tmpFile, err := aghrenameio.NewPendingFile(cachePath, aghos.DefaultPermFile) if err != nil { return nil, fmt.Errorf("creating temp file: %w", err) } diff --git a/internal/home/auth.go b/internal/home/auth.go index 18bc5668..5b6cf9b8 100644 --- a/internal/home/auth.go +++ b/internal/home/auth.go @@ -9,6 +9,7 @@ import ( "sync" "time" + "github.com/AdguardTeam/AdGuardHome/internal/aghos" "github.com/AdguardTeam/golibs/errors" "github.com/AdguardTeam/golibs/log" "github.com/AdguardTeam/golibs/netutil" @@ -89,7 +90,7 @@ func InitAuth( trustedProxies: trustedProxies, } var err error - a.db, err = bbolt.Open(dbFilename, 0o644, nil) + a.db, err = bbolt.Open(dbFilename, aghos.DefaultPermFile, nil) if err != nil { log.Error("auth: open DB: %s: %s", dbFilename, err) if err.Error() == "invalid argument" { diff --git a/internal/home/config.go b/internal/home/config.go index b8d69dae..20d2eb97 100644 --- a/internal/home/config.go +++ b/internal/home/config.go @@ -9,6 +9,7 @@ import ( "sync" "github.com/AdguardTeam/AdGuardHome/internal/aghalg" + "github.com/AdguardTeam/AdGuardHome/internal/aghos" "github.com/AdguardTeam/AdGuardHome/internal/aghtls" "github.com/AdguardTeam/AdGuardHome/internal/configmigrate" "github.com/AdguardTeam/AdGuardHome/internal/dhcpd" @@ -26,9 +27,15 @@ import ( yaml "gopkg.in/yaml.v3" ) -// dataDir is the name of a directory under the working one to store some -// persistent data. -const dataDir = "data" +const ( + // dataDir is the name of a directory under the working one to store some + // persistent data. + dataDir = "data" + + // userFilterDataDir is the name of the directory used to store users' + // FS-based rule lists. + userFilterDataDir = "userfilters" +) // logSettings are the logging settings part of the configuration file. type logSettings struct { @@ -520,6 +527,7 @@ func parseConfig() (err error) { migrator := configmigrate.New(&configmigrate.Config{ WorkingDir: Context.workDir, + DataDir: Context.getDataDir(), }) var upgraded bool @@ -534,7 +542,7 @@ func parseConfig() (err error) { confPath := configFilePath() log.Debug("writing config file %q after config upgrade", confPath) - err = maybe.WriteFile(confPath, config.fileData, 0o644) + err = maybe.WriteFile(confPath, config.fileData, aghos.DefaultPermFile) if err != nil { return fmt.Errorf("writing new config: %w", err) } @@ -700,7 +708,7 @@ func (c *configuration) write() (err error) { return fmt.Errorf("generating config file: %w", err) } - err = maybe.WriteFile(confPath, buf.Bytes(), 0o644) + err = maybe.WriteFile(confPath, buf.Bytes(), aghos.DefaultPermFile) if err != nil { return fmt.Errorf("writing config file: %w", err) } diff --git a/internal/home/controlinstall.go b/internal/home/controlinstall.go index f94457d0..3051f0f6 100644 --- a/internal/home/controlinstall.go +++ b/internal/home/controlinstall.go @@ -270,9 +270,9 @@ DNSStubListener=no const resolvConfPath = "/etc/resolv.conf" // Deactivate DNSStubListener -func disableDNSStubListener() error { +func disableDNSStubListener() (err error) { dir := filepath.Dir(resolvedConfPath) - err := os.MkdirAll(dir, 0o755) + err = os.MkdirAll(dir, 0o755) if err != nil { return fmt.Errorf("os.MkdirAll: %s: %w", dir, err) } @@ -413,9 +413,12 @@ func (web *webAPI) handleInstallConfigure(w http.ResponseWriter, r *http.Request copyInstallSettings(curConfig, config) Context.firstRun = false - config.HTTPConfig.Address = netip.AddrPortFrom(req.Web.IP, req.Web.Port) config.DNS.BindHosts = []netip.Addr{req.DNS.IP} config.DNS.Port = req.DNS.Port + config.Filtering.SafeFSPatterns = []string{ + filepath.Join(Context.workDir, userFilterDataDir, "*"), + } + config.HTTPConfig.Address = netip.AddrPortFrom(req.Web.IP, req.Web.Port) u := &webUser{ Name: req.Username, diff --git a/internal/home/dns.go b/internal/home/dns.go index 9dd711f5..f0c6d4de 100644 --- a/internal/home/dns.go +++ b/internal/home/dns.go @@ -47,14 +47,9 @@ func onConfigModified() { // initDNS updates all the fields of the [Context] needed to initialize the DNS // server and initializes it at last. It also must not be called unless // [config] and [Context] are initialized. l must not be nil. -func initDNS(l *slog.Logger) (err error) { +func initDNS(l *slog.Logger, statsDir, querylogDir string) (err error) { anonymizer := config.anonymizer() - statsDir, querylogDir, err := checkStatsAndQuerylogDirs(&Context, config) - if err != nil { - return err - } - statsConf := stats.Config{ Logger: l.With(slogutil.KeyPrefix, "stats"), Filename: filepath.Join(statsDir, "stats.db"), diff --git a/internal/home/home.go b/internal/home/home.go index 69a67223..df4e4296 100644 --- a/internal/home/home.go +++ b/internal/home/home.go @@ -31,6 +31,7 @@ import ( "github.com/AdguardTeam/AdGuardHome/internal/filtering" "github.com/AdguardTeam/AdGuardHome/internal/filtering/hashprefix" "github.com/AdguardTeam/AdGuardHome/internal/filtering/safesearch" + "github.com/AdguardTeam/AdGuardHome/internal/permcheck" "github.com/AdguardTeam/AdGuardHome/internal/querylog" "github.com/AdguardTeam/AdGuardHome/internal/stats" "github.com/AdguardTeam/AdGuardHome/internal/updater" @@ -630,9 +631,9 @@ func run(opts options, clientBuildFS fs.FS, done chan struct{}) { } } - dir := Context.getDataDir() - err = os.MkdirAll(dir, 0o755) - fatalOnError(errors.Annotate(err, "creating DNS data dir at %s: %w", dir)) + dataDir := Context.getDataDir() + err = os.MkdirAll(dataDir, aghos.DefaultPermDir) + fatalOnError(errors.Annotate(err, "creating DNS data dir at %s: %w", dataDir)) GLMode = opts.glinetMode @@ -649,8 +650,11 @@ func run(opts options, clientBuildFS fs.FS, done chan struct{}) { Context.web, err = initWeb(opts, clientBuildFS, upd, slogLogger) fatalOnError(err) + statsDir, querylogDir, err := checkStatsAndQuerylogDirs(&Context, config) + fatalOnError(err) + if !Context.firstRun { - err = initDNS(slogLogger) + err = initDNS(slogLogger, statsDir, querylogDir) fatalOnError(err) Context.tls.start() @@ -671,6 +675,12 @@ func run(opts options, clientBuildFS fs.FS, done chan struct{}) { } } + if permcheck.NeedsMigration(confPath) { + permcheck.Migrate(Context.workDir, dataDir, statsDir, querylogDir, confPath) + } + + permcheck.Check(Context.workDir, dataDir, statsDir, querylogDir, confPath) + Context.web.start() // Wait for other goroutines to complete their job. @@ -714,7 +724,12 @@ func (c *configuration) anonymizer() (ipmut *aghnet.IPMut) { // startMods initializes and starts the DNS server after installation. l must // not be nil. func startMods(l *slog.Logger) (err error) { - err = initDNS(l) + statsDir, querylogDir, err := checkStatsAndQuerylogDirs(&Context, config) + if err != nil { + return err + } + + err = initDNS(l, statsDir, querylogDir) if err != nil { return err } diff --git a/internal/next/configmgr/configmgr.go b/internal/next/configmgr/configmgr.go index 84e4e61a..a75f1304 100644 --- a/internal/next/configmgr/configmgr.go +++ b/internal/next/configmgr/configmgr.go @@ -14,6 +14,7 @@ import ( "sync" "time" + "github.com/AdguardTeam/AdGuardHome/internal/aghos" "github.com/AdguardTeam/AdGuardHome/internal/next/agh" "github.com/AdguardTeam/AdGuardHome/internal/next/dnssvc" "github.com/AdguardTeam/AdGuardHome/internal/next/websvc" @@ -182,7 +183,7 @@ func (m *Manager) write() (err error) { return fmt.Errorf("encoding: %w", err) } - err = maybe.WriteFile(m.fileName, b, 0o644) + err = maybe.WriteFile(m.fileName, b, aghos.DefaultPermFile) if err != nil { return fmt.Errorf("writing: %w", err) } diff --git a/internal/permcheck/migrate.go b/internal/permcheck/migrate.go new file mode 100644 index 00000000..b052bffa --- /dev/null +++ b/internal/permcheck/migrate.go @@ -0,0 +1,93 @@ +package permcheck + +import ( + "io/fs" + "os" + "path/filepath" + + "github.com/AdguardTeam/AdGuardHome/internal/aghos" + "github.com/AdguardTeam/golibs/errors" + "github.com/AdguardTeam/golibs/log" +) + +// NeedsMigration returns true if AdGuard Home files need permission migration. +// +// TODO(a.garipov): Consider ways to detect this better. +func NeedsMigration(confFilePath string) (ok bool) { + s, err := os.Stat(confFilePath) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + // Likely a first run. Don't check. + return false + } + + log.Error("permcheck: checking if files need migration: %s", err) + + // Unexpected error. Try to migrate just in case. + return true + } + + return s.Mode().Perm() != aghos.DefaultPermFile +} + +// Migrate attempts to change the permissions of AdGuard Home's files. It logs +// the results at an appropriate level. +func Migrate(workDir, dataDir, statsDir, querylogDir, confFilePath string) { + chmodDir(workDir) + + chmodFile(confFilePath) + + // TODO(a.garipov): Put all paths in one place and remove this duplication. + chmodDir(dataDir) + chmodDir(filepath.Join(dataDir, "filters")) + chmodFile(filepath.Join(dataDir, "sessions.db")) + chmodFile(filepath.Join(dataDir, "leases.json")) + + if dataDir != querylogDir { + chmodDir(querylogDir) + } + chmodFile(filepath.Join(querylogDir, "querylog.json")) + chmodFile(filepath.Join(querylogDir, "querylog.json.1")) + + if dataDir != statsDir { + chmodDir(statsDir) + } + chmodFile(filepath.Join(statsDir, "stats.db")) +} + +// chmodDir changes the permissions of a single directory. The results are +// logged at the appropriate level. +func chmodDir(dirPath string) { + chmodPath(dirPath, typeDir, aghos.DefaultPermDir) +} + +// chmodFile changes the permissions of a single file. The results are logged +// at the appropriate level. +func chmodFile(filePath string) { + chmodPath(filePath, typeFile, aghos.DefaultPermFile) +} + +// chmodPath changes the permissions of a single filesystem entity. The results +// are logged at the appropriate level. +func chmodPath(entPath, fileType string, fm fs.FileMode) { + err := os.Chmod(entPath, fm) + if err == nil { + log.Info("permcheck: changed permissions for %s %q", fileType, entPath) + + return + } else if errors.Is(err, os.ErrNotExist) { + log.Debug("permcheck: changing permissions for %s %q: %s", fileType, entPath, err) + + return + } + + log.Error( + "permcheck: SECURITY WARNING: cannot change permissions for %s %q to %#o: %s; "+ + "this can leave your system vulnerable, see "+ + "https://adguard-dns.io/kb/adguard-home/running-securely/#os-service-concerns", + fileType, + entPath, + fm, + err, + ) +} diff --git a/internal/permcheck/permcheck.go b/internal/permcheck/permcheck.go new file mode 100644 index 00000000..aea4a743 --- /dev/null +++ b/internal/permcheck/permcheck.go @@ -0,0 +1,86 @@ +// Package permcheck contains code for simplifying permissions checks on files +// and directories. +// +// TODO(a.garipov): Improve the approach on Windows. +package permcheck + +import ( + "io/fs" + "os" + "path/filepath" + + "github.com/AdguardTeam/AdGuardHome/internal/aghos" + "github.com/AdguardTeam/golibs/errors" + "github.com/AdguardTeam/golibs/log" +) + +// File type constants for logging. +const ( + typeDir = "directory" + typeFile = "file" +) + +// Check checks the permissions on important files. It logs the results at +// appropriate levels. +func Check(workDir, dataDir, statsDir, querylogDir, confFilePath string) { + checkDir(workDir) + + checkFile(confFilePath) + + // TODO(a.garipov): Put all paths in one place and remove this duplication. + checkDir(dataDir) + checkDir(filepath.Join(dataDir, "filters")) + checkFile(filepath.Join(dataDir, "sessions.db")) + checkFile(filepath.Join(dataDir, "leases.json")) + + if dataDir != querylogDir { + checkDir(querylogDir) + } + checkFile(filepath.Join(querylogDir, "querylog.json")) + checkFile(filepath.Join(querylogDir, "querylog.json.1")) + + if dataDir != statsDir { + checkDir(statsDir) + } + checkFile(filepath.Join(statsDir, "stats.db")) +} + +// checkDir checks the permissions of a single directory. The results are +// logged at the appropriate level. +func checkDir(dirPath string) { + checkPath(dirPath, typeDir, aghos.DefaultPermDir) +} + +// checkFile checks the permissions of a single file. The results are logged at +// the appropriate level. +func checkFile(filePath string) { + checkPath(filePath, typeFile, aghos.DefaultPermFile) +} + +// checkPath checks the permissions of a single filesystem entity. The results +// are logged at the appropriate level. +func checkPath(entPath, fileType string, want fs.FileMode) { + s, err := os.Stat(entPath) + if err != nil { + logFunc := log.Error + if errors.Is(err, os.ErrNotExist) { + logFunc = log.Debug + } + + logFunc("permcheck: checking %s %q: %s", fileType, entPath, err) + + return + } + + // TODO(a.garipov): Add a more fine-grained check and result reporting. + perm := s.Mode().Perm() + if perm != want { + log.Info( + "permcheck: SECURITY WARNING: %s %q has unexpected permissions %#o; want %#o", + fileType, + entPath, + perm, + want, + ) + } +} diff --git a/internal/querylog/qlogfile.go b/internal/querylog/qlogfile.go index 397840c9..175420ea 100644 --- a/internal/querylog/qlogfile.go +++ b/internal/querylog/qlogfile.go @@ -8,6 +8,7 @@ import ( "sync" "time" + "github.com/AdguardTeam/AdGuardHome/internal/aghos" "github.com/AdguardTeam/golibs/errors" "github.com/AdguardTeam/golibs/log" ) @@ -56,7 +57,7 @@ type qLogFile struct { // newQLogFile initializes a new instance of the qLogFile. func newQLogFile(path string) (qf *qLogFile, err error) { - f, err := os.OpenFile(path, os.O_RDONLY, 0o644) + f, err := os.OpenFile(path, os.O_RDONLY, aghos.DefaultPermFile) if err != nil { return nil, err } diff --git a/internal/querylog/querylogfile.go b/internal/querylog/querylogfile.go index 3783376f..412a364b 100644 --- a/internal/querylog/querylogfile.go +++ b/internal/querylog/querylogfile.go @@ -7,6 +7,7 @@ import ( "os" "time" + "github.com/AdguardTeam/AdGuardHome/internal/aghos" "github.com/AdguardTeam/golibs/errors" "github.com/AdguardTeam/golibs/log" ) @@ -70,7 +71,7 @@ func (l *queryLog) flushToFile(b *bytes.Buffer) (err error) { filename := l.logFile - f, err := os.OpenFile(filename, os.O_WRONLY|os.O_CREATE|os.O_APPEND, 0o644) + f, err := os.OpenFile(filename, os.O_WRONLY|os.O_CREATE|os.O_APPEND, aghos.DefaultPermFile) if err != nil { return fmt.Errorf("creating file %q: %w", filename, err) } diff --git a/internal/stats/stats.go b/internal/stats/stats.go index c6de4c66..a3a0f62e 100644 --- a/internal/stats/stats.go +++ b/internal/stats/stats.go @@ -15,6 +15,7 @@ import ( "github.com/AdguardTeam/AdGuardHome/internal/aghhttp" "github.com/AdguardTeam/AdGuardHome/internal/aghnet" + "github.com/AdguardTeam/AdGuardHome/internal/aghos" "github.com/AdguardTeam/golibs/errors" "github.com/AdguardTeam/golibs/logutil/slogutil" "github.com/AdguardTeam/golibs/timeutil" @@ -383,7 +384,7 @@ func (s *StatsCtx) openDB() (err error) { s.logger.Debug("opening database") var db *bbolt.DB - db, err = bbolt.Open(s.filename, 0o644, nil) + db, err = bbolt.Open(s.filename, aghos.DefaultPermFile, nil) if err != nil { if err.Error() == "invalid argument" { const lines = `AdGuard Home cannot be initialized due to an incompatible file system. diff --git a/internal/updater/updater.go b/internal/updater/updater.go index 4e4a21d4..b3e85dee 100644 --- a/internal/updater/updater.go +++ b/internal/updater/updater.go @@ -15,6 +15,7 @@ import ( "sync" "time" + "github.com/AdguardTeam/AdGuardHome/internal/aghos" "github.com/AdguardTeam/AdGuardHome/internal/version" "github.com/AdguardTeam/golibs/errors" "github.com/AdguardTeam/golibs/ioutil" @@ -263,7 +264,7 @@ func (u *Updater) check() (err error) { // ignores the configuration file if firstRun is true. func (u *Updater) backup(firstRun bool) (err error) { log.Debug("updater: backing up current configuration") - _ = os.Mkdir(u.backupDir, 0o755) + _ = os.Mkdir(u.backupDir, aghos.DefaultPermDir) if !firstRun { err = copyFile(u.confName, filepath.Join(u.backupDir, "AdGuardHome.yaml")) if err != nil { @@ -337,10 +338,10 @@ func (u *Updater) downloadPackageFile() (err error) { return fmt.Errorf("io.ReadAll() failed: %w", err) } - _ = os.Mkdir(u.updateDir, 0o755) + _ = os.Mkdir(u.updateDir, aghos.DefaultPermDir) log.Debug("updater: saving package to file") - err = os.WriteFile(u.packageName, body, 0o644) + err = os.WriteFile(u.packageName, body, aghos.DefaultPermFile) if err != nil { return fmt.Errorf("os.WriteFile() failed: %w", err) } @@ -527,7 +528,7 @@ func copyFile(src, dst string) error { if e != nil { return e } - e = os.WriteFile(dst, d, 0o644) + e = os.WriteFile(dst, d, aghos.DefaultPermFile) if e != nil { return e } diff --git a/scripts/install.sh b/scripts/install.sh index 08c54e87..31184cb0 100644 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -497,7 +497,7 @@ download() { # Function unpack unpacks the passed archive depending on it's extension. unpack() { log "unpacking package from $pkg_name into $out_dir" - if ! mkdir -p "$out_dir" + if ! mkdir -m 0700 -p "$out_dir" then error_exit "cannot create directory $out_dir" fi