Skip to content

Commit 355cec1

Browse files
ainar-gainar-g
committed
Pull request 2284: AG-32257-file-permission-mitigation
Squashed commit of the following: commit 6e0e61e Merge: e3cccc0 5b5b397 Author: Ainar Garipov <A.Garipov@AdGuard.COM> Date: Wed Oct 2 20:51:29 2024 +0300 Merge branch 'master' into AG-32257-file-permission-mitigation commit e3cccc0 Author: Ainar Garipov <A.Garipov@AdGuard.COM> Date: Wed Oct 2 19:57:32 2024 +0300 dnsforward: imp test commit 16ecebb Author: Ainar Garipov <A.Garipov@AdGuard.COM> Date: Wed Oct 2 19:22:10 2024 +0300 configmigrate: imp tests commit da8777c Author: Ainar Garipov <A.Garipov@AdGuard.COM> Date: Wed Oct 2 18:58:46 2024 +0300 all: imp types, tests commit 58822a0 Author: Ainar Garipov <A.Garipov@AdGuard.COM> Date: Wed Oct 2 18:28:37 2024 +0300 all: imp chlog commit 8ce81f9 Author: Ainar Garipov <A.Garipov@AdGuard.COM> Date: Wed Oct 2 18:09:57 2024 +0300 all: improve permissions, add safe_fs_patterns
1 parent 5b5b397 commit 355cec1

31 files changed

+879
-52
lines changed

‎CHANGELOG.md

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,20 @@ NOTE: Add new changes BELOW THIS COMMENT.
2929

3030
### Security
3131

32+
- Previous versions of AdGuard Home allowed users to add any system it had
33+
access to as filters, exposing them to be world-readable. To prevent this,
34+
AdGuard Home now allows adding filtering-rule list files only from files
35+
matching the patterns enumerated in the `filtering.safe_fs_patterns` property
36+
in the configuration file.
37+
38+
We thank @itz-d0dgy for reporting this vulnerability, designated
39+
CVE-2024-36814, to us.
40+
- Additionally, AdGuard Home will now try to change the permissions of its files
41+
and directories to more restrictive ones to prevent similar vulnerabilities
42+
as well as limit the access to the configuration.
43+
44+
We thank @go-compile for reporting this vulnerability, designated
45+
CVE-2024-36586, to us.
3246
- Go version has been updated to prevent the possibility of exploiting the Go
3347
vulnerabilities fixed in [1.23.2][go-1.23.2].
3448

@@ -42,6 +56,15 @@ NOTE: Add new changes BELOW THIS COMMENT.
4256
- Upstream server URL domain names requirements has been relaxed and now follow
4357
the same rules as their domain specifications.
4458

59+
#### Configuration changes
60+
61+
In this release, the schema version has changed from 28 to 29.
62+
63+
- The new array `filtering.safe_fs_patterns` contains glob patterns for paths of
64+
files that can be added as local filtering-rule lists. The migration should
65+
add list files that have already been added, as well as the default value,
66+
`$DATA_DIR/userfilters/*`.
67+
4568
### Fixed
4669

