Skip to content

Commit dccd614

Browse files
authored
Merge pull request #1324 from chaitin/ci-fix-management-security-findings
fix(management): close the security scan findings in mgt-api and tcon…
2 parents 2566db0 + 489af0f commit dccd614

10 files changed

Lines changed: 294 additions & 35 deletions

File tree

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
package controller
2+
3+
import (
4+
"fmt"
5+
"regexp"
6+
"strconv"
7+
"strings"
8+
)
9+
10+
const (
11+
// maxPort is the largest TCP port that may be listened on.
12+
maxPort = 65535
13+
14+
// maxServerNameLength and maxLabelLength follow the DNS limits.
15+
maxServerNameLength = 253
16+
maxLabelLength = 63
17+
)
18+
19+
var (
20+
// portPattern matches the decimal notation of a TCP port.
21+
portPattern = regexp.MustCompile(`^[0-9]{1,5}$`)
22+
23+
// serverNameLabelPattern matches one label of a literal host name. The
24+
// accepted character set is deliberately narrow: the value ends up in an
25+
// nginx "server_name" directive, so anything that could terminate the
26+
// directive (or start another one) must not pass.
27+
serverNameLabelPattern = regexp.MustCompile(`^[A-Za-z0-9_]([A-Za-z0-9_-]*[A-Za-z0-9_])?$`)
28+
)
29+
30+
// validatePort accepts the decimal notation of a TCP port. It rejects any
31+
// other value because the port is interpolated into a "listen" directive.
32+
func validatePort(port string) error {
33+
if !portPattern.MatchString(port) {
34+
return fmt.Errorf("invalid port %q: only digits are allowed", port)
35+
}
36+
37+
value, err := strconv.Atoi(port)
38+
if err != nil || value < 1 || value > maxPort {
39+
return fmt.Errorf("invalid port %q: must be between 1 and %d", port, maxPort)
40+
}
41+
42+
return nil
43+
}
44+
45+
// validateServerName accepts a literal host name, optionally with a wildcard
46+
// as its first label. It rejects any other value because the name is
47+
// interpolated into a "server_name" directive.
48+
func validateServerName(name string) error {
49+
if len(name) > maxServerNameLength {
50+
return fmt.Errorf("invalid server name %q: longer than %d characters", name, maxServerNameLength)
51+
}
52+
53+
for i, label := range strings.Split(name, ".") {
54+
if label == "*" {
55+
// nginx only accepts a wildcard as the left most label.
56+
if i != 0 {
57+
return fmt.Errorf("invalid server name %q: wildcard must be the first label", name)
58+
}
59+
continue
60+
}
61+
62+
if len(label) > maxLabelLength || !serverNameLabelPattern.MatchString(label) {
63+
return fmt.Errorf("invalid server name %q: %q is not a valid host name label", name, label)
64+
}
65+
}
66+
67+
return nil
68+
}

‎management/tcontrollerd/controller/website.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,13 +70,18 @@ func generateNginxConfig(website *model.WebsiteConfig) (string, error) {
7070
if sn == "*" || sn == "" {
7171
sn = "_"
7272
addrAnyProperties = addrAnyPropertiesTpl
73+
} else if err := validateServerName(sn); err != nil {
74+
return "", err
7375
}
7476
serverName = fmt.Sprintf(serverNameTpl, sn)
7577
}
7678

7779
// only ONE port supported in v1.0
7880
var serverListen string
7981
for _, port := range website.Ports {
82+
if err := validatePort(port); err != nil {
83+
return "", err
84+
}
8085
serverListen = fmt.Sprintf(serverListenTpl, port, sslFlag, addrAnyProperties)
8186
}
8287

‎management/webserver/api/auth.go‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515
"chaitin.cn/patronus/safeline-2/management/webserver/pkg/constants"
1616
"chaitin.cn/patronus/safeline-2/management/webserver/pkg/database"
1717
"chaitin.cn/patronus/safeline-2/management/webserver/pkg/log"
18+
"chaitin.cn/patronus/safeline-2/management/webserver/pkg/sessionopts"
1819
)
1920

2021
var logger = log.GetLogger("api")
@@ -70,14 +71,7 @@ func PostLogin(c *gin.Context) {
7071
db.Save(&user)
7172

7273
session := sessions.Default(c)
73-
session.Options(sessions.Options{
74-
Path: "/",
75-
MaxAge: 3600 * 24 * 7,
76-
//Domain: options.Domain,
77-
//HttpOnly: true,
78-
//SameSite: http.SameSiteLaxMode,
79-
//Secure: false,
80-
})
74+
session.Options(sessionopts.Options(c))
8175
session.Set(constants.DefaultSessionUserKey, user.ID)
8276
if err := session.Save(); err != nil {
8377
logger.Error(err)

‎management/webserver/api/website.go‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,12 @@ func PostWebsite(ctx *gin.Context) {
5353
return
5454
}
5555

56+
if err := params.Validate(); err != nil {
57+
logger.Warn(err)
58+
response.Error(ctx, response.JSONBody{Err: response.ErrInternalError, Msg: err.Error()}, http.StatusBadRequest)
59+
return
60+
}
61+
5662
db := database.GetDB()
5763
err := db.Transaction(func(tx *gorm.DB) error {
5864
website := &model.Website{Comment: params.Comment, ServerNames: params.ServerNames, Upstreams: params.Upstreams, Ports: params.Ports,
@@ -90,6 +96,12 @@ func PutWebsite(ctx *gin.Context) {
9096
return
9197
}
9298

99+
if err := params.Validate(); err != nil {
100+
logger.Warn(err)
101+
response.Error(ctx, response.JSONBody{Err: response.ErrInternalError, Msg: err.Error()}, http.StatusBadRequest)
102+
return
103+
}
104+
93105
db := database.GetDB()
94106
err := db.Transaction(func(tx *gorm.DB) error {
95107
var website model.Website

‎management/webserver/main.go‎

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -139,15 +139,13 @@ func main() {
139139

140140
var option model.Options
141141
database.GetDB().Where(&model.Options{Key: constants.SecretKey}).First(&option)
142-
logger.Debugf("Secret: %s", option.Value)
143142
store := cookie.NewStore([]byte(option.Value))
144143
r.Use(sessions.Sessions("session", store))
145144

146145
publicRouters := r.Group("/api")
147146
publicRouters.POST(api.Login, api.PostLogin)
148147
publicRouters.POST(api.Logout, api.PostLogout)
149148
publicRouters.POST(api.Behaviour, api.PostBehaviour)
150-
publicRouters.POST(api.FalsePositives, api.PostFalsePositives)
151149
publicRouters.GET(api.OTPUrl, api.GetOTPUrl)
152150
publicRouters.GET(api.Version, api.GetVersion)
153151
publicRouters.GET(api.UpgradeTips, api.GetUpgradeTips)
@@ -159,22 +157,21 @@ func main() {
159157
})
160158

161159
limitedRouters := r.Group("/api")
162-
noAuth, existed := os.LookupEnv("NO_AUTH")
163-
if existed && len(noAuth) >= 0 {
164-
logger.Warn("No auth")
160+
if envEnabled(os.Getenv("NO_AUTH")) {
161+
logger.Warn("Authentication is disabled because NO_AUTH is enabled")
165162
} else {
166163
limitedRouters.Use(middleware.AuthRequired)
167164
}
168-
readOnly, existed := os.LookupEnv("READ_ONLY")
169-
if existed && len(readOnly) >= 0 {
170-
logger.Warn("Read only")
165+
if envEnabled(os.Getenv("READ_ONLY")) {
166+
logger.Warn("Write requests are rejected because READ_ONLY is enabled")
171167
limitedRouters.Use(middleware.ReadOnly)
172168
}
173169

174170
limitedRouters.GET(api.User, api.GetUser)
175171

176172
limitedRouters.GET(api.DetectLogList, api.GetDetectLogList)
177173
limitedRouters.GET(api.DetectLogDetail, api.GetDetectLogDetail)
174+
limitedRouters.POST(api.FalsePositives, api.PostFalsePositives)
178175

179176
limitedRouters.POST(api.Website, api.PostWebsite)
180177
limitedRouters.PUT(api.Website, api.PutWebsite)
@@ -208,3 +205,16 @@ func main() {
208205
logger.Fatalln("Error occurred when running web server: ", err)
209206
}
210207
}
208+
209+
// envEnabled reports whether an environment variable holds an explicit truthy
210+
// value. Switches such as NO_AUTH and READ_ONLY disable a protection, so only
211+
// values that actually ask for it may enable them: an empty string (or any
212+
// other unparsable value) must never do it.
213+
func envEnabled(value string) bool {
214+
switch strings.ToLower(strings.TrimSpace(value)) {
215+
case "1", "true", "yes", "on":
216+
return true
217+
default:
218+
return false
219+
}
220+
}

‎management/webserver/middleware/auth.go‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88

99
"chaitin.cn/patronus/safeline-2/management/webserver/api/response"
1010
"chaitin.cn/patronus/safeline-2/management/webserver/pkg/constants"
11+
"chaitin.cn/patronus/safeline-2/management/webserver/pkg/sessionopts"
1112
)
1213

1314
func AuthRequired(c *gin.Context) {
@@ -21,14 +22,7 @@ func AuthRequired(c *gin.Context) {
2122
}
2223

2324
// extend session expired time
24-
session.Options(sessions.Options{
25-
Path: "/",
26-
MaxAge: 3600 * 24 * 7,
27-
//Domain: options.Domain,
28-
//HttpOnly: true,
29-
//SameSite: http.SameSiteLaxMode,
30-
//Secure: false,
31-
})
25+
session.Options(sessionopts.Options(c))
3226

3327
if err := session.Save(); err != nil {
3428
response.Error(c, response.JSONBody{Err: response.ErrInternalError, Msg: "Error occurred when creating sessions"}, http.StatusInternalServerError)

‎management/webserver/model/website.go‎

Lines changed: 119 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,34 @@
11
package model
22

3-
import "gorm.io/datatypes"
3+
import (
4+
"encoding/json"
5+
"fmt"
6+
"regexp"
7+
"strconv"
8+
"strings"
9+
10+
"gorm.io/datatypes"
11+
)
12+
13+
const (
14+
// maxPort is the largest TCP port that may be listened on.
15+
maxPort = 65535
16+
17+
// maxServerNameLength and maxLabelLength follow the DNS limits.
18+
maxServerNameLength = 253
19+
maxLabelLength = 63
20+
)
21+
22+
var (
23+
// portPattern matches the decimal notation of a TCP port.
24+
portPattern = regexp.MustCompile(`^[0-9]{1,5}$`)
25+
26+
// serverNameLabelPattern matches one label of a literal host name. The
27+
// accepted character set is deliberately narrow: the value ends up in an
28+
// nginx "server_name" directive, so anything that could terminate the
29+
// directive (or start another one) must not pass.
30+
serverNameLabelPattern = regexp.MustCompile(`^[A-Za-z0-9_]([A-Za-z0-9_-]*[A-Za-z0-9_])?$`)
31+
)
432

533
type Website struct {
634
Base
@@ -14,3 +42,93 @@ type Website struct {
1442

1543
IsEnabled bool `gorm:"is_enabled;default=true" json:"is_enabled"`
1644
}
45+
46+
// Validate checks the fields that tcontrollerd renders into the nginx
47+
// configuration of the site.
48+
//
49+
// Ports and server names are interpolated into nginx directives, so they have
50+
// to be plain ports and host names: a value carrying nginx syntax must be
51+
// rejected here, before it is stored and pushed to the engine.
52+
func (w *Website) Validate() error {
53+
serverNames, err := unmarshalStringList(w.ServerNames)
54+
if err != nil {
55+
return fmt.Errorf("invalid server_names: %v", err)
56+
}
57+
58+
ports, err := unmarshalStringList(w.Ports)
59+
if err != nil {
60+
return fmt.Errorf("invalid ports: %v", err)
61+
}
62+
63+
for _, serverName := range serverNames {
64+
if err = ValidateServerName(serverName); err != nil {
65+
return err
66+
}
67+
}
68+
69+
for _, port := range ports {
70+
if err = ValidatePort(port); err != nil {
71+
return err
72+
}
73+
}
74+
75+
return nil
76+
}
77+
78+
// ValidatePort accepts the decimal notation of a TCP port.
79+
func ValidatePort(port string) error {
80+
if !portPattern.MatchString(port) {
81+
return fmt.Errorf("invalid port %q: only digits are allowed", port)
82+
}
83+
84+
value, err := strconv.Atoi(port)
85+
if err != nil || value < 1 || value > maxPort {
86+
return fmt.Errorf("invalid port %q: must be between 1 and %d", port, maxPort)
87+
}
88+
89+
return nil
90+
}
91+
92+
// ValidateServerName accepts a literal host name, optionally with a wildcard
93+
// as its first label, or "*" for the default site.
94+
func ValidateServerName(name string) error {
95+
// "*" (and the empty string, which the console sends for the same purpose)
96+
// makes the site the default server and is rendered as "_".
97+
if name == "*" || name == "" {
98+
return nil
99+
}
100+
101+
if len(name) > maxServerNameLength {
102+
return fmt.Errorf("invalid server name %q: longer than %d characters", name, maxServerNameLength)
103+
}
104+
105+
for i, label := range strings.Split(name, ".") {
106+
if label == "*" {
107+
// nginx only accepts a wildcard as the left most label.
108+
if i != 0 {
109+
return fmt.Errorf("invalid server name %q: wildcard must be the first label", name)
110+
}
111+
continue
112+
}
113+
114+
if len(label) > maxLabelLength || !serverNameLabelPattern.MatchString(label) {
115+
return fmt.Errorf("invalid server name %q: %q is not a valid host name label", name, label)
116+
}
117+
}
118+
119+
return nil
120+
}
121+
122+
// unmarshalStringList decodes one of the JSON list columns of a website.
123+
func unmarshalStringList(raw datatypes.JSON) ([]string, error) {
124+
if len(raw) == 0 {
125+
return nil, nil
126+
}
127+
128+
var values []string
129+
if err := json.Unmarshal(raw, &values); err != nil {
130+
return nil, err
131+
}
132+
133+
return values, nil
134+
}

‎management/webserver/pkg/fvm/fvm.go‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ import (
1919
"C" //nolint:typecheck
2020
)
2121
import (
22-
"crypto/tls"
2322
"fmt"
2423
"io"
2524
"net/http"
@@ -209,11 +208,11 @@ func (f *FVM) PushFsl(server string, update *FVMUpdate) error {
209208
req.Header.Add("Content-Type", "application/octet-stream")
210209
// req.Header.Set("Connection", "close")
211210

212-
httpClient := &http.Client{
213-
Transport: &http.Transport{
214-
TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
215-
},
216-
}
211+
// The compiled policy is pushed to the detector over the container network
212+
// and replaces the bytecode the engine executes, so the certificate of an
213+
// HTTPS detector must be verified: a man in the middle could otherwise
214+
// replace the policy and silently disable the WAF.
215+
httpClient := &http.Client{}
217216

218217
var (
219218
respErr error

0 commit comments

Comments
 (0)