Skip to content

Commit 948a14c

Browse files
sean-breensean-breen
authored
Add validation to allowed directories on config load (#1144)
* check for invalid characters in allowedDirectories when loading config * add sanitising of allowed directories when loading configuration * remove log * fix lint issues * fix test * add check for symlinks, disallow all symlinks * fix lint * fix lint * fix lint * fix default paths * update comment * fix default collector config test * return bool + error from isAllowedDirs * fix lint * rename tests * remove needless logs * fix lint warning * refactor to remove unessessary logs * fix lint * fix test names * fix test * add unit test for isSymlink * use require in place of assert * update regex * update comment * fix unit test * Remove symlink check * fix unit test after merging main
1 parent 1cc62f9 commit 948a14c

File tree

5 files changed

+271
-156
lines changed

5 files changed

+271
-156
lines changed

‎internal/config/config.go‎

Lines changed: 34 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"log/slog"
1515
"os"
1616
"path/filepath"
17+
"regexp"
1718
"slices"
1819
"strconv"
1920
"strings"
@@ -36,11 +37,15 @@ const (
3637
EnvPrefix = "NGINX_AGENT"
3738
KeyDelimiter = "_"
3839
KeyValueNumber = 2
39-
AgentDirName = "/etc/nginx-agent/"
40+
AgentDirName = "/etc/nginx-agent"
4041
DefaultMetricsBatchProcessor = "default_metrics"
4142
DefaultLogsBatchProcessor = "default_logs"
4243
DefaultExporter = "default"
4344
DefaultPipeline = "default"
45+
46+
// Regular expression to match invalid characters in paths.
47+
// It matches whitespace, control characters, non-printable characters, and specific Unicode characters.
48+
regexInvalidPath = "\\s|[[:cntrl:]]|[[:space:]]|[[^:print:]]|ㅤ|\\.\\.|\\*"
4449
)
4550

4651
var viperInstance = viper.NewWithOptions(viper.KeyDelimiter(KeyDelimiter))
@@ -83,27 +88,13 @@ func RegisterConfigFile() error {
8388
}
8489

8590
func ResolveConfig() (*Config, error) {
86-
// Collect allowed directories, so that paths in the config can be validated.
87-
directories := viperInstance.GetStringSlice(AllowedDirectoriesKey)
88-
allowedDirs := []string{AgentDirName}
89-
9091
log := resolveLog()
9192
slogger := logger.New(log.Path, log.Level)
9293
slog.SetDefault(slogger)
9394

94-
// Check directories in allowed_directories are valid
95-
for _, dir := range directories {
96-
if dir == "" || !filepath.IsAbs(dir) {
97-
slog.Warn("Invalid directory: ", "dir", dir)
98-
continue
99-
}
100-
101-
if !strings.HasSuffix(dir, "/") {
102-
dir += "/"
103-
}
104-
allowedDirs = append(allowedDirs, dir)
105-
}
106-
95+
// Collect allowed directories, so that paths in the config can be validated.
96+
directories := viperInstance.GetStringSlice(AllowedDirectoriesKey)
97+
allowedDirs := resolveAllowedDirectories(directories)
10798
slog.Info("Configured allowed directories", "allowed_directories", allowedDirs)
10899

109100
// Collect all parsing errors before returning the error, so the user sees all issues with config
@@ -142,6 +133,29 @@ func ResolveConfig() (*Config, error) {
142133
return config, nil
143134
}
144135

136+
// resolveAllowedDirectories checks if the provided directories are valid and returns a slice of cleaned absolute paths.
137+
// It ignores empty paths, paths that are not absolute, and paths containing invalid characters.
138+
// Invalid paths are logged as warnings.
139+
func resolveAllowedDirectories(dirs []string) []string {
140+
allowed := []string{AgentDirName}
141+
for _, dir := range dirs {
142+
re := regexp.MustCompile(regexInvalidPath)
143+
invalidChars := re.MatchString(dir)
144+
if dir == "" || dir == "/" || !filepath.IsAbs(dir) || invalidChars {
145+
slog.Warn("Ignoring invalid directory", "dir", dir)
146+
continue
147+
}
148+
dir = filepath.Clean(dir)
149+
if dir == AgentDirName {
150+
// If the directory is the default agent directory, we skip adding it again.
151+
continue
152+
}
153+
allowed = append(allowed, dir)
154+
}
155+
156+
return allowed
157+
}
158+
145159
func defaultCollector(collector *Collector, config *Config) {
146160
// Always add default host metric receiver and default processor
147161
addDefaultHostMetricsReceiver(collector)
@@ -916,13 +930,14 @@ func resolveClient() *Client {
916930
}
917931

918932
func resolveCollector(allowedDirs []string) (*Collector, error) {
933+
// Collect receiver configurations
919934
var receivers Receivers
920-
921935
err := resolveMapStructure(CollectorReceiversKey, &receivers)
922936
if err != nil {
923937
return nil, fmt.Errorf("unmarshal collector receivers config: %w", err)
924938
}
925939

940+
// Collect exporter configurations
926941
exporters, err := resolveExporters()
927942
if err != nil {
928943
return nil, fmt.Errorf("unmarshal collector exporters config: %w", err)

‎internal/config/config_test.go‎

Lines changed: 167 additions & 85 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,84 @@ func TestNormalizeFunc(t *testing.T) {
163163
assert.Equal(t, expected, result)
164164
}
165165

166+
func TestResolveAllowedDirectories(t *testing.T) {
167+
tests := []struct {
168+
name string
169+
configuredDirs []string
170+
expected []string
171+
}{
172+
{
173+
name: "Test 1: Empty path",
174+
configuredDirs: []string{""},
175+
expected: []string{"/etc/nginx-agent"},
176+
},
177+
{
178+
name: "Test 2: Absolute path",
179+
configuredDirs: []string{"/etc/agent/"},
180+
expected: []string{"/etc/nginx-agent", "/etc/agent"},
181+
},
182+
{
183+
name: "Test 3: Absolute paths",
184+
configuredDirs: []string{"/etc/nginx/"},
185+
expected: []string{"/etc/nginx-agent", "/etc/nginx"},
186+
},
187+
{
188+
name: "Test 4: Absolute path with multiple slashes",
189+
configuredDirs: []string{"/etc///////////nginx-agent/"},
190+
expected: []string{"/etc/nginx-agent"},
191+
},
192+
{
193+
name: "Test 5: Absolute path with directory traversal",
194+
configuredDirs: []string{"/etc/nginx/../nginx-agent"},
195+
expected: []string{"/etc/nginx-agent"},
196+
},
197+
{
198+
name: "Test 6: Absolute path with repeat directory traversal",
199+
configuredDirs: []string{"/etc/nginx-agent/../../../../../nginx-agent"},
200+
expected: []string{"/etc/nginx-agent"},
201+
},
202+
{
203+
name: "Test 7: Absolute path with control characters",
204+
configuredDirs: []string{"/etc/nginx-agent/\\x08../tmp/"},
205+
expected: []string{"/etc/nginx-agent"},
206+
},
207+
{
208+
name: "Test 8: Absolute path with invisible characters",
209+
configuredDirs: []string{"/etc/nginx-agent/ㅤㅤㅤ/tmp/"},
210+
expected: []string{"/etc/nginx-agent"},
211+
},
212+
{
213+
name: "Test 9: Absolute path with escaped invisible characters",
214+
configuredDirs: []string{"/etc/nginx-agent/\\\\ㅤ/tmp/"},
215+
expected: []string{"/etc/nginx-agent"},
216+
},
217+
{
218+
name: "Test 10: Mixed paths",
219+
configuredDirs: []string{
220+
"nginx-agent",
221+
"",
222+
"..",
223+
"/",
224+
"\\/",
225+
".",
226+
"/etc/nginx/",
227+
},
228+
expected: []string{"/etc/nginx-agent", "/etc/nginx"},
229+
},
230+
{
231+
name: "Test 11: Relative path",
232+
configuredDirs: []string{"nginx-agent"},
233+
expected: []string{"/etc/nginx-agent"},
234+
},
235+
}
236+
for _, test := range tests {
237+
t.Run(test.name, func(t *testing.T) {
238+
allowed := resolveAllowedDirectories(test.configuredDirs)
239+
assert.Equal(t, test.expected, allowed)
240+
})
241+
}
242+
}
243+
166244
func TestResolveLog(t *testing.T) {
167245
viperInstance = viper.NewWithOptions(viper.KeyDelimiter(KeyDelimiter))
168246
viperInstance.Set(LogLevelKey, "error")
@@ -867,89 +945,7 @@ func agentConfig() *Config {
867945
"/etc/nginx/", "/etc/nginx-agent/", "/usr/local/etc/nginx/", "/var/run/nginx/", "/var/log/nginx/",
868946
"/usr/share/nginx/modules/", "/etc/app_protect/",
869947
},
870-
Collector: &Collector{
871-
ConfigPath: "/etc/nginx-agent/nginx-agent-otelcol.yaml",
872-
Exporters: Exporters{
873-
OtlpExporters: map[string]*OtlpExporter{
874-
"default": {
875-
Server: &ServerConfig{
876-
Host: "127.0.0.1",
877-
Port: 1234,
878-
Type: Grpc,
879-
},
880-
TLS: &TLSConfig{
881-
Cert: "/path/to/server-cert.pem",
882-
Key: "/path/to/server-cert.pem",
883-
Ca: "/path/to/server-cert.pem",
884-
SkipVerify: true,
885-
ServerName: "remote-saas-server",
886-
},
887-
},
888-
},
889-
},
890-
Processors: Processors{
891-
Batch: map[string]*Batch{
892-
"default_logs": {
893-
SendBatchMaxSize: DefCollectorLogsBatchProcessorSendBatchMaxSize,
894-
SendBatchSize: DefCollectorLogsBatchProcessorSendBatchSize,
895-
Timeout: DefCollectorLogsBatchProcessorTimeout,
896-
},
897-
},
898-
LogsGzip: map[string]*LogsGzip{
899-
"default": {},
900-
},
901-
},
902-
Receivers: Receivers{
903-
OtlpReceivers: map[string]*OtlpReceiver{
904-
"default": {
905-
Server: &ServerConfig{
906-
Host: "localhost",
907-
Port: 4317,
908-
Type: Grpc,
909-
},
910-
Auth: &AuthConfig{
911-
Token: "even-secreter-token",
912-
},
913-
OtlpTLSConfig: &OtlpTLSConfig{
914-
GenerateSelfSignedCert: false,
915-
Cert: "/path/to/server-cert.pem",
916-
Key: "/path/to/server-cert.pem",
917-
Ca: "/path/to/server-cert.pem",
918-
SkipVerify: true,
919-
ServerName: "local-data-plane-server",
920-
},
921-
},
922-
},
923-
NginxReceivers: []NginxReceiver{
924-
{
925-
InstanceID: "cd7b8911-c2c5-4daf-b311-dbead151d938",
926-
StubStatus: APIDetails{
927-
URL: "http://localhost:4321/status",
928-
Listen: "",
929-
},
930-
AccessLogs: []AccessLog{
931-
{
932-
LogFormat: accessLogFormat,
933-
FilePath: "/var/log/nginx/access-custom.conf",
934-
},
935-
},
936-
},
937-
},
938-
},
939-
Extensions: Extensions{
940-
Health: &Health{
941-
Server: &ServerConfig{
942-
Host: "localhost",
943-
Port: 1337,
944-
},
945-
Path: "/",
946-
},
947-
},
948-
Log: &Log{
949-
Level: "INFO",
950-
Path: "/var/log/nginx-agent/opentelemetry-collector-agent.log",
951-
},
952-
},
948+
Collector: createDefaultCollectorConfig(),
953949
Command: &Command{
954950
Server: &ServerConfig{
955951
Host: "127.0.0.1",
@@ -1002,8 +998,8 @@ func createConfig() *Config {
1002998
},
1003999
},
10041000
AllowedDirectories: []string{
1005-
"/etc/nginx-agent/", "/etc/nginx/", "/usr/local/etc/nginx/", "/var/run/nginx/",
1006-
"/usr/share/nginx/modules/", "/var/log/nginx/",
1001+
"/etc/nginx-agent", "/etc/nginx", "/usr/local/etc/nginx", "/var/run/nginx",
1002+
"/usr/share/nginx/modules", "/var/log/nginx",
10071003
},
10081004
DataPlaneConfig: &DataPlaneConfig{
10091005
Nginx: &NginxDataPlaneConfig{
@@ -1226,3 +1222,89 @@ func createConfig() *Config {
12261222
},
12271223
}
12281224
}
1225+
1226+
func createDefaultCollectorConfig() *Collector {
1227+
return &Collector{
1228+
ConfigPath: "/etc/nginx-agent/nginx-agent-otelcol.yaml",
1229+
Exporters: Exporters{
1230+
OtlpExporters: map[string]*OtlpExporter{
1231+
"default": {
1232+
Server: &ServerConfig{
1233+
Host: "127.0.0.1",
1234+
Port: 1234,
1235+
Type: Grpc,
1236+
},
1237+
TLS: &TLSConfig{
1238+
Cert: "/path/to/server-cert.pem",
1239+
Key: "/path/to/server-cert.pem",
1240+
Ca: "/path/to/server-cert.pem",
1241+
SkipVerify: true,
1242+
ServerName: "remote-saas-server",
1243+
},
1244+
},
1245+
},
1246+
},
1247+
Processors: Processors{
1248+
Batch: map[string]*Batch{
1249+
"default_logs": {
1250+
SendBatchMaxSize: DefCollectorLogsBatchProcessorSendBatchMaxSize,
1251+
SendBatchSize: DefCollectorLogsBatchProcessorSendBatchSize,
1252+
Timeout: DefCollectorLogsBatchProcessorTimeout,
1253+
},
1254+
},
1255+
LogsGzip: map[string]*LogsGzip{
1256+
"default": {},
1257+
},
1258+
},
1259+
Receivers: Receivers{
1260+
OtlpReceivers: map[string]*OtlpReceiver{
1261+
"default": {
1262+
Server: &ServerConfig{
1263+
Host: "localhost",
1264+
Port: 4317,
1265+
Type: Grpc,
1266+
},
1267+
Auth: &AuthConfig{
1268+
Token: "even-secreter-token",
1269+
},
1270+
OtlpTLSConfig: &OtlpTLSConfig{
1271+
GenerateSelfSignedCert: false,
1272+
Cert: "/path/to/server-cert.pem",
1273+
Key: "/path/to/server-cert.pem",
1274+
Ca: "/path/to/server-cert.pem",
1275+
SkipVerify: true,
1276+
ServerName: "local-data-plane-server",
1277+
},
1278+
},
1279+
},
1280+
NginxReceivers: []NginxReceiver{
1281+
{
1282+
InstanceID: "cd7b8911-c2c5-4daf-b311-dbead151d938",
1283+
StubStatus: APIDetails{
1284+
URL: "http://localhost:4321/status",
1285+
Listen: "",
1286+
},
1287+
AccessLogs: []AccessLog{
1288+
{
1289+
LogFormat: accessLogFormat,
1290+
FilePath: "/var/log/nginx/access-custom.conf",
1291+
},
1292+
},
1293+
},
1294+
},
1295+
},
1296+
Extensions: Extensions{
1297+
Health: &Health{
1298+
Server: &ServerConfig{
1299+
Host: "localhost",
1300+
Port: 1337,
1301+
},
1302+
Path: "/",
1303+
},
1304+
},
1305+
Log: &Log{
1306+
Level: "INFO",
1307+
Path: "/var/log/nginx-agent/opentelemetry-collector-agent.log",
1308+
},
1309+
}
1310+
}

0 commit comments

Comments
 (0)