Fix critical issues from follow-up security audit

A second-pass audit surfaced three severe issues missed by the previous
review, each a sibling code path of a bug class that was only partially
fixed before:

- auth: JWT session login (jwtSession.go) registered its authenticator
  even when CROSS_LOGIN_JWT_HS512_KEY was unset, leaving an empty HMAC
  key. golang-jwt verifies any HS256/HS512 signature against an empty
  key, allowing unauthenticated admin token forgery. Init() now refuses
  to register without a key, with a defense-in-depth empty-key guard in
  the keyfunc.

- repository: metric names from GraphQL ([String!]) were interpolated
  raw into json_extract(footprint, "$.<name>") SQL. SQLite parses
  double-quoted strings as literals, enabling SQL injection by any
  authenticated user. Validate metric names against ^[a-zA-Z0-9_]+$ in
  jobsMetricStatisticsHistogram and buildFloatJSONCondition.

- metricstore: cluster/host line-protocol tags flowed unvalidated into
  path.Join(RootDir, cluster, host) for checkpoint/WAL files, allowing
  arbitrary file write outside the checkpoint root via NATS
  (unauthenticated) or POST /api/write. Reject path-traversal sequences
  in DecodeLine before the tags become path components.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: b57246993ec1
This commit is contained in:
2026-06-04 19:07:20 +02:00
parent 6f7e262f3f
commit 6d86690c76
4 changed files with 58 additions and 7 deletions

View File

