Skip to content

Commit fb54992

Browse files
authored
Merge pull request #1330 from chaitin/ci-fix-fourth-scan-findings
fix(management,mcp-server,yanshi): harden the remaining rescan findings
2 parents 50bb315 + c5bac20 commit fb54992

7 files changed

Lines changed: 396 additions & 40 deletions

File tree

‎management/webserver/api/auth.go‎

Lines changed: 130 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,8 @@ func PostLogin(c *gin.Context) {
115115
return
116116
}
117117

118-
usedStep, valid := verifyPasscode(user.TFASecret, params.Passcode, user.TFALastUsedStep, time.Now())
118+
now := time.Now()
119+
usedStep, valid := verifyPasscode(user.TFASecret, params.Passcode, user.TFALastUsedStep, now)
119120
if !valid {
120121
loginThrottle.fail(keys)
121122
logger.Warnf("Failed login attempt from %s", c.ClientIP())
@@ -137,17 +138,25 @@ func PostLogin(c *gin.Context) {
137138
return
138139
}
139140

140-
loginThrottle.reset(keys)
141+
// The passcode is spent here, in one conditional update, and the session
142+
// below is only created when this request is the one that spent it. A plain
143+
// Save after the check would leave a window in which two requests that
144+
// carry the same passcode both read the same stored time step and both
145+
// accept it.
146+
consumed, err := consumePasscode(db, &user, usedStep, now.Unix()/int64(OtpOpts.Period), now)
147+
if err != nil {
148+
logger.Error(err)
149+
response.Error(c, response.JSONBody{Err: response.ErrInternalError, Msg: "Error occurred when creating sessions"}, http.StatusInternalServerError)
150+
return
151+
}
152+
if !consumed {
153+
loginThrottle.fail(keys)
154+
logger.Warnf("Refused a login from %s: the passcode of time step %d was already used", c.ClientIP(), usedStep)
155+
response.Error(c, response.JSONBody{Err: response.ErrWrongPasscode, Msg: "Failed to verify your passcode"}, http.StatusUnauthorized)
156+
return
157+
}
141158

142-
user.LastLoginTime = time.Now().Unix()
143-
user.IsEnabled = true
144-
// The bootstrap is over: the secret is bound, so it is no longer pending
145-
// and no longer tied to the address that fetched it.
146-
user.TFABootstrapIP = ""
147-
// Remember the time step of the accepted passcode so that the same one
148-
// cannot be used a second time.
149-
user.TFALastUsedStep = usedStep
150-
db.Save(&user)
159+
loginThrottle.reset(keys)
151160

152161
session := sessions.Default(c)
153162
session.Options(sessionopts.Options(c))
@@ -214,11 +223,18 @@ func GetOTPUrl(c *gin.Context) {
214223
// An installation that already carries a pending secret from a
215224
// release that did not record the address keeps working: the first
216225
// caller adopts it, exactly as it did before.
217-
user.TFABootstrapIP = clientIP
218-
db.Save(&user)
226+
adopted, err := adoptBootstrapSecret(db, user.TFASecret, clientIP)
227+
if err != nil {
228+
logger.Error(err)
229+
response.Error(c, response.JSONBody{Err: response.ErrInternalError, Msg: "Error occurred when generating otp qrcode"}, http.StatusInternalServerError)
230+
return
231+
}
232+
if !adopted {
233+
refusePendingSecret(c, clientIP)
234+
return
235+
}
219236
} else if user.TFABootstrapIP != clientIP {
220-
logger.Warnf("Refused to hand out the pending TFA secret to %s: it was issued to %s", clientIP, user.TFABootstrapIP)
221-
response.Error(c, response.JSONBody{Err: response.ErrLoginRequired, Msg: "The pending TFA secret belongs to another address, run 'mgt -reset_user <user>' to bind a new one"}, http.StatusForbidden)
237+
refusePendingSecret(c, clientIP)
222238
return
223239
}
224240

@@ -234,13 +250,109 @@ func GetOTPUrl(c *gin.Context) {
234250
return
235251
}
236252

237-
user.TFASecret = otpKey.Secret()
238-
user.TFABootstrapIP = clientIP
239-
db.Save(&user)
253+
// The secret is installed by an update that only matches while the row
254+
// still has none, so two calls that arrive together cannot both be told
255+
// that the secret they generated is the one the account carries: the
256+
// caller that loses reads the stored secret below and hands that one out
257+
// when it belongs to the same address, which is what a console that
258+
// retries produces, and is refused otherwise.
259+
issued, err := claimBootstrapSecret(db, clientIP, otpKey.Secret())
260+
if err != nil {
261+
logger.Error(err)
262+
response.Error(c, response.JSONBody{Err: response.ErrInternalError, Msg: "Error occurred when generating otp qrcode"}, http.StatusInternalServerError)
263+
return
264+
}
265+
if !issued {
266+
logger.Warnf("Lost the TFA bootstrap race to another caller, %s is served the stored secret if it is its own", clientIP)
267+
var stored model.User
268+
db.Where(&model.User{Username: constants.SuperUser}).First(&stored)
269+
if stored.TFASecret == "" || stored.TFABootstrapIP != clientIP {
270+
refusePendingSecret(c, clientIP)
271+
return
272+
}
273+
274+
response.Success(c, gin.H{"url": otpURL(stored.TFASecret)})
275+
return
276+
}
277+
240278
logger.Warnf("Issued a new TFA secret to %s; it can only be bound and used from that address until the first login", clientIP)
241279
response.Success(c, gin.H{"url": otpKey.URL()})
242280
}
243281