4770
- Property `clients.runtime_sources.dhcp` in the configuration file not taking
@@ -50,6 +73,22 @@ NOTE: Add new changes BELOW THIS COMMENT.
5073
- Enforce Bing safe search from Edge sidebar ([#7154]).
5174
- Text overflow on the query log page ([#7119]).
5275

76+
### Known issues
77+
78+
- Due to the complexity of the Windows permissions architecture and poor support
79+
from the standard Go library, we have to postpone the proper automated Windows
80+
fix until the next release.
81+
82+
**Temporary workaround:** Set the permissions of the `AdGuardHome` directory
83+
to more restrictive ones manually. To do that:
84+
85+
1. Locate the `AdGuardHome` directory.
86+
2. Right-click on it and navigate to *Properties → Security → Advanced*.
87+
3. (You might need to disable permission inheritance to make them more
88+
restricted.)
89+
4. Adjust to give the `Full control` access to only the user which runs
90+
AdGuard Home. Typically, `Administrator`.
91+
5392
[#5009]: https://github.com/AdguardTeam/AdGuardHome/issues/5009
5493
[#5704]: https://github.com/AdguardTeam/AdGuardHome/issues/5704
5594
[#7119]: https://github.com/AdguardTeam/AdGuardHome/issues/7119

‎internal/aghos/os.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"bufio"
88
"fmt"
99
"io"
10+
"io/fs"
1011
"os"
1112
"os/exec"
1213
"path"
@@ -19,6 +20,12 @@ import (
1920
"github.com/AdguardTeam/golibs/log"
2021
)
2122

23+
// Default file and directory permissions.
24+
const (
25+
DefaultPermDir fs.FileMode = 0o700
26+
DefaultPermFile fs.FileMode = 0o600
27+
)
28+
2229
// Unsupported is a helper that returns a wrapped [errors.ErrUnsupported].
2330
func Unsupported(op string) (err error) {
2431
return fmt.Errorf("%s: not supported on %s: %w", op, runtime.GOOS, errors.ErrUnsupported)

‎internal/configmigrate/configmigrate.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,4 @@
22
package configmigrate
33

44
// LastSchemaVersion is the most recent schema version.
5-
const LastSchemaVersion uint = 28
5+
const LastSchemaVersion uint = 29

‎internal/configmigrate/migrations_internal_test.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ func TestUpgradeSchema1to2(t *testing.T) {
1919

2020
m := New(&Config{
2121
WorkingDir: "",
22+
DataDir: "",
2223
})
2324

2425
err := m.migrateTo2(diskConf)

‎internal/configmigrate/migrator.go

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,20 +10,24 @@ import (
1010

1111
// Config is a the configuration for initializing a [Migrator].
1212
type Config struct {
13-
// WorkingDir is an absolute path to the working directory of AdGuardHome.
13+
// WorkingDir is the absolute path to the working directory of AdGuardHome.
1414
WorkingDir string
15+
16+
// DataDir is the absolute path to the data directory of AdGuardHome.
17+
DataDir string
1518
}
1619

1720
// Migrator performs the YAML configuration file migrations.
1821
type Migrator struct {
19-
// workingDir is an absolute path to the working directory of AdGuardHome.
2022
workingDir string
23+
dataDir string
2124
}
2225

2326
// New creates a new Migrator.
24-
func New(cfg *Config) (m *Migrator) {
27+
func New(c *Config) (m *Migrator) {
2528
return &Migrator{
26-
workingDir: cfg.WorkingDir,
29+
workingDir: c.WorkingDir,
30+
dataDir: c.DataDir,
2731
}
2832
}
2933

@@ -120,6 +124,7 @@ func (m *Migrator) upgradeConfigSchema(current, target uint, diskConf yobj) (err
120124
25: migrateTo26,
121125
26: migrateTo27,
122126
27: migrateTo28,
127+
28: m.migrateTo29,
123128
}
124129

125130
for i, migrate := range upgrades[current:target] {

‎internal/configmigrate/migrator_test.go

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,12 @@
11
package configmigrate_test
22

33
import (
4+
"bytes"
45
"io/fs"
56
"os"
67
"path"
8+
"path/filepath"
9+
"runtime"
710
"testing"
811

912
"github.com/AdguardTeam/AdGuardHome/internal/configmigrate"
@@ -202,6 +205,7 @@ func TestMigrateConfig_Migrate(t *testing.T) {
202205

203206
migrator := configmigrate.New(&configmigrate.Config{
204207
WorkingDir: t.Name(),
208+
DataDir: filepath.Join(t.Name(), "data"),
205209
})
206210
newBody, upgraded, err := migrator.Migrate(body, tc.targetVersion)
207211
require.NoError(t, err)
@@ -211,3 +215,43 @@ func TestMigrateConfig_Migrate(t *testing.T) {
211215
})
212216
}
213217
}
218+
219+
// TODO(a.garipov): Consider ways of merging into the previous one.
220+
func TestMigrateConfig_Migrate_v29(t *testing.T) {
221+
const (
222+
pathUnix = `/path/to/file.txt`
223+
userDirPatUnix = `TestMigrateConfig_Migrate/v29/data/userfilters/*`
224+
225+
pathWindows = `C:\path\to\file.txt`
226+
userDirPatWindows = `TestMigrateConfig_Migrate\v29\data\userfilters\*`
227+
)
228+
229+
pathToReplace := pathUnix
230+
patternToReplace := userDirPatUnix
231+
if runtime.GOOS == "windows" {
232+
pathToReplace = pathWindows
233+
patternToReplace = userDirPatWindows
234+
}
235+
236+
body, err := fs.ReadFile(testdata, "TestMigrateConfig_Migrate/v29/input.yml")
237+
require.NoError(t, err)
238+
239+
body = bytes.ReplaceAll(body, []byte("FILEPATH"), []byte(pathToReplace))
240+
241+
wantBody, err := fs.ReadFile(testdata, "TestMigrateConfig_Migrate/v29/output.yml")
242+
require.NoError(t, err)
243+
244+
wantBody = bytes.ReplaceAll(wantBody, []byte("FILEPATH"), []byte(pathToReplace))
245+
wantBody = bytes.ReplaceAll(wantBody, []byte("USERFILTERSPATH"), []byte(patternToReplace))
246+
247+
migrator := configmigrate.New(&configmigrate.Config{
248+
WorkingDir: t.Name(),
249+
DataDir: "TestMigrateConfig_Migrate/v29/data",
250+
})
251+
252+
newBody, upgraded, err := migrator.Migrate(body, 29)
253+
require.NoError(t, err)
254+
require.True(t, upgraded)
255+
256+
require.YAMLEq(t, string(wantBody), string(newBody))
257+
}

‎internal/configmigrate/testdata/TestMigrateConfig_Migrate/v29/input.yml

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,117 @@
1+
http:
2+
address: 127.0.0.1:3000
3+
session_ttl: 3h
4+
pprof:
5+
enabled: true
6+
port: 6060
7+
users:
8+
- name: testuser
9+
password: testpassword
10+
dns:
11+
bind_hosts:
12+
- 127.0.0.1
13+
port: 53
14+
parental_sensitivity: 0
15+
upstream_dns:
16+
- tls://1.1.1.1
17+
- tls://1.0.0.1
18+
- quic://8.8.8.8:784
19+
bootstrap_dns:
20+
- 8.8.8.8:53
21+
edns_client_subnet:
22+
enabled: true
23+
use_custom: false
24+
custom_ip: ""
25+
filtering:
26+
filtering_enabled: true
27+
parental_enabled: false
28+
safebrowsing_enabled: false
29+
safe_search:
30+
enabled: false
31+
bing: true
32+
duckduckgo: true
33+
google: true
34+
pixabay: true
35+
yandex: true
36+
youtube: true
37+
protection_enabled: true
38+
blocked_services:
39+
schedule:
40+
time_zone: Local
41+
ids:
42+
- 500px
43+
blocked_response_ttl: 10
44+
filters:
45+
- url: https://adaway.org/hosts.txt
46+
name: AdAway
47+
enabled: false
48+
- url: FILEPATH
49+
name: Local Filter
50+
enabled: false
51+
clients:
52+
persistent:
53+
- name: localhost
54+
ids:
55+
- 127.0.0.1
56+
- aa:aa:aa:aa:aa:aa
57+
use_global_settings: true
58+
use_global_blocked_services: true
59+
filtering_enabled: false
60+
parental_enabled: false
61+
safebrowsing_enabled: false
62+
safe_search:
63+
enabled: true
64+
bing: true
65+
duckduckgo: true
66+
google: true
67+
pixabay: true
68+
yandex: true
69+
youtube: true
70+
blocked_services:
71+
schedule:
72+
time_zone: Local
73+
ids:
74+
- 500px
75+
runtime_sources:
76+
whois: true
77+
arp: true
78+
rdns: true
79+
dhcp: true
80+
hosts: true
81+
dhcp:
82+
enabled: false
83+
interface_name: vboxnet0
84+
local_domain_name: local
85+
dhcpv4:
86+
gateway_ip: 192.168.0.1
87+
subnet_mask: 255.255.255.0
88+
range_start: 192.168.0.10
89+
range_end: 192.168.0.250
90+
lease_duration: 1234
91+
icmp_timeout_msec: 10
92+
schema_version: 28
93+
user_rules: []
94+
querylog:
95+
enabled: true
96+
file_enabled: true
97+
interval: 720h
98+
size_memory: 1000
99+
ignored:
100+
- '|.^'
101+
statistics:
102+
enabled: true
103+
interval: 240h
104+
ignored:
105+
- '|.^'
106+
os:
107+
group: ''
108+
rlimit_nofile: 123
109+
user: ''
110+
log:
111+
file: ""
112+
max_backups: 0
113+
max_size: 100
114+
max_age: 3
115+
compress: true
116+
local_time: false
117+
verbose: true

0 commit comments

Comments
 (0)