@@ -25,15 +25,21 @@ type JWTSessionAuthenticator struct {
var _ Authenticator = (*JWTSessionAuthenticator)(nil) var _ Authenticator = (*JWTSessionAuthenticator)(nil)
func (ja *JWTSessionAuthenticator) Init() error { func (ja *JWTSessionAuthenticator) Init() error {
if pubKey := os.Getenv("CROSS_LOGIN_JWT_HS512_KEY"); pubKey != "" { pubKey := os.Getenv("CROSS_LOGIN_JWT_HS512_KEY")
bytes, err := base64.StdEncoding.DecodeString(pubKey) if pubKey == "" {
if err != nil { // Without a configured key the HMAC verification below would run against
cclog.Warn("Could not decode cross login JWT HS512 key") // an empty key, which lets anyone forge a valid token. Refuse to register
return err // the authenticator in that case so JWT session login is simply disabled.
} return errors.New("CROSS_LOGIN_JWT_HS512_KEY not set: JWT session login disabled")
ja.loginTokenKey = bytes
} }
bytes, err := base64.StdEncoding.DecodeString(pubKey)
if err != nil {
cclog.Warn("Could not decode cross login JWT HS512 key")
return err
}
ja.loginTokenKey = bytes
cclog.Info("JWT Session authenticator successfully registered") cclog.Info("JWT Session authenticator successfully registered")
return nil return nil
} }
@@ -60,6 +66,12 @@ func (ja *JWTSessionAuthenticator) Login(
token, err := jwt.Parse(rawtoken, func(t *jwt.Token) (any, error) { token, err := jwt.Parse(rawtoken, func(t *jwt.Token) (any, error) {
if t.Method == jwt.SigningMethodHS256 || t.Method == jwt.SigningMethodHS512 { if t.Method == jwt.SigningMethodHS256 || t.Method == jwt.SigningMethodHS512 {
// Defense in depth: an empty key would verify any HMAC signature.
// Init() already refuses to register without a key, so this should
// never trigger, but guard explicitly rather than trust the chain.
if len(ja.loginTokenKey) == 0 {
return nil, errors.New("HS login key not configured")
}
return ja.loginTokenKey, nil return ja.loginTokenKey, nil
} }
return nil, fmt.Errorf("unkown signing method for login token: %s (known: HS256, HS512, EdDSA)", t.Method.Alg()) return nil, fmt.Errorf("unkown signing method for login token: %s (known: HS256, HS512, EdDSA)", t.Method.Alg())

View File

@@ -336,8 +336,18 @@ func buildTimeCondition(field string, cond *config.TimeRange, query sq.SelectBui
} }
} }
// validMetricName guards metric/footprint names that are interpolated into the
// json_extract() path of footprint queries. SQLite treats double-quoted strings
// as string literals, so an unescaped name (e.g. containing a `"`) would allow
// SQL injection. Legitimate metric names only use these characters.
var validMetricName = regexp.MustCompile(`^[a-zA-Z0-9_]+$`)
// buildFloatJSONCondition creates a filter on a numeric field within the footprint JSON column, using BETWEEN only if required. // buildFloatJSONCondition creates a filter on a numeric field within the footprint JSON column, using BETWEEN only if required.
func buildFloatJSONCondition(jsonField string, cond *model.FloatRange, query sq.SelectBuilder) sq.SelectBuilder { func buildFloatJSONCondition(jsonField string, cond *model.FloatRange, query sq.SelectBuilder) sq.SelectBuilder {
if !validMetricName.MatchString(jsonField) {
cclog.Warnf("buildFloatJSONCondition: rejecting invalid metric name %q", jsonField)
return query.Where("0 = 1")
}
query = query.Where("JSON_VALID(footprint)") query = query.Where("JSON_VALID(footprint)")
if cond.From > 0.0 && cond.To > 0.0 { if cond.From > 0.0 && cond.To > 0.0 {
return query.Where("JSON_EXTRACT(footprint, \"$."+jsonField+"\") BETWEEN ? AND ?", cond.From, cond.To) return query.Where("JSON_EXTRACT(footprint, \"$."+jsonField+"\") BETWEEN ? AND ?", cond.From, cond.To)

View File

@@ -909,6 +909,13 @@ func (r *JobRepository) jobsMetricStatisticsHistogram(
filters []*model.JobFilter, filters []*model.JobFilter,
bins *int, bins *int,
) (*model.MetricHistoPoints, error) { ) (*model.MetricHistoPoints, error) {
// The metric name is interpolated into the json_extract() path of the SQL
// below. SQLite parses double-quoted strings as literals, so reject anything
// that is not a plain metric identifier to prevent SQL injection.
if !validMetricName.MatchString(metric) {
return nil, fmt.Errorf("invalid metric name: %q", metric)
}
// Peak value defines the upper bound for binning: values are distributed across // Peak value defines the upper bound for binning: values are distributed across
// bins from 0 to peak. First try to get peak from filtered cluster, otherwise // bins from 0 to peak. First try to get peak from filtered cluster, otherwise
// scan all clusters to find the maximum peak value. // scan all clusters to find the maximum peak value.

View File

@@ -22,6 +22,7 @@ import (
"bytes" "bytes"
"context" "context"
"fmt" "fmt"
"strings"
"sync" "sync"
"sync/atomic" "sync/atomic"
"time" "time"
@@ -196,6 +197,16 @@ var decodeStatePool = sync.Pool{
// When the checkpoint format is "wal" each successfully decoded sample is also // When the checkpoint format is "wal" each successfully decoded sample is also
// sent to WALMessages so the WAL staging goroutine can persist it durably // sent to WALMessages so the WAL staging goroutine can persist it durably
// before the next binary snapshot. // before the next binary snapshot.
// isValidPathComponent reports whether s is safe to use as a single filesystem
// path component (cluster or host name) when building checkpoint/WAL paths. It
// rejects path-traversal sequences and embedded separators. An empty string is
// allowed because a missing host tag is legitimate and does not escape the root.
func isValidPathComponent(s string) bool {
return !strings.Contains(s, "..") &&
!strings.Contains(s, "/") &&
!strings.Contains(s, "\\")
}
func DecodeLine(dec *lineprotocol.Decoder, func DecodeLine(dec *lineprotocol.Decoder,
ms *MemoryStore, ms *MemoryStore,
clusterDefault string, clusterDefault string,
@@ -282,6 +293,17 @@ func DecodeLine(dec *lineprotocol.Decoder,
} }
} }
// cluster and host are taken verbatim from attacker-controllable line
// protocol tags and are later used as filesystem path components for the
// checkpoint/WAL files (path.Join(RootDir, cluster, host)). Reject
// path-traversal sequences so a malicious tag cannot escape the
// checkpoint root. Skip the offending sample; the rest of the batch is
// still processed.
if !isValidPathComponent(cluster) || !isValidPathComponent(host) {
cclog.Warnf("[METRICSTORE]> rejecting metric with invalid cluster/host tag (cluster=%q host=%q)", cluster, host)
continue
}
// If the cluster or host changed, the lvl was set to nil // If the cluster or host changed, the lvl was set to nil
if lvl == nil { if lvl == nil {
st.selector = st.selector[:2] st.selector = st.selector[:2]