Pull request 2284: AG-32257-file-permission-mitigation

Squashed commit of the following: commit 6e0e61e Merge: e3cccc0 5b5b397 Author: Ainar Garipov <[email protected]> Date: Wed Oct 2 20:51:29 2024 +0300 Merge branch 'master' into AG-32257-file-permission-mitigation commit e3cccc0 Author: Ainar Garipov <[email protected]> Date: Wed Oct 2 19:57:32 2024 +0300 dnsforward: imp test commit 16ecebb Author: Ainar Garipov <[email protected]> Date: Wed Oct 2 19:22:10 2024 +0300 configmigrate: imp tests commit da8777c Author: Ainar Garipov <[email protected]> Date: Wed Oct 2 18:58:46 2024 +0300 all: imp types, tests commit 58822a0 Author: Ainar Garipov <[email protected]> Date: Wed Oct 2 18:28:37 2024 +0300 all: imp chlog commit 8ce81f9 Author: Ainar Garipov <[email protected]> Date: Wed Oct 2 18:09:57 2024 +0300 all: improve permissions, add safe_fs_patterns
AdguardTeam · Oct 2, 2024 · 355cec1 · 355cec1
1 parent 5b5b397
commit 355cec1
Show file tree

Hide file tree

Showing 31 changed files with 879 additions and 52 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -29,6 +29,20 @@ NOTE: Add new changes BELOW THIS COMMENT.
 
 ### Security
 
+- Previous 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
@@ -7,6 +7,7 @@ import (
 	"bufio"
 	"fmt"
 	"io"
+	"io/fs"
 	"os"
 	"os/exec"
 	"path"
@@ -19,6 +20,12 @@ import (
 	"github.com/AdguardTeam/golibs/log"
 )
 
+// Default file and directory permissions.
+const (
+	DefaultPermDir  fs.FileMode = 0o700
+	DefaultPermFile fs.FileMode = 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
@@ -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
@@ -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
@@ -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
@@ -1,9 +1,12 @@
 package configmigrate_test
 
 import (
+	"bytes"
 	"io/fs"
 	"os"
 	"path"
+	"path/filepath"
+	"runtime"
 	"testing"
 
 	"github.com/AdguardTeam/AdGuardHome/internal/configmigrate"
@@ -202,6 +205,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)
@@ -211,3 +215,43 @@ func TestMigrateConfig_Migrate(t *testing.T) {
 		})
 	}
 }
+
+// TODO(a.garipov):  Consider ways of merging into the previous one.
+func TestMigrateConfig_Migrate_v29(t *testing.T) {
+	const (
+		pathUnix       = `/path/to/file.txt`
+		userDirPatUnix = `TestMigrateConfig_Migrate/v29/data/userfilters/*`
+
+		pathWindows       = `C:\path\to\file.txt`
+		userDirPatWindows = `TestMigrateConfig_Migrate\v29\data\userfilters\*`
+	)
+
+	pathToReplace := pathUnix
+	patternToReplace := userDirPatUnix
+	if runtime.GOOS == "windows" {
+		pathToReplace = pathWindows
+		patternToReplace = userDirPatWindows
+	}
+
+	body, err := fs.ReadFile(testdata, "TestMigrateConfig_Migrate/v29/input.yml")
+	require.NoError(t, err)
+
+	body = bytes.ReplaceAll(body, []byte("FILEPATH"), []byte(pathToReplace))
+
+	wantBody, err := fs.ReadFile(testdata, "TestMigrateConfig_Migrate/v29/output.yml")
+	require.NoError(t, err)
+
+	wantBody = bytes.ReplaceAll(wantBody, []byte("FILEPATH"), []byte(pathToReplace))
+	wantBody = bytes.ReplaceAll(wantBody, []byte("USERFILTERSPATH"), []byte(patternToReplace))
+
+	migrator := configmigrate.New(&configmigrate.Config{
+		WorkingDir: t.Name(),
+		DataDir:    "TestMigrateConfig_Migrate/v29/data",
+	})
+
+	newBody, upgraded, err := migrator.Migrate(body, 29)
+	require.NoError(t, err)
+	require.True(t, upgraded)
+
+	require.YAMLEq(t, string(wantBody), string(newBody))
+}
diff --git a/internal/configmigrate/testdata/TestMigrateConfig_Migrate/v29/input.yml 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: FILEPATH
+  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