282+
// refusePendingSecret answers a caller that may not be handed the pending TFA
283+
// secret, because it was issued to another address.
284+
func refusePendingSecret(c *gin.Context, clientIP string) {
285+
logger.Warnf("Refused to hand out the pending TFA secret to %s: it belongs to another address", clientIP)
286+
response.Error(c, response.JSONBody{Err: response.ErrLoginRequired, Msg: "The pending TFA secret belongs to another address, run 'mgt -reset_user <user>' to bind a new one"}, http.StatusForbidden)
287+
}
288+
289+
// claimBootstrapSecret installs secret as the pending TFA secret of the
290+
// account and reports whether this call is the one that installed it.
291+
//
292+
// The update matches only while the row carries no secret yet, which is what
293+
// makes the claim exclusive. Reading the row and writing it back would let two
294+
// callers that arrive while the account has no secret both store the one they
295+
// generated, so both would be told the account is bound to the secret they
296+
// hold, while the database keeps only the last write.
297+
func claimBootstrapSecret(db *database.PostgresDB, clientIP, secret string) (bool, error) {
298+
result := db.Model(&model.User{}).
299+
Where("username = ? AND (tfa_secret = '' OR tfa_secret IS NULL)", constants.SuperUser).
300+
Updates(map[string]interface{}{
301+
"tfa_secret": secret,
302+
"tfa_bootstrap_ip": clientIP,
303+
})
304+
if result.Error != nil {
305+
return false, result.Error
306+
}
307+
308+
return result.RowsAffected == 1, nil
309+
}
310+
311+
// adoptBootstrapSecret records clientIP as the address of a pending secret
312+
// that was stored before the address was recorded.
313+
//
314+
// The update is restricted to the secret that was read, so a caller that lost a
315+
// race against the installation of a new secret adopts nothing.
316+
func adoptBootstrapSecret(db *database.PostgresDB, pending, clientIP string) (bool, error) {
317+
result := db.Model(&model.User{}).
318+
Where("username = ? AND tfa_secret = ? AND (tfa_bootstrap_ip = '' OR tfa_bootstrap_ip IS NULL)", constants.SuperUser, pending).
319+
Updates(map[string]interface{}{
320+
"tfa_bootstrap_ip": clientIP,
321+
})
322+
if result.Error != nil {
323+
return false, result.Error
324+
}
325+
326+
return result.RowsAffected == 1, nil
327+
}
328+
329+
// consumePasscode records an accepted passcode together with the login it
330+
// belongs to, as a single conditional update.
331+
//
332+
// The update is what decides whether the time step of the passcode was already
333+
// spent: it only matches while the stored step is below the accepted one, so
334+
// the second of two concurrent requests that carry the same passcode affects no
335+
// row and is treated as a replay. currentStep is the step the passcode was
336+
// verified against; a stored step ahead of it means the clock moved backwards,
337+
// which this process treats as stale (see verifyPasscode) and repairs here.
338+
func consumePasscode(db *database.PostgresDB, user *model.User, usedStep, currentStep int64, now time.Time) (bool, error) {
339+
result := db.Model(&model.User{}).
340+
Where("id = ? AND (tfa_last_used_step < ? OR tfa_last_used_step > ?)", user.ID, usedStep, currentStep+1).
341+
Updates(map[string]interface{}{
342+
"tfa_last_used_step": usedStep,
343+
// The bootstrap is over: the secret is bound, so it is no longer
344+
// pending and no longer tied to the address that fetched it.
345+
"tfa_bootstrap_ip": "",
346+
"last_login_time": now.Unix(),
347+
"is_enabled": true,
348+
})
349+
if result.Error != nil {
350+
return false, result.Error
351+
}
352+
353+
return result.RowsAffected == 1, nil
354+
}
355+
244356
// verifyPasscode checks a TOTP passcode and returns the time step it belongs to.
245357
//
246358
// lastUsedStep is the step of the passcode that was accepted last; every step

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

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,18 @@ func (u *FVMUpdate) ToBytes() []byte {
5959
return C.GoBytes(unsafe.Pointer(u.update.buf.ptr), C.int(u.update.buf.length))
6060
}
6161

62+
// FromBytes builds an update from the serialized byte code, as it is stored in
63+
// the database.
64+
//
65+
// An empty buffer is rejected before the address of its first byte is taken:
66+
// &out[0] panics on a zero length slice, and fvm_update_create would have to
67+
// read the buffer that the length describes.
6268
func (u *FVMUpdate) FromBytes(out []byte) error {
63-
ptr := C.build_update(unsafe.Pointer(&out[0]), C.ulong(len(out)))
69+
if len(out) == 0 {
70+
return errors.New("refuse to create an update from an empty buffer")
71+
}
72+
73+
ptr := C.build_update(maybePointer(out), C.ulong(len(out)))
6474
if ptr == nil {
6575
return errors.New("failed to create update from db")
6676
}

‎mcp_server/internal/api/client.go‎

Lines changed: 50 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,14 @@ import (
1616
)
1717

1818
const (
19+
// tokenHeader carries the deployment token of the configured instance.
20+
tokenHeader = "X-SLCE-API-TOKEN"
21+
1922
maxErrorBodySize = 4 << 10
2023
maxResponseBodySize = 4 << 20
24+
25+
// maxRedirects bounds how many redirects one request may follow.
26+
maxRedirects = 10
2127
)
2228

2329
// Client is the transport used by a single SafeLine instance. Its request
@@ -50,15 +56,58 @@ func newClient(baseURL string, timeout time.Duration, insecureSkipVerify bool, t
5056
httpClient: &http.Client{
5157
Timeout: timeout,
5258
Transport: transport,
59+
// Every request carries the deployment token in X-SLCE-API-TOKEN.
60+
// net/http drops the Authorization header when a redirect leaves
61+
// the origin, but it knows nothing about this header, so a 3xx
62+
// from the configured instance would hand the token to whatever
63+
// host the Location header names. Redirects are therefore followed
64+
// only while they stay on the configured origin.
65+
CheckRedirect: sameOriginRedirects(parsedURL),
5366
},
5467
headers: make(http.Header),
5568
}
5669
client.headers.Set("Accept", "application/json")
5770
client.headers.Set("User-Agent", "SafeLine-MCP/1.0")
58-
client.headers.Set("X-SLCE-API-TOKEN", token)
71+
client.headers.Set(tokenHeader, token)
5972
return client, nil
6073
}
6174

75+
// sameOriginRedirects returns the redirect policy of a client: a redirect is
76+
// followed only while it stays on the scheme, host and port of the configured
77+
// instance. Anything else fails the request instead of replaying the token.
78+
func sameOriginRedirects(base *url.URL) func(*http.Request, []*http.Request) error {
79+
return func(req *http.Request, via []*http.Request) error {
80+
if len(via) >= maxRedirects {
81+
return fmt.Errorf("stopped after %d redirects", maxRedirects)
82+
}
83+
if !sameOrigin(base, req.URL) {
84+
return fmt.Errorf("refused to follow a redirect to %s: it would send the instance token to another origin", req.URL.Redacted())
85+
}
86+
87+
return nil
88+
}
89+
}
90+
91+
// sameOrigin reports whether target has the same scheme, host and port as base.
92+
func sameOrigin(base, target *url.URL) bool {
93+
return strings.EqualFold(base.Scheme, target.Scheme) &&
94+
strings.EqualFold(base.Hostname(), target.Hostname()) &&
95+
effectivePort(base) == effectivePort(target)
96+
}
97+
98+
// effectivePort returns the port a URL connects to, including the default port
99+
// of its scheme.
100+
func effectivePort(u *url.URL) string {
101+
if port := u.Port(); port != "" {
102+
return port
103+
}
104+
if strings.EqualFold(u.Scheme, "https") {
105+
return "443"
106+
}
107+
108+
return "80"
109+
}
110+
62111
func (c *Client) get(ctx context.Context, path string, query url.Values, result any) error {
63112
endpoint := *c.baseURL
64113
endpoint.Path = strings.TrimRight(endpoint.Path, "/") + path

0 commit comments

Comments
 (0)