Skip to content

Commit 85cfa2d

Browse files
committed
security: fix CodeQL security vulnerabilities in TLS parameters
Address 9 high-severity security issues identified by GitHub CodeQL: 1. **Integer Conversion Security**: - Add proper bounds checking for tls_min_version and tls_max_version - Validate input range (0-65535) before casting to uint16 - Prevent integer overflow vulnerabilities 2. **TLS Security Enforcement**: - Enforce minimum TLS 1.2 (771) for all TLS version parameters - Reject insecure TLS versions (< TLS 1.2) with clear error messages - Prevent downgrade attacks and insecure configurations 3. **Comprehensive Validation**: - Applied security fixes to all client types (single, cluster, sentinel) - Added security validation test cases - Updated documentation to reflect security requirements 4. **Test Coverage**: - Added tests for insecure TLS version rejection - Added tests for integer overflow protection - Updated existing tests to use secure TLS versions (771, 772) Security improvements: - Prevents integer overflow attacks via malicious URL parameters - Enforces secure TLS configurations by default - Provides clear error messages for security violations - Maintains backward compatibility for secure configurations Fixes all CodeQL security alerts while maintaining functionality.
1 parent 8c57646 commit 85cfa2d

File tree

5 files changed

+70
-13
lines changed

5 files changed

+70
-13
lines changed

options.go

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -360,7 +360,7 @@ func NewDialer(opt *Options) func(context.Context, string, string) (net.Conn, er
360360
// - use "skip_verify=true" to ignore TLS certificate validation
361361
// - for rediss:// URLs, additional TLS parameters are supported:
362362
// - tls_cert_file and tls_key_file: paths to client certificate and key files
363-
// - tls_min_version and tls_max_version: TLS version constraints (e.g., 771 for TLS 1.2)
363+
// - tls_min_version and tls_max_version: TLS version constraints (minimum TLS 1.2: 771, TLS 1.3: 772)
364364
// - tls_server_name: override server name for certificate validation
365365
//
366366
// Examples:
@@ -598,10 +598,26 @@ func setupConnParams(u *url.URL, o *Options) (*Options, error) {
598598
}
599599

600600
if q.has("tls_min_version") {
601-
o.TLSConfig.MinVersion = uint16(q.int("tls_min_version"))
601+
minVer := q.int("tls_min_version")
602+
if minVer < 0 || minVer > 65535 {
603+
return nil, fmt.Errorf("redis: invalid tls_min_version: %d (must be between 0 and 65535)", minVer)
604+
}
605+
// Enforce minimum TLS 1.2 for security
606+
if minVer > 0 && minVer < int(tls.VersionTLS12) {
607+
return nil, fmt.Errorf("redis: tls_min_version %d is insecure (minimum allowed is TLS 1.2: %d)", minVer, tls.VersionTLS12)
608+
}
609+
o.TLSConfig.MinVersion = uint16(minVer)
602610
}
603611
if q.has("tls_max_version") {
604-
o.TLSConfig.MaxVersion = uint16(q.int("tls_max_version"))
612+
maxVer := q.int("tls_max_version")
613+
if maxVer < 0 || maxVer > 65535 {
614+
return nil, fmt.Errorf("redis: invalid tls_max_version: %d (must be between 0 and 65535)", maxVer)
615+
}
616+
// Ensure max version is not lower than TLS 1.2
617+
if maxVer > 0 && maxVer < int(tls.VersionTLS12) {
618+
return nil, fmt.Errorf("redis: tls_max_version %d is insecure (minimum allowed is TLS 1.2: %d)", maxVer, tls.VersionTLS12)
619+
}
620+
o.TLSConfig.MaxVersion = uint16(maxVer)
605621
}
606622

607623
tlsServerName := q.string("tls_server_name")

options_test.go

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,8 +52,8 @@ EKTcWGekdmdDPsHloRNtsiCa697B2O9IFA==
5252
url: "rediss://localhost:123",
5353
o: &Options{Addr: "localhost:123", TLSConfig: &tls.Config{ServerName: "localhost", MinVersion: tls.VersionTLS12}},
5454
}, {
55-
url: "rediss://localhost:123?tls_server_name=abc&tls_min_version=1&tls_max_version=3&skip_verify=true",
56-
o: &Options{Addr: "localhost:123", TLSConfig: &tls.Config{ServerName: "abc", MinVersion: 1, MaxVersion: 3, InsecureSkipVerify: true}},
55+
url: "rediss://localhost:123?tls_server_name=abc&tls_min_version=771&tls_max_version=772&skip_verify=true",
56+
o: &Options{Addr: "localhost:123", TLSConfig: &tls.Config{ServerName: "abc", MinVersion: 771, MaxVersion: 772, InsecureSkipVerify: true}},
5757
}, {
5858
url: "rediss://localhost:123?tls_cert_file=./testdata/testcert.pem&tls_key_file=./testdata/testkey.pem",
5959
o: &Options{Addr: "localhost:123", TLSConfig: &tls.Config{ServerName: "localhost", MinVersion: tls.VersionTLS12, Certificates: []tls.Certificate{testCert}}},
@@ -68,6 +68,15 @@ EKTcWGekdmdDPsHloRNtsiCa697B2O9IFA==
6868
}, {
6969
url: "rediss://localhost:123?tls_key_file=./testdata/testkey.pem",
7070
err: errors.New("redis: tls_cert_file and tls_key_file URL parameters must be both set or both omitted"),
71+
}, {
72+
url: "rediss://localhost:123?tls_min_version=1",
73+
err: errors.New("redis: tls_min_version 1 is insecure (minimum allowed is TLS 1.2: 771)"),
74+
}, {
75+
url: "rediss://localhost:123?tls_max_version=1",
76+
err: errors.New("redis: tls_max_version 1 is insecure (minimum allowed is TLS 1.2: 771)"),
77+
}, {
78+
url: "rediss://localhost:123?tls_min_version=70000",
79+
err: errors.New("redis: invalid tls_min_version: 70000 (must be between 0 and 65535)"),
7180
}, {
7281
url: "rediss://localhost:123/?skip_verify=true",
7382
o: &Options{Addr: "localhost:123", TLSConfig: &tls.Config{ServerName: "localhost", MinVersion: tls.VersionTLS12, InsecureSkipVerify: true}},

osscluster.go

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,7 @@ func (opt *ClusterOptions) init() {
219219
// - use "skip_verify=true" to ignore TLS certificate validation
220220
// - for rediss:// URLs, additional TLS parameters are supported:
221221
// - tls_cert_file and tls_key_file: paths to client certificate and key files
222-
// - tls_min_version and tls_max_version: TLS version constraints (e.g., 771 for TLS 1.2)
222+
// - tls_min_version and tls_max_version: TLS version constraints (minimum TLS 1.2: 771, TLS 1.3: 772)
223223
// - tls_server_name: override server name for certificate validation
224224
//
225225
// Example:
@@ -325,10 +325,26 @@ func setupClusterQueryParams(u *url.URL, o *ClusterOptions) (*ClusterOptions, er
325325
}
326326

327327
if q.has("tls_min_version") {
328-
o.TLSConfig.MinVersion = uint16(q.int("tls_min_version"))
328+
minVer := q.int("tls_min_version")
329+
if minVer < 0 || minVer > 65535 {
330+
return nil, fmt.Errorf("redis: invalid tls_min_version: %d (must be between 0 and 65535)", minVer)
331+
}
332+
// Enforce minimum TLS 1.2 for security
333+
if minVer > 0 && minVer < int(tls.VersionTLS12) {
334+
return nil, fmt.Errorf("redis: tls_min_version %d is insecure (minimum allowed is TLS 1.2: %d)", minVer, tls.VersionTLS12)
335+
}
336+
o.TLSConfig.MinVersion = uint16(minVer)
329337
}
330338
if q.has("tls_max_version") {
331-
o.TLSConfig.MaxVersion = uint16(q.int("tls_max_version"))
339+
maxVer := q.int("tls_max_version")
340+
if maxVer < 0 || maxVer > 65535 {
341+
return nil, fmt.Errorf("redis: invalid tls_max_version: %d (must be between 0 and 65535)", maxVer)
342+
}
343+
// Ensure max version is not lower than TLS 1.2
344+
if maxVer > 0 && maxVer < int(tls.VersionTLS12) {
345+
return nil, fmt.Errorf("redis: tls_max_version %d is insecure (minimum allowed is TLS 1.2: %d)", maxVer, tls.VersionTLS12)
346+
}
347+
o.TLSConfig.MaxVersion = uint16(maxVer)
332348
}
333349

334350
tlsServerName := q.string("tls_server_name")

osscluster_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1656,8 +1656,8 @@ EKTcWGekdmdDPsHloRNtsiCa697B2O9IFA==
16561656
o: &redis.ClusterOptions{Addrs: []string{"localhost:123", "localhost:1234", "localhost:12345"}, TLSConfig: &tls.Config{ServerName: "localhost"}},
16571657
}, {
16581658
test: "RedissTLSParams",
1659-
url: "rediss://localhost:123?tls_server_name=abc&tls_min_version=1&tls_max_version=3&skip_verify=true",
1660-
o: &redis.ClusterOptions{Addrs: []string{"localhost:123"}, TLSConfig: &tls.Config{ServerName: "abc", MinVersion: 1, MaxVersion: 3, InsecureSkipVerify: true}},
1659+
url: "rediss://localhost:123?tls_server_name=abc&tls_min_version=771&tls_max_version=772&skip_verify=true",
1660+
o: &redis.ClusterOptions{Addrs: []string{"localhost:123"}, TLSConfig: &tls.Config{ServerName: "abc", MinVersion: 771, MaxVersion: 772, InsecureSkipVerify: true}},
16611661
}, {
16621662
test: "RedissTLSCert",
16631663
url: "rediss://localhost:123?tls_cert_file=./testdata/testcert.pem&tls_key_file=./testdata/testkey.pem",

sentinel.go

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -304,7 +304,7 @@ func (opt *FailoverOptions) clusterOptions() *ClusterOptions {
304304
// - use "skip_verify=true" to ignore TLS certificate validation
305305
// - for rediss:// URLs, additional TLS parameters are supported:
306306
// - tls_cert_file and tls_key_file: paths to client certificate and key files
307-
// - tls_min_version and tls_max_version: TLS version constraints (e.g., 771 for TLS 1.2)
307+
// - tls_min_version and tls_max_version: TLS version constraints (minimum TLS 1.2: 771, TLS 1.3: 772)
308308
// - tls_server_name: override server name for certificate validation
309309
//
310310
// Example:
@@ -435,10 +435,26 @@ func setupFailoverConnParams(u *url.URL, o *FailoverOptions) (*FailoverOptions,
435435
}
436436

437437
if q.has("tls_min_version") {
438-
o.TLSConfig.MinVersion = uint16(q.int("tls_min_version"))
438+
minVer := q.int("tls_min_version")
439+
if minVer < 0 || minVer > 65535 {
440+
return nil, fmt.Errorf("redis: invalid tls_min_version: %d (must be between 0 and 65535)", minVer)
441+
}
442+
// Enforce minimum TLS 1.2 for security
443+
if minVer > 0 && minVer < int(tls.VersionTLS12) {
444+
return nil, fmt.Errorf("redis: tls_min_version %d is insecure (minimum allowed is TLS 1.2: %d)", minVer, tls.VersionTLS12)
445+
}
446+
o.TLSConfig.MinVersion = uint16(minVer)
439447
}
440448
if q.has("tls_max_version") {
441-
o.TLSConfig.MaxVersion = uint16(q.int("tls_max_version"))
449+
maxVer := q.int("tls_max_version")
450+
if maxVer < 0 || maxVer > 65535 {
451+
return nil, fmt.Errorf("redis: invalid tls_max_version: %d (must be between 0 and 65535)", maxVer)
452+
}
453+
// Ensure max version is not lower than TLS 1.2
454+
if maxVer > 0 && maxVer < int(tls.VersionTLS12) {
455+
return nil, fmt.Errorf("redis: tls_max_version %d is insecure (minimum allowed is TLS 1.2: %d)", maxVer, tls.VersionTLS12)
456+
}
457+
o.TLSConfig.MaxVersion = uint16(maxVer)
442458
}
443459

444460
tlsServerName := q.string("tls_server_name")

0 commit comments

Comments
 (0)