diff --git a/cli/internal/awsapi/awsapi.go b/cli/internal/awsapi/awsapi.go index 500ee3ca..15f6a578 100644 --- a/cli/internal/awsapi/awsapi.go +++ b/cli/internal/awsapi/awsapi.go @@ -262,7 +262,7 @@ func toError(s *Service, err error) *Error { ce = core.Errf(http.StatusConflict, "Conflict", "the resource was modified concurrently; retry") default: log.Printf("aws %s: internal error: %v", s.Name, err) - ce = core.Errf(http.StatusInternalServerError, "InternalError", "%v", err) + ce = core.Errf(http.StatusInternalServerError, "InternalError", "%s", core.InternalErrorMessage) } code := "" if s != nil && s.ErrorCode != nil { @@ -311,7 +311,11 @@ func Match(r *http.Request) bool { return lookupUnsigned(r) != nil } -const maxBody = 100 << 20 +const ( + maxBody = 100 << 20 + // maxPublicBody bounds what an unsigned (public operation) request may send. + maxPublicBody = 1 << 20 +) func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { start := time.Now() @@ -388,19 +392,49 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { q.Protocol = Query } - // Read the body (unless the service streams it) and check the signature. + // Before buffering a body, make sure the caller is worth it: the signature + // must be fresh and name a live access key, and when the client declared the + // payload hash the signature is verified before the body is read. Otherwise + // anyone could make the server buffer maxBody bytes per connection. payloadHash := "" + var secret string + var principal *httpx.Principal + verified := false + now := time.Now() + if h.Now != nil { + now = h.Now() + } if sig != nil { payloadHash = sig.PayloadHash + if err := sig.checkTime(now); err != nil { + q.fail(err) + return + } + var err error + if secret, principal, err = h.Creds.SigningSecret(sig.AccessKeyID, sig.SessionToken); err != nil { + q.fail(err) + return + } + if payloadHash != "" && !strings.HasPrefix(payloadHash, "STREAMING-") || sig.Presigned { + if err := sig.Verify(r, secret, payloadHash, now); err != nil { + q.fail(err) + return + } + verified = true + } } if !(q.Svc.StreamBody && q.Protocol == REST) { - b, err := io.ReadAll(io.LimitReader(r.Body, maxBody+1)) + limit := int64(maxBody) + if sig == nil { + limit = maxPublicBody // only public operations are reachable unsigned + } + b, err := io.ReadAll(io.LimitReader(r.Body, limit+1)) if err != nil { q.fail(Errorf(http.StatusBadRequest, "IncompleteBody", "read body: %v", err)) return } - if len(b) > maxBody { - q.fail(Errorf(http.StatusRequestEntityTooLarge, "RequestEntityTooLarge", "request body exceeds %d MB", maxBody>>20)) + if int64(len(b)) > limit { + q.fail(Errorf(http.StatusRequestEntityTooLarge, "RequestEntityTooLarge", "request body exceeds %d MB", limit>>20)) return } q.Body = b @@ -431,18 +465,12 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { public := (q.Svc.PublicOps[q.Op] && q.Protocol != REST) || (q.Protocol == REST && q.Svc.Unsigned != nil) if sig != nil { - now := time.Now() - if h.Now != nil { - now = h.Now() - } - secret, p, err := h.Creds.SigningSecret(sig.AccessKeyID, sig.SessionToken) - if err != nil { - q.fail(err) - return - } - if err := sig.Verify(r, secret, payloadHash, now); err != nil { - q.fail(err) - return + p := principal + if !verified { + if err := sig.Verify(r, secret, payloadHash, now); err != nil { + q.fail(err) + return + } } q.Sig, q.Secret, q.P = sig, secret, p p.AddRequestContext(r) diff --git a/cli/internal/awsapi/preauth_test.go b/cli/internal/awsapi/preauth_test.go new file mode 100644 index 00000000..57ab1d67 --- /dev/null +++ b/cli/internal/awsapi/preauth_test.go @@ -0,0 +1,93 @@ +package awsapi + +import ( + "errors" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/homecloudhq/homecloud/cli/internal/httpx" +) + +type noCreds struct{} + +func (noCreds) SigningSecret(string, string) (string, *httpx.Principal, error) { + return "", nil, Errorf(http.StatusForbidden, "InvalidClientTokenId", "The security token included in the request is invalid.") +} + +// tripBody fails the test when the server reads it. +type tripBody struct{ t *testing.T } + +func (b tripBody) Read([]byte) (int, error) { + b.t.Error("the body of an unauthenticated request was read") + return 0, errors.New("read") +} +func (tripBody) Close() error { return nil } + +const testSvc = "regsvc" + +func init() { + Register(&Service{Name: testSvc, JSONPrefix: "RegSvc", Ops: map[string]Op{"Ping": func(*Req) (any, error) { return nil, nil }}, + PublicOps: map[string]bool{"Open": true}}) +} + +func signedReq(t *testing.T, signed string, body *strings.Reader) *http.Request { + now := time.Now().UTC().Format(amzDateFormat) + r := httptest.NewRequest(http.MethodPost, "/", body) + r.Header.Set("X-Amz-Date", now) + r.Header.Set("X-Amz-Target", "RegSvc.Ping") + r.Header.Set("Authorization", "AWS4-HMAC-SHA256 Credential=AKIAUNKNOWN/"+now[:8]+"/us-east-1/"+testSvc+"/aws4_request, SignedHeaders="+signed+", Signature=00") + return r +} + +// A request signed with an unknown access key must be refused before its body +// is buffered: otherwise anyone can make the server allocate up to maxBody bytes +// per connection. +func TestBodyNotReadBeforeAuthentication(t *testing.T) { + h := &Handler{Creds: noCreds{}} + r := signedReq(t, "host;x-amz-date;x-amz-target", strings.NewReader("")) + r.Body = tripBody{t} + w := httptest.NewRecorder() + h.ServeHTTP(w, r) + if w.Code != http.StatusForbidden { + t.Fatalf("status %d: %s", w.Code, w.Body) + } +} + +func TestUnsignedBodyIsCapped(t *testing.T) { + h := &Handler{Creds: noCreds{}} + r := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(strings.Repeat("a", maxPublicBody+10))) + r.Header.Set("X-Amz-Target", "RegSvc.Open") + w := httptest.NewRecorder() + h.ServeHTTP(w, r) + if w.Code != http.StatusRequestEntityTooLarge { + t.Fatalf("status %d: %s", w.Code, w.Body) + } +} + +// Unexpected failures carry file paths and daemon output: they belong in the +// server log, not in the response, the audit trail or other users' LookupEvents. +func TestInternalErrorsAreNotEchoed(t *testing.T) { + e := toError(&Service{Name: "x"}, errors.New("open /home/svc/.homecloud/state.json: permission denied")) + if e.Code != "InternalFailure" || strings.Contains(e.Message, "/home/svc") { + t.Fatalf("internal error leaked: %+v", e) + } + w := httptest.NewRecorder() + httpx.WriteError(w, errors.New("docker: dial unix /var/run/docker.sock: connect: permission denied")) + if w.Code != 500 || strings.Contains(w.Body.String(), "docker.sock") { + t.Fatalf("native API leaked an internal error: %d %s", w.Code, w.Body) + } +} + +func TestHostAndTargetMustBeSigned(t *testing.T) { + for _, signed := range []string{"x-amz-date", "host;x-amz-date"} { + if _, err := ParseSignature(signedReq(t, signed, strings.NewReader(""))); err == nil { + t.Errorf("SignedHeaders=%s accepted", signed) + } + } + if _, err := ParseSignature(signedReq(t, "host;x-amz-date;x-amz-target", strings.NewReader(""))); err != nil { + t.Error(err) + } +} diff --git a/cli/internal/awsapi/sigv4.go b/cli/internal/awsapi/sigv4.go index 280fb723..9e302382 100644 --- a/cli/internal/awsapi/sigv4.go +++ b/cli/internal/awsapi/sigv4.go @@ -91,6 +91,15 @@ func ParseSignature(r *http.Request) (*Signature, error) { if len(parts) != 5 || parts[4] != "aws4_request" || s.Signature == "" || len(s.SignedHeaders) == 0 { return nil, Errorf(http.StatusBadRequest, "IncompleteSignature", "the request signature is malformed") } + // As AWS does, insist that the host (and the operation header) are signed, so a + // captured signature can't be replayed against another host or operation. + signed := map[string]bool{} + for _, h := range s.SignedHeaders { + signed[h] = true + } + if !signed["host"] || (r.Header.Get("X-Amz-Target") != "" && !signed["x-amz-target"]) { + return nil, Errorf(http.StatusForbidden, "SignatureDoesNotMatch", "the host and x-amz-target headers must be signed") + } s.AccessKeyID, s.Date, s.Region, s.Service = parts[0], parts[1], parts[2], parts[3] t, err := time.Parse(amzDateFormat, dateStr) if err != nil { diff --git a/cli/internal/core/core.go b/cli/internal/core/core.go index fc1c0a0b..32d17242 100644 --- a/cli/internal/core/core.go +++ b/cli/internal/core/core.go @@ -243,6 +243,24 @@ func CanonicalARN(s string) string { return strings.Join(parts, ":") } +// InternalErrorMessage is what clients are told when a request fails for a +// reason that is not a *Error: the real error (file paths, Docker daemon output, +// database errors) goes to the server log, not to the caller or the audit trail. +const InternalErrorMessage = "an internal error occurred; details are in the server log" + +// IsLocalARN reports whether arn names a resource of this deployment: the same +// partition and account, and the deployment's region (or none, for global +// services). Code that delivers to "the resource named by this ARN" by looking +// up only its name must check this first, or an ARN for another account or +// region would be authorized as one resource and served as another. +func IsLocalARN(arn, account string) bool { + parts := strings.SplitN(CanonicalARN(arn), ":", 6) + if len(parts) != 6 || parts[0] != "arn" || parts[1] != Partition || parts[4] != account { + return false + } + return parts[3] == Region || (parts[3] == "" && globalServices[parts[2]]) +} + func Now() time.Time { return time.Now().UTC().Truncate(time.Second) } // Error is an API error with an AWS-style code and an HTTP status. diff --git a/cli/internal/core/targets.go b/cli/internal/core/targets.go index 1c3a4b92..4a0a815f 100644 --- a/cli/internal/core/targets.go +++ b/cli/internal/core/targets.go @@ -7,6 +7,8 @@ import ( "net" "net/http" "net/url" + "os" + "regexp" "strings" "syscall" "time" @@ -32,13 +34,101 @@ func TargetAction(arn string) string { return "" } -// blockedIP reports addresses outbound webhooks may not reach: loopback, -// link-local (including cloud metadata), unspecified and multicast. +var ecrHostedImage = regexp.MustCompile(`^[0-9]{12}\.dkr\.ecr\.[a-z0-9-]+\.amazonaws\.com/`) + +// LocalImageRepo reports which repository of HomeCloud's own registry an image +// reference points at: an AWS-style ECR URI (".dkr.ecr..amazonaws.com/app:tag") +// or the registry's local address. Images from elsewhere return ok == false. +func LocalImageRepo(image string, ecrPort int) (repo string, ok bool) { + rest := "" + if loc := ecrHostedImage.FindStringIndex(image); loc != nil { + rest = image[loc[1]:] + } else { + for _, h := range []string{"localhost", "127.0.0.1"} { + if r, found := strings.CutPrefix(image, fmt.Sprintf("%s:%d/", h, ecrPort)); found { + rest = r + } + } + } + if rest == "" { + return "", false + } + if i := strings.IndexByte(rest, '@'); i >= 0 { + rest = rest[:i] + } + if i := strings.LastIndexByte(rest, ':'); i > strings.LastIndexByte(rest, '/') { + rest = rest[:i] + } + return rest, rest != "" +} + +// blockedNets are ranges outbound requests made on behalf of users never reach: +// "this network", the cloud metadata services of VPS providers that sit outside +// link-local space, and IPv6 prefixes that embed IPv4 addresses (NAT64, 6to4), +// which could smuggle a blocked IPv4 address past the checks. +var blockedNets = func() []*net.IPNet { + var out []*net.IPNet + for _, c := range []string{"0.0.0.0/8", "100.100.100.200/32", "192.0.0.192/32", "fd00:ec2::/32", "64:ff9b::/96", "64:ff9b:1::/48", "2002::/16", "::/128"} { + _, n, _ := net.ParseCIDR(c) + out = append(out, n) + } + return out +}() + +// DenyPrivateTargets, when set (HOMECLOUD_DENY_PRIVATE_TARGETS=1), also blocks +// RFC 1918 / unique-local addresses. They stay reachable by default because +// self-hosters legitimately call services on their LAN and in their VPCs. +var DenyPrivateTargets = os.Getenv("HOMECLOUD_DENY_PRIVATE_TARGETS") == "1" + +// blockedIP reports addresses outbound requests made for users may not reach: +// loopback, link-local (including the 169.254.169.254 metadata service), +// unspecified, multicast, the ranges in blockedNets and every address of this +// host itself (its public address and the Docker bridge gateways, through which +// workloads and users would otherwise reach the HomeCloud API and the Docker +// daemon). func blockedIP(ip net.IP) bool { - return ip.IsLoopback() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() || ip.IsUnspecified() || ip.IsMulticast() || ip.IsInterfaceLocalMulticast() + if v4 := ip.To4(); v4 != nil { + ip = v4 // IPv4-mapped IPv6 addresses are IPv4 addresses + } + if ip.IsLoopback() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() || ip.IsUnspecified() || ip.IsMulticast() || ip.IsInterfaceLocalMulticast() { + return true + } + for _, n := range blockedNets { + if n.Contains(ip) { + return true + } + } + if DenyPrivateTargets && ip.IsPrivate() { + return true + } + addrs, _ := net.InterfaceAddrs() + for _, a := range addrs { + if n, ok := a.(*net.IPNet); ok && n.IP.Equal(ip) { + return true + } + } + return false } -var errBlocked = errors.New("destination address is not allowed (loopback, link-local or unspecified)") +var errBlocked = errors.New("destination address is not allowed (loopback, link-local, metadata or this host)") + +// SafeClient is an HTTP client for requests to user-supplied URLs. It refuses +// to connect to blocked addresses at dial time (after DNS resolution, so +// rebinding and redirects cannot reach them), ignores proxy environment +// variables and follows at most maxRedirects redirects. +func SafeClient(timeout time.Duration, maxRedirects int) *http.Client { + c := WebhookClient(timeout) + c.CheckRedirect = func(req *http.Request, via []*http.Request) error { + if len(via) > maxRedirects { + if maxRedirects == 0 { + return http.ErrUseLastResponse + } + return errors.New("too many redirects") + } + return CheckWebhookURL(req.URL.String()) + } + return c +} // CheckWebhookURL validates an outbound webhook URL. func CheckWebhookURL(raw string) error { @@ -70,7 +160,7 @@ func WebhookClient(timeout time.Duration) *http.Client { }} tr := &http.Transport{DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) { return dialer.DialContext(ctx, network, addr) - }, TLSHandshakeTimeout: 10 * time.Second, MaxIdleConns: 10} + }, TLSHandshakeTimeout: 10 * time.Second, MaxIdleConns: 10, Proxy: nil} return &http.Client{Timeout: timeout, Transport: tr, CheckRedirect: func(req *http.Request, via []*http.Request) error { if len(via) >= 3 { return errors.New("too many redirects") diff --git a/cli/internal/core/targets_test.go b/cli/internal/core/targets_test.go new file mode 100644 index 00000000..99dec7dc --- /dev/null +++ b/cli/internal/core/targets_test.go @@ -0,0 +1,49 @@ +package core + +import ( + "net" + "testing" +) + +func TestLocalImageRepo(t *testing.T) { + for in, want := range map[string]string{ + "localhost:5500/app:1": "app", + "127.0.0.1:5500/team/app": "team/app", + "123456789012.dkr.ecr.us-east-1.amazonaws.com/app@sha256:ab": "app", + "123456789012.dkr.ecr.us-east-1.amazonaws.com/a/b:v2": "a/b", + "nginx:alpine": "", "localhost:5501/app": "", "docker.io/library/nginx": "", + } { + got, ok := LocalImageRepo(in, 5500) + if got != want || ok != (want != "") { + t.Errorf("%s: %q %v", in, got, ok) + } + } +} + +func TestBlockedIP(t *testing.T) { + for _, s := range []string{"127.0.0.1", "::1", "169.254.169.254", "::ffff:169.254.169.254", "::ffff:127.0.0.1", "0.0.0.0", "0.1.2.3", + "100.100.100.200", "192.0.0.192", "fd00:ec2::254", "64:ff9b::7f00:1", "2002:7f00:1::", "fe80::1", "224.0.0.1"} { + if !blockedIP(net.ParseIP(s)) { + t.Errorf("%s is reachable", s) + } + } + for _, s := range []string{"8.8.8.8", "93.184.216.34", "2606:4700:4700::1111", "100.64.0.1"} { + if blockedIP(net.ParseIP(s)) { + t.Errorf("%s is blocked", s) + } + } + // Every address of this host is off limits (Docker bridge gateways, public IP). + addrs, _ := net.InterfaceAddrs() + for _, a := range addrs { + if n, ok := a.(*net.IPNet); ok && !blockedIP(n.IP) { + t.Errorf("host address %s is reachable", n.IP) + } + } + DenyPrivateTargets = true + defer func() { DenyPrivateTargets = false }() + for _, s := range []string{"10.0.0.5", "172.17.0.1", "192.168.1.1", "fd12::1"} { + if !blockedIP(net.ParseIP(s)) { + t.Errorf("%s reachable with HOMECLOUD_DENY_PRIVATE_TARGETS", s) + } + } +} diff --git a/cli/internal/httpx/authz.go b/cli/internal/httpx/authz.go index a6bdc06e..76f10f4f 100644 --- a/cli/internal/httpx/authz.go +++ b/cli/internal/httpx/authz.go @@ -6,6 +6,8 @@ import ( "strconv" "strings" "sync" + + "github.com/homecloudhq/homecloud/cli/internal/core" ) // Decision is the outcome of evaluating identity policies. @@ -132,6 +134,10 @@ func (p *Principal) Permits(action, resource string, extra Access) bool { if p == nil { return PermitsAnonymous(action, resource, extra) } + resource = core.CanonicalARN(resource) + if p.ResolveResource != nil { + resource = p.ResolveResource(action, resource) + } acc := providerAccess(resource) if extra.Policy != "" { acc.Policy, acc.KeyPolicy = extra.Policy, extra.KeyPolicy diff --git a/cli/internal/httpx/httpx.go b/cli/internal/httpx/httpx.go index 179b8bc4..337139e0 100644 --- a/cli/internal/httpx/httpx.go +++ b/cli/internal/httpx/httpx.go @@ -45,6 +45,10 @@ type Principal struct { // as "aws:sourceip" or "aws:username"). IAM fills the identity keys; see // AddRequestContext for the request keys. Context map[string][]string `json:"-"` + // ResolveResource maps the resource a caller named to the one it denotes (IAM + // resolves role names and paths for iam:PassRole), so that a policy on the real + // resource cannot be dodged by another spelling of it. + ResolveResource func(action, resource string) string `json:"-"` } // AddRequestContext records the IAM global condition keys that come from the @@ -91,8 +95,17 @@ type routeOpts struct { resource string public bool deferred bool + maxBody int64 } +// PublicBodyLimit is the request body cap of routes marked SmallBody: sign-in +// and similar calls whose bodies are a few hundred bytes. +const PublicBodyLimit = 64 << 10 + +// SmallBody caps the request body at PublicBodyLimit; use it on public routes, +// which are reachable without credentials. +func SmallBody() Opt { return func(o *routeOpts) { o.maxBody = PublicBodyLimit } } + type Opt func(*routeOpts) // Res sets the resource ARN template checked against the route's action; @@ -124,6 +137,9 @@ func (rt *Router) Handle(pattern, action string, h Handler, opts ...Opt) { c := &Ctx{W: w, R: r, Account: rt.Account} sw := &statusWriter{ResponseWriter: w, status: 200} c.W = sw + if o.maxBody > 0 { + r.Body = http.MaxBytesReader(sw, r.Body, o.maxBody) + } resource := strings.ReplaceAll(strings.ReplaceAll(o.resource, "{account}", rt.Account), "{region}", core.Region) resource = placeholder.ReplaceAllStringFunc(resource, func(m string) string { return r.PathValue(m[1 : len(m)-1]) @@ -258,7 +274,7 @@ func WriteError(w http.ResponseWriter, err error) { ce = core.Errf(http.StatusNotFound, "ResourceNotFound", "resource not found") default: log.Printf("internal error: %v", err) - ce = core.Errf(http.StatusInternalServerError, "InternalError", "%v", err) + ce = core.Errf(http.StatusInternalServerError, "InternalError", "%s", core.InternalErrorMessage) } WriteJSON(w, ce.Status, map[string]any{"error": ce}) } diff --git a/cli/internal/server/sandbox_test.go b/cli/internal/server/sandbox_test.go new file mode 100644 index 00000000..b7f61314 --- /dev/null +++ b/cli/internal/server/sandbox_test.go @@ -0,0 +1,48 @@ +package server + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// The console opens an object inline with the viewer's session token in the URL +// (?access_token=), so a script inside the object could read the token from its +// own location. Inline views must therefore run no scripts. +// A function (or an HTTP integration's upstream) picks its own response headers; +// it must not be able to replace the sandbox and run script on the console origin. +func TestUserContentCannotReplaceTheSandbox(t *testing.T) { + h := sandboxUserContent(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Security-Policy", "default-src *") + w.Header().Set("X-Content-Type-Options", "") + w.Write([]byte("")) + })) + for _, p := range []string{"/lambda-url/f/", "/apigw/x/y", "/website/b/index.html"} { + w := httptest.NewRecorder() + h.ServeHTTP(w, httptest.NewRequest(http.MethodGet, p, nil)) + if csp := w.Header().Get("Content-Security-Policy"); !strings.HasPrefix(csp, "sandbox ") || w.Header().Get("X-Content-Type-Options") != "nosniff" { + t.Fatalf("%s: response replaced the sandbox: %q", p, csp) + } + } +} + +func TestInlineObjectViewsRunNoScripts(t *testing.T) { + h := sandboxUserContent(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + r := httptest.NewRequest(http.MethodGet, "/api/v1/s3/b/object?key=x.html&inline=true&access_token=hcs_secret", nil) + w := httptest.NewRecorder() + h.ServeHTTP(w, r) + csp := w.Header().Get("Content-Security-Policy") + if !strings.HasPrefix(csp, "sandbox;") || strings.Contains(csp, "allow-scripts") || !strings.Contains(csp, "default-src 'none'") { + t.Fatalf("CSP %q lets an object run scripts or load resources", csp) + } + if w.Header().Get("Referrer-Policy") != "no-referrer" { + t.Fatal("the token-bearing URL would be sent as a Referer") + } + // Hosted websites keep their scripts (they are sandboxed to an opaque origin and carry no token). + w = httptest.NewRecorder() + h.ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/website/b/index.html", nil)) + if !strings.Contains(w.Header().Get("Content-Security-Policy"), "allow-scripts") { + t.Fatal("website sandbox lost allow-scripts") + } +} diff --git a/cli/internal/server/server.go b/cli/internal/server/server.go index 4f272534..a695a97f 100644 --- a/cli/internal/server/server.go +++ b/cli/internal/server/server.go @@ -264,7 +264,7 @@ func Run(ctx context.Context, cfg core.Config, opts Options) error { } lambdaSvc.VerifyJWT = cognitoSvc.VerifyToken lambdaSvc.CheckAuthorizer = cognitoSvc.CheckClient - tg := &targets{lambda: lambdaSvc, sqs: sqsSvc, sns: snsSvc, sfn: sfnSvc} + tg := &targets{lambda: lambdaSvc, sqs: sqsSvc, sns: snsSvc, sfn: sfnSvc, account: account} sfnSvc.Tasks = tg sfnSvc.Call = func(ctx context.Context, p *httpx.Principal, service, op string, in any) (json.RawMessage, error) { return awsapi.Call(ctx, p, account, service, op, in) @@ -400,7 +400,7 @@ func Run(ctx context.Context, cfg core.Config, opts Options) error { return err } var handler http.Handler = trust.Wrap(root) - srv := &http.Server{Addr: cfg.APIAddr, Handler: handler, ReadHeaderTimeout: 10 * time.Second} + srv := &http.Server{Addr: cfg.APIAddr, Handler: handler, ReadHeaderTimeout: 10 * time.Second, IdleTimeout: 2 * time.Minute, MaxHeaderBytes: 256 << 10} if workloadLn != nil { // Workloads are never proxies: their listeners ignore forwarding headers. defer serveWorkloads(workloadLn, root, logf)() @@ -419,7 +419,7 @@ func Run(ctx context.Context, cfg core.Config, opts Options) error { if goruntime.GOOS == "linux" { if host, _, _ := net.SplitHostPort(cfg.APIAddr); host == "127.0.0.1" || host == "localhost" { if gw := dk.BridgeGateway(); gw != "" { - extra := &http.Server{Addr: net.JoinHostPort(gw, apiPort), Handler: root, ReadHeaderTimeout: 10 * time.Second} + extra := &http.Server{Addr: net.JoinHostPort(gw, apiPort), Handler: root, ReadHeaderTimeout: 10 * time.Second, IdleTimeout: 2 * time.Minute, MaxHeaderBytes: 256 << 10} go func() { var err error if cfg.TLSCert != "" { @@ -453,6 +453,11 @@ func Run(ctx context.Context, cfg core.Config, opts Options) error { func vpcList(st *store.Store) []vpc.VPC { return store.List[vpc.VPC](st, "vpc_vpcs") } +// inlineObjectCSP is the policy of object bytes shown from the native API: an +// opaque origin, no scripts, forms or navigation, and only inline styles and +// data:/same-origin media. +const inlineObjectCSP = "sandbox; default-src 'none'; style-src 'unsafe-inline'; img-src data: 'self'; media-src data: 'self'; font-src data:; form-action 'none'; base-uri 'none'" + // sandboxUserContent isolates responses whose bytes come from users (static // websites, function URLs, HTTP APIs and inline object views). They share an // origin with the console, so without a sandbox a page could read the console's @@ -460,18 +465,54 @@ func vpcList(st *store.Store) []vpc.VPC { return store.List[vpc.VPC](st, "vpc_vp func sandboxUserContent(h http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { p := r.URL.Path - if strings.HasPrefix(p, "/website/") || strings.HasPrefix(p, "/lambda-url/") || strings.HasPrefix(p, "/apigw/") || - (strings.HasPrefix(p, "/api/v1/s3/") && strings.HasSuffix(p, "/object")) { + if strings.HasPrefix(p, "/api/v1/s3/") && strings.HasSuffix(p, "/object") { + // The console opens objects with ?access_token= in the URL, which + // a script in the object could read from its own location and send away. + // So inline views run no scripts, load nothing from elsewhere and send no Referer. + w.Header().Set("Content-Security-Policy", inlineObjectCSP) + w.Header().Set("X-Content-Type-Options", "nosniff") + } else if strings.HasPrefix(p, "/website/") || strings.HasPrefix(p, "/lambda-url/") || strings.HasPrefix(p, "/apigw/") { w.Header().Set("Content-Security-Policy", "sandbox allow-scripts allow-forms allow-popups allow-modals allow-downloads") w.Header().Set("X-Content-Type-Options", "nosniff") } if strings.HasPrefix(p, "/api/") { w.Header().Set("X-Content-Type-Options", "nosniff") + w.Header().Set("Referrer-Policy", "no-referrer") + } + if csp := w.Header().Get("Content-Security-Policy"); csp != "" { + // Functions and HTTP integrations choose their own response headers: pin + // the sandbox so they cannot replace it and run on the console's origin. + w = &pinnedHeaders{ResponseWriter: w, pins: map[string]string{"Content-Security-Policy": csp, "X-Content-Type-Options": "nosniff"}} } h.ServeHTTP(w, r) }) } +// pinnedHeaders re-applies fixed headers just before the response is written, +// whatever the handler set in between. +type pinnedHeaders struct { + http.ResponseWriter + pins map[string]string +} + +func (p *pinnedHeaders) pin() { + for k, v := range p.pins { + p.Header().Set(k, v) + } +} +func (p *pinnedHeaders) WriteHeader(code int) { p.pin(); p.ResponseWriter.WriteHeader(code) } +func (p *pinnedHeaders) Write(b []byte) (int, error) { + p.pin() + return p.ResponseWriter.Write(b) +} +func (p *pinnedHeaders) Flush() { + p.pin() + if f, ok := p.ResponseWriter.(http.Flusher); ok { + f.Flush() + } +} +func (p *pinnedHeaders) Unwrap() http.ResponseWriter { return p.ResponseWriter } + // withCORS allows the console dev server and other origins to call the API. // Credentials travel in the Authorization header, never cookies, so a wildcard is safe. func withCORS(h http.Handler) http.Handler { diff --git a/cli/internal/server/targets.go b/cli/internal/server/targets.go index d61c33cf..f47d518b 100644 --- a/cli/internal/server/targets.go +++ b/cli/internal/server/targets.go @@ -21,6 +21,8 @@ type targets struct { sqs *sqs.Service sns *sns.Service sfn *sfn.Service + // account, when set, restricts deliveries to this account's own ARNs. + account string } // Invoke, SendMessage and Publish implement sfn.Tasks. @@ -64,7 +66,10 @@ func (t *targets) Publish(topic, subject, message string) (map[string]any, error } // parse splits "arn:aws::::". -func parse(arn string) (service, resource string, ok bool) { +func (t *targets) parse(arn string) (service, resource string, ok bool) { + if t.account != "" && !core.IsLocalARN(arn, t.account) { + return "", "", false // another account or region: not a resource this server can deliver to + } parts := strings.SplitN(core.CanonicalARN(arn), ":", 6) if len(parts) != 6 || parts[0] != "arn" || parts[1] != core.Partition { return "", "", false @@ -73,7 +78,7 @@ func parse(arn string) (service, resource string, ok bool) { } func (t *targets) exists(arn string) bool { - svc, res, ok := parse(arn) + svc, res, ok := t.parse(arn) if !ok { return false } @@ -98,7 +103,7 @@ func denied(src core.Source, target string) error { } func (t *targets) deliver(ctx context.Context, arn string, payload []byte) error { - svc, res, ok := parse(arn) + svc, res, ok := t.parse(arn) if !ok { return fmt.Errorf("malformed ARN %q", arn) } diff --git a/cli/internal/server/targets_arn_test.go b/cli/internal/server/targets_arn_test.go new file mode 100644 index 00000000..7ae8e379 --- /dev/null +++ b/cli/internal/server/targets_arn_test.go @@ -0,0 +1,32 @@ +package server + +import ( + "testing" + + "github.com/homecloudhq/homecloud/cli/internal/core" +) + +// A target ARN of another account or region must not be treated as a local +// resource of the same name: IAM would authorize it as the foreign ARN while the +// delivery went to the local queue. +func TestTargetsRefuseForeignARNs(t *testing.T) { + tg := &targets{account: "111122223333"} + if _, _, ok := tg.parse(core.ARN("111122223333", "sqs", "q")); !ok { + t.Fatal("a local ARN was refused") + } + if _, _, ok := tg.parse("arn:hc:sqs:local-1:111122223333:q"); !ok { + t.Fatal("a legacy-form local ARN was refused") + } + for _, arn := range []string{ + "arn:aws:sqs:" + core.Region + ":999999999999:q", + "arn:aws:sqs:eu-north-9:111122223333:q", + "arn:aws:sqs::111122223333:q", + } { + if _, _, ok := tg.parse(arn); ok { + t.Errorf("%s was accepted", arn) + } + if tg.exists(arn) { + t.Errorf("%s exists", arn) + } + } +} diff --git a/cli/internal/server/workloads.go b/cli/internal/server/workloads.go index c4bf7f77..e2fb8b74 100644 --- a/cli/internal/server/workloads.go +++ b/cli/internal/server/workloads.go @@ -32,7 +32,7 @@ func workloadListenAddr(goos, bridgeGateway string) (addr string, ok bool) { // serveWorkloads serves h on l until the server is closed; the returned // function stops it. func serveWorkloads(l net.Listener, h http.Handler, logf func(string, ...any)) (stop func()) { - srv := &http.Server{Handler: h, ReadHeaderTimeout: 10 * time.Second} + srv := &http.Server{Handler: h, ReadHeaderTimeout: 10 * time.Second, IdleTimeout: 2 * time.Minute, MaxHeaderBytes: 256 << 10} go func() { if err := srv.Serve(l); err != nil && !errors.Is(err, http.ErrServerClosed) { logf("workload endpoint %s: %v", l.Addr(), err) diff --git a/cli/internal/svc/autoscaling/autoscaling.go b/cli/internal/svc/autoscaling/autoscaling.go index 142ae114..a8956d61 100644 --- a/cli/internal/svc/autoscaling/autoscaling.go +++ b/cli/internal/svc/autoscaling/autoscaling.go @@ -35,6 +35,9 @@ type LaunchConfig struct { SecurityGroupIDs []string `json:"security_group_ids"` UserData string `json:"user_data,omitempty"` FileSystems []ec2.FSMount `json:"file_systems,omitempty"` + // IAMInstanceProfile is the launch template's instance profile; creating or + // updating a group needs iam:PassRole for its role. + IAMInstanceProfile string `json:"iam_instance_profile,omitempty"` } type Policy struct { @@ -428,6 +431,9 @@ func (s *Service) authorizeLaunch(c authz, l LaunchConfig, targetGroups []string if err := c.Authorize("ec2:RunInstances", "*"); err != nil { return err } + if err := s.ec2.PassProfile(c.Authorize, l.IAMInstanceProfile); err != nil { + return err + } for _, m := range l.FileSystems { if err := c.Authorize("elasticfilesystem:ClientMount", s.env.ARN("elasticfilesystem", "file-system/"+m.FileSystemID)); err != nil { return err @@ -486,6 +492,7 @@ func (s *Service) resolveTemplate(g *Group) error { } g.Template.ID, g.Template.Name = rt.ID, rt.Name g.Launch.ImageID, g.Launch.InstanceType, g.Launch.SecurityGroupIDs, g.Launch.UserData = rt.Input.ImageID, rt.Input.InstanceType, rt.Input.SecurityGroupIDs, rt.Input.UserData + g.Launch.IAMInstanceProfile = rt.Input.IAMInstanceProfile return nil } diff --git a/cli/internal/svc/autoscaling/passrole_test.go b/cli/internal/svc/autoscaling/passrole_test.go new file mode 100644 index 00000000..74663a62 --- /dev/null +++ b/cli/internal/svc/autoscaling/passrole_test.go @@ -0,0 +1,72 @@ +package autoscaling_test + +import ( + "strings" + "testing" + "time" + + "github.com/homecloudhq/homecloud/cli/internal/awsapi/awstest" + "github.com/homecloudhq/homecloud/cli/internal/svc/autoscaling" + "github.com/homecloudhq/homecloud/cli/internal/svc/cloudwatch" + "github.com/homecloudhq/homecloud/cli/internal/svc/ec2" + "github.com/homecloudhq/homecloud/cli/internal/svc/vpc" +) + +// profiles maps instance profile "-prof" to role "". +type profiles struct{ acct string } + +func (p profiles) InstanceProfile(ref string) (arn, id, role string, err error) { + name := strings.TrimSuffix(ref[strings.LastIndex(ref, "/")+1:], "-prof") + return "arn:aws:iam::" + p.acct + ":instance-profile/" + name + "-prof", "AIPA" + name, "arn:aws:iam::" + p.acct + ":role/" + name, nil +} +func (profiles) InstanceCredentials(string, string, time.Duration) (ec2.Credentials, error) { + return ec2.Credentials{}, nil +} + +// An instance profile's role reaches whoever controls the instance, so putting +// one in a launch template, launching with it or creating an Auto Scaling group +// that launches from such a template all need iam:PassRole for the role. +func TestInstanceProfileNeedsPassRole(t *testing.T) { + h := awstest.New(t) + v := vpc.New(h.Env) + e := ec2.New(h.Env, v) + e.Roles = profiles{h.Env.AccountID} + e.RegisterAWS() + e.Routes(h.Router) + cw, err := cloudwatch.New(h.Env) + if err != nil { + t.Fatal(err) + } + a := autoscaling.New(h.Env, e, nil, cw) + a.RegisterAWS() + a.Routes(h.Router) + + h.Native(t, "POST", "/api/v1/iam/policies", map[string]any{"name": "dev", "document": map[string]any{"Version": "2012-10-17", + "Statement": []any{ + map[string]any{"Effect": "Allow", "Action": []string{"ec2:*", "autoscaling:*"}, "Resource": "*"}, + map[string]any{"Effect": "Allow", "Action": "iam:PassRole", "Resource": "arn:aws:iam::" + h.Env.AccountID + ":role/dev-*"}, + }}}) + akid, secret := h.User(t, "dev", "dev") + + // Native launch. + code, body := h.NativeAs(t, akid, secret, "POST", "/api/v1/ec2/instances", map[string]any{"image_id": "ami-x", "iam_instance_profile": "admin-prof"}) + if code != 403 || !strings.Contains(string(body), "iam:PassRole") { + t.Fatalf("native launch with another role's profile: %d %s", code, body) + } + + // Launch templates. + if out, err := h.AWSAs(t, akid, secret, "", "ec2", "create-launch-template", "--launch-template-name", "evil", + "--launch-template-data", `{"IamInstanceProfile":{"Name":"admin-prof"}}`); err == nil || !strings.Contains(out, "iam:PassRole") { + t.Fatalf("template with another role's profile: %v %s", err, out) + } + h.AWSAs(t, akid, secret, "", "ec2", "create-launch-template", "--launch-template-name", "fine", "--launch-template-data", `{"IamInstanceProfile":{"Name":"dev-app-prof"}}`) + + // A template an administrator made with a powerful profile can't be used by + // the developer to start a group. + h.AWS(t, "ec2", "create-launch-template", "--launch-template-name", "admin-lt", "--launch-template-data", `{"IamInstanceProfile":{"Name":"admin-prof"},"InstanceType":"t3.micro"}`) + code, body = h.NativeAs(t, akid, secret, "POST", "/api/v1/autoscaling/groups", + map[string]any{"name": "g", "template": map[string]any{"name": "admin-lt", "version": "$Latest"}, "min_size": 0, "max_size": 1}) + if code != 403 || !strings.Contains(string(body), "iam:PassRole") { + t.Fatalf("group from a template with another role's profile: %d %s", code, body) + } +} diff --git a/cli/internal/svc/cognito/cognito.go b/cli/internal/svc/cognito/cognito.go index 33aacec6..8fd41b39 100644 --- a/cli/internal/svc/cognito/cognito.go +++ b/cli/internal/svc/cognito/cognito.go @@ -952,12 +952,12 @@ func (s *Service) removeGroup(pool, g string) (Pool, error) { // ---- public (application-facing) routes ---- func (s *Service) publicRoutes(r *httpx.Router) { - r.Handle("POST /cognito/{pool}/sign-up", "", s.signUp, httpx.Public()) - r.Handle("POST /cognito/{pool}/auth", "", s.auth, httpx.Public()) - r.Handle("POST /cognito/{pool}/respond", "", s.respond, httpx.Public()) - r.Handle("POST /cognito/{pool}/change-password", "", s.changePassword, httpx.Public()) + r.Handle("POST /cognito/{pool}/sign-up", "", s.signUp, httpx.Public(), httpx.SmallBody()) + r.Handle("POST /cognito/{pool}/auth", "", s.auth, httpx.Public(), httpx.SmallBody()) + r.Handle("POST /cognito/{pool}/respond", "", s.respond, httpx.Public(), httpx.SmallBody()) + r.Handle("POST /cognito/{pool}/change-password", "", s.changePassword, httpx.Public(), httpx.SmallBody()) r.Handle("GET /cognito/{pool}/userinfo", "", s.userinfo, httpx.Public()) - r.Handle("POST /cognito/{pool}/sign-out", "", s.signOut, httpx.Public()) + r.Handle("POST /cognito/{pool}/sign-out", "", s.signOut, httpx.Public(), httpx.SmallBody()) r.Handle("GET /cognito/{pool}/.well-known/jwks.json", "", s.jwks, httpx.Public()) r.Handle("GET /cognito/{pool}/.well-known/openid-configuration", "", s.discovery, httpx.Public()) } @@ -976,7 +976,17 @@ func (s *Service) client(p Pool, id, secret string) (Client, error) { // attempt records a sign-in attempt for key and reports whether it is allowed. // Attempts are counted before the password check so parallel guesses cannot // slip past the limit; a success clears the record. -func (s *Service) attempt(key string) bool { +func (s *Service) attempt(key string) bool { return s.attemptLimit(key, 10) } + +const ( + // maxAccountAttempts bounds password guesses per user across all addresses in + // the window; maxSignUps bounds self sign-ups per pool (each costs a bcrypt hash). + maxAccountAttempts = 40 + maxSignUps = 120 +) + +// attemptLimit is attempt with a limit of max attempts per window. +func (s *Service) attemptLimit(key string, max int) bool { s.mu.Lock() defer s.mu.Unlock() recent := s.fails[key][:0] @@ -985,7 +995,7 @@ func (s *Service) attempt(key string) bool { recent = append(recent, t) } } - if len(recent) >= 10 { + if len(recent) >= max { s.fails[key] = recent return false } @@ -1045,6 +1055,9 @@ func (s *Service) selfSignUp(poolID string, in authInput) (User, error) { if _, err := s.client(p, in.ClientID, in.ClientSecret); err != nil { return User{}, err } + if !s.attemptLimit("signup/"+p.ID, maxSignUps) { + return User{}, core.Errf(http.StatusTooManyRequests, "TooManyRequestsException", "too many sign-ups; try again later") + } status := "UNCONFIRMED" if p.AutoConfirm { status = "CONFIRMED" @@ -1130,7 +1143,9 @@ func (s *Service) authenticate(poolID string, in authInput, ip string) (any, err switch in.Flow { case "", "USER_PASSWORD_AUTH": tk := p.ID + "/" + strings.ToLower(in.Username) + "/" + ip - if !s.attempt(tk) { + // The per-address limit alone is bypassed by rotating addresses: also bound + // the guesses an account can receive from everywhere together. + if !s.attemptLimit(p.ID+"/acct/"+strings.ToLower(in.Username), maxAccountAttempts) || !s.attempt(tk) { return nil, core.Errf(http.StatusTooManyRequests, "TooManyRequestsException", "too many failed attempts; try again later") } u, err := store.Get[User](s.env.Store, cUsers, userKey(p.ID, in.Username)) diff --git a/cli/internal/svc/cognito/signup_limit_test.go b/cli/internal/svc/cognito/signup_limit_test.go new file mode 100644 index 00000000..969b1ac9 --- /dev/null +++ b/cli/internal/svc/cognito/signup_limit_test.go @@ -0,0 +1,27 @@ +package cognito_test + +import ( + "encoding/json" + "fmt" + "testing" +) + +// Self sign-up is open to anyone and costs a bcrypt hash and a store write each: +// a pool must stop accepting a flood of them. +func TestSelfSignUpIsRateLimited(t *testing.T) { + h, _ := newCognito(t) + var pool, cl map[string]any + _ = json.Unmarshal(h.Native(t, "POST", "/api/v1/cognito/user-pools", map[string]any{"name": "open"}), &pool) + pid := pool["id"].(string) + _ = json.Unmarshal(h.Native(t, "POST", "/api/v1/cognito/user-pools/"+pid+"/clients", map[string]any{"name": "c"}), &cl) + limited := 0 + for i := 0; i < 130; i++ { + st, _ := post(t, h.URL+"/cognito/"+pid+"/sign-up", map[string]any{"client_id": cl["id"], "username": fmt.Sprintf("u%d", i), "password": "pass1234"}, "") + if st == 429 { + limited++ + } + } + if limited < 5 { + t.Fatalf("only %d of 130 sign-ups were refused", limited) + } +} diff --git a/cli/internal/svc/cognito/srp.go b/cli/internal/svc/cognito/srp.go index ce26e99e..3ce43d95 100644 --- a/cli/internal/svc/cognito/srp.go +++ b/cli/internal/svc/cognito/srp.go @@ -167,7 +167,7 @@ func (s *Service) srpInitiate(p Pool, cl Client, username, srpA, ip string) (map return nil, invalid("Invalid SRP_A") } rk := p.ID + "/srp/" + strings.ToLower(username) + "/" + ip - if !s.attempt(rk) { + if !s.attemptLimit(p.ID+"/acct/"+strings.ToLower(username), maxAccountAttempts) || !s.attempt(rk) { return nil, core.Errf(http.StatusTooManyRequests, "TooManyRequestsException", "too many failed attempts; try again later") } st := &srpPending{pool: p.ID, client: cl.ID, username: username, rateKey: rk, a: a, b: randInt(512), expires: time.Now().Add(srpChallengeTTL)} diff --git a/cli/internal/svc/ec2/ec2.go b/cli/internal/svc/ec2/ec2.go index 5bcb8955..c8433b83 100644 --- a/cli/internal/svc/ec2/ec2.go +++ b/cli/internal/svc/ec2/ec2.go @@ -391,6 +391,26 @@ func checkTags(t core.Tags) error { return nil } +// PassProfile checks, through az, that the caller may pass the role of the +// instance profile ref (a name or ARN) to instances: launching with a profile +// hands the role's credentials to whoever controls the instance. +func (s *Service) PassProfile(az func(action, resource string) error, ref string) error { + if ref == "" { + return nil + } + if s.Roles == nil { + return core.BadRequest("instance profiles are not available") + } + _, _, role, err := s.Roles.InstanceProfile(ref) + if err != nil { + return err + } + if role == "" { + return nil + } + return az("iam:PassRole", role) +} + func (s *Service) run(c *httpx.Ctx) (any, error) { var in RunInput if err := c.Bind(&in); err != nil { @@ -399,6 +419,9 @@ func (s *Service) run(c *httpx.Ctx) (any, error) { if err := checkTags(in.Tags); err != nil { return nil, err } + if err := s.PassProfile(c.Authorize, in.IAMInstanceProfile); err != nil { + return nil, err + } for _, v := range in.Volumes { if v.VolumeID != "" { if err := c.Authorize("ec2:AttachVolume", s.env.ARN("ec2", "volume/"+v.VolumeID)); err != nil { diff --git a/cli/internal/svc/ec2/launchtemplates.go b/cli/internal/svc/ec2/launchtemplates.go index 5da53519..28ca8376 100644 --- a/cli/internal/svc/ec2/launchtemplates.go +++ b/cli/internal/svc/ec2/launchtemplates.go @@ -272,6 +272,14 @@ func (s *Service) ltData(q *awsapi.Req) (map[string]string, error) { return nil, core.Errf(http.StatusBadRequest, "InvalidGroup.NotFound", "The security group '%s' does not exist", g) } } + // As in AWS, putting an instance profile in a template needs iam:PassRole. + ref := d["IamInstanceProfile.Arn"] + if ref == "" { + ref = d["IamInstanceProfile.Name"] + } + if err := s.PassProfile(q.Check, ref); err != nil { + return nil, err + } return d, nil } diff --git a/cli/internal/svc/ecs/aws.go b/cli/internal/svc/ecs/aws.go index 1902dfad..b48b08df 100644 --- a/cli/internal/svc/ecs/aws.go +++ b/cli/internal/svc/ecs/aws.go @@ -500,9 +500,8 @@ func (e *ECS) awsRegisterTaskDefinition(q *awsapi.Req) (any, error) { } } if role := str(raw, "taskRoleArn"); role != "" { - if err := q.Authorize("iam:PassRole", role); err != nil { - return nil, err - } + // PassRole is checked against the resolved role ARN by registerTaskDef: a + // spelling such as role/x-/admin must not dodge a policy on role/admin. if e.Roles != nil { arn, err := e.Roles.TaskRole(role) if err != nil { @@ -513,9 +512,6 @@ func (e *ECS) awsRegisterTaskDefinition(q *awsapi.Req) (any, error) { td.TaskRole = role } if role := str(raw, "executionRoleArn"); role != "" { - if err := q.Authorize("iam:PassRole", role); err != nil { - return nil, err - } td.ExecutionRole = role } td, err := e.registerTaskDef(q.Authorize, td) diff --git a/cli/internal/svc/ecs/ecs.go b/cli/internal/svc/ecs/ecs.go index eda69e77..3f034a21 100644 --- a/cli/internal/svc/ecs/ecs.go +++ b/cli/internal/svc/ecs/ecs.go @@ -704,6 +704,41 @@ func (e *ECS) registerTaskDef(az authz, in TaskDefinition) (TaskDefinition, erro if err := e.authorizeSecrets(az, in); err != nil { return in, err } + // Passing a role to tasks needs iam:PassRole on the role's real ARN, on every + // path that registers task definitions (native API, AWS API, CloudFormation): + // running the task hands the role's credentials to the task's code. + if in.TaskRole != "" { + arn := in.TaskRole + if e.Roles != nil { + var err error + if arn, err = e.Roles.TaskRole(in.TaskRole); err != nil { + return in, core.BadRequest("task role: %v", err) + } + } + if err := az("iam:PassRole", arn); err != nil { + return in, err + } + in.TaskRole = arn + } + if in.ExecutionRole != "" { + arn := core.CanonicalARN(in.ExecutionRole) + if !strings.HasPrefix(arn, "arn:") { + arn = e.env.ARN("iam", "role/"+in.ExecutionRole) + } + if err := az("iam:PassRole", arn); err != nil { + return in, err + } + } + // Running the image discloses its contents to the task: pulling from the local + // registry needs the same permissions as pulling from ECR. + if repo, ok := core.LocalImageRepo(in.Image, e.env.Cfg.ECRPort); ok { + arn := e.env.ARN("ecr", "repository/"+repo) + for _, action := range []string{"ecr:BatchGetImage", "ecr:GetDownloadUrlForLayer"} { + if err := az(action, arn); err != nil { + return in, err + } + } + } e.tdMu.Lock() defer e.tdMu.Unlock() rev := 1 diff --git a/cli/internal/svc/ecs/passrole_test.go b/cli/internal/svc/ecs/passrole_test.go new file mode 100644 index 00000000..212ddd5c --- /dev/null +++ b/cli/internal/svc/ecs/passrole_test.go @@ -0,0 +1,65 @@ +package ecs_test + +import ( + "strings" + "testing" + "time" + + "github.com/homecloudhq/homecloud/cli/internal/awsapi/awstest" + "github.com/homecloudhq/homecloud/cli/internal/svc/ecs" +) + +// stubRoles resolves every spelling of a role to its real ARN, as IAM does +// (role/dev-/admin and "admin" both name the role "admin"). +type stubRoles struct{ acct string } + +func (r stubRoles) TaskRole(ref string) (string, error) { + name := ref[strings.LastIndex(ref, "/")+1:] + return "arn:aws:iam::" + r.acct + ":role/" + name, nil +} +func (stubRoles) TaskCredentials(string, string, time.Duration) (ecs.Credentials, error) { + return ecs.Credentials{}, nil +} + +// Handing a task role to a task hands its credentials to the task's code, so +// every way of registering a task definition needs iam:PassRole on the role, +// judged on the role's real ARN. +func TestTaskRoleNeedsPassRole(t *testing.T) { + h := awstest.New(t) + e := ecs.New(h.Env, nil, nil, h.Secrets) + e.Roles = stubRoles{h.Env.AccountID} + e.Routes(h.Router) + e.RegisterAWS() + h.Native(t, "POST", "/api/v1/iam/policies", map[string]any{"name": "ecs-dev", "document": map[string]any{"Version": "2012-10-17", + "Statement": []any{ + map[string]any{"Effect": "Allow", "Action": "ecs:*", "Resource": "*"}, + map[string]any{"Effect": "Allow", "Action": "iam:PassRole", "Resource": "arn:aws:iam::" + h.Env.AccountID + ":role/dev-*"}, + }}}) + akid, secret := h.User(t, "dev", "ecs-dev") + + td := func(role string) map[string]any { + return map[string]any{"family": "f", "image": "alpine", "task_role": role} + } + for _, role := range []string{"admin", "arn:aws:iam::" + h.Env.AccountID + ":role/admin", "arn:aws:iam::" + h.Env.AccountID + ":role/dev-/admin"} { + if code, body := h.NativeAs(t, akid, secret, "POST", "/api/v1/ecs/task-definitions", td(role)); code != 403 { + t.Fatalf("native register with task role %q: %d %s", role, code, body) + } + in := `{"family":"g","taskRoleArn":"` + role + `","containerDefinitions":[{"name":"a","image":"alpine"}]}` + if out, err := h.AWSAs(t, akid, secret, "", "ecs", "register-task-definition", "--cli-input-json", in); err == nil || !strings.Contains(out, "iam:PassRole") { + t.Fatalf("AWS register with task role %q: %v %s", role, err, out) + } + } + // Images of the local registry need the ECR pull permissions. + for _, image := range []string{"localhost:5500/secret:1", "123456789012.dkr.ecr.us-east-1.amazonaws.com/secret@sha256:abc"} { + h.Env.Cfg.ECRPort = 5500 + if code, body := h.NativeAs(t, akid, secret, "POST", "/api/v1/ecs/task-definitions", map[string]any{"family": "img", "image": image}); code != 403 || !strings.Contains(string(body), "ecr:BatchGetImage") { + t.Fatalf("image %s: %d %s", image, code, body) + } + } + if code, body := h.NativeAs(t, akid, secret, "POST", "/api/v1/ecs/task-definitions", map[string]any{"family": "img", "image": "nginx:alpine"}); code != 200 { + t.Fatalf("a public image was refused: %d %s", code, body) + } + if code, body := h.NativeAs(t, akid, secret, "POST", "/api/v1/ecs/task-definitions", td("dev-app")); code != 200 { + t.Fatalf("a role the caller may pass was refused: %d %s", code, body) + } +} diff --git a/cli/internal/svc/iam/iam.go b/cli/internal/svc/iam/iam.go index f97db255..e656395a 100644 --- a/cli/internal/svc/iam/iam.go +++ b/cli/internal/svc/iam/iam.go @@ -189,6 +189,33 @@ func (s *Service) throttled(ip string, fail bool) bool { return len(recent) >= maxFailures } +// releaseAttempt gives back the throttle slot of a sign-in that succeeded. +func (s *Service) releaseAttempt(ip string) { + s.failMu.Lock() + defer s.failMu.Unlock() + if l := s.failures[ip]; len(l) > 1 { + s.failures[ip] = l[:len(l)-1] + } else { + delete(s.failures, ip) + } +} + +var ( + dummyOnce sync.Once + dummyBcr string +) + +// dummyHash is a valid bcrypt hash nobody knows the password of, compared +// against when a sign-in names an unknown user so the response time doesn't +// reveal which user names exist. +func dummyHash() string { + dummyOnce.Do(func() { + h, _ := bcrypt.GenerateFromPassword([]byte(core.NewSecret(24)), bcrypt.DefaultCost) + dummyBcr = string(h) + }) + return dummyBcr +} + // BootstrapResult carries credentials that exist only at first start. type BootstrapResult struct { AccountID string @@ -304,7 +331,12 @@ func (s *Service) Authenticate(r *http.Request) (*httpx.Principal, error) { if h := r.Header.Get("Authorization"); strings.HasPrefix(h, "Bearer ") { token = strings.TrimSpace(strings.TrimPrefix(h, "Bearer ")) } else if r.Method == http.MethodGet { - token = r.URL.Query().Get("access_token") // lets the console link downloads directly + // Lets the console link downloads directly. URLs end up in logs and history, + // so only a (revocable, expiring) console session may travel in one, never an + // access key secret. + if t := r.URL.Query().Get("access_token"); strings.HasPrefix(t, "hcs_") { + token = t + } } if token == "" { return nil, core.Errf(http.StatusUnauthorized, "MissingAuthenticationToken", "request is not signed: send 'Authorization: Bearer :'") @@ -384,7 +416,7 @@ func (s *Service) userContext(u User) CondContext { } func (s *Service) principal(u User, keyID string) *httpx.Principal { - p := &httpx.Principal{AccountID: s.env.AccountID, UserName: u.Name, ARN: u.ARN, Root: u.Root, AccessKey: keyID, Context: s.userContext(u)} + p := &httpx.Principal{AccountID: s.env.AccountID, UserName: u.Name, ARN: u.ARN, Root: u.Root, AccessKey: keyID, Context: s.userContext(u), ResolveResource: s.resolveResource} if u.Root { p.Can = func(string, string) bool { return true } p.Identity = func(string, string, map[string][]string) httpx.Decision { return httpx.Allowed } diff --git a/cli/internal/svc/iam/login_test.go b/cli/internal/svc/iam/login_test.go new file mode 100644 index 00000000..69112afb --- /dev/null +++ b/cli/internal/svc/iam/login_test.go @@ -0,0 +1,89 @@ +package iam_test + +import ( + "bytes" + "encoding/json" + "net/http" + "strings" + "sync" + "testing" + + "github.com/homecloudhq/homecloud/cli/internal/awsapi/awstest" +) + +// A burst of parallel guesses must not get past the throttle just because none +// of them had failed yet when the others were checked. +func TestLoginThrottleHoldsUnderParallelBurst(t *testing.T) { + h := awstest.New(t) + var mu sync.Mutex + codes := map[int]int{} + var wg sync.WaitGroup + for i := 0; i < 60; i++ { + wg.Add(1) + go func() { + defer wg.Done() + st := loginStatus(t, h, "root", "wrong-password") + mu.Lock() + codes[st]++ + mu.Unlock() + }() + } + wg.Wait() + if codes[401] > 10 || codes[429] < 50 { + t.Fatalf("status counts %v: more than 10 guesses were evaluated", codes) + } +} + +// Query-string credentials end up in access logs and browser history: only a +// console session may be sent that way, never an access key secret. +func TestQueryTokenAcceptsSessionsOnly(t *testing.T) { + h := awstest.New(t) + get := func(tok string) int { + resp, err := http.Get(h.URL + "/api/v1/auth/whoami?access_token=" + tok) + if err != nil { + t.Fatal(err) + } + resp.Body.Close() + return resp.StatusCode + } + if st := get(h.AccessKeyID + ":" + h.SecretKey); st != http.StatusUnauthorized { + t.Fatalf("an access key secret in the query string was accepted: %d", st) + } + if err := h.IAM.SetRootPassword("a-chosen-passphrase"); err != nil { + t.Fatal(err) + } + resp, err := http.Post(h.URL+"/api/v1/auth/login", "application/json", strings.NewReader(`{"username":"root","password":"a-chosen-passphrase"}`)) + if err != nil { + t.Fatal(err) + } + var out struct{ Token string } + _ = json.NewDecoder(resp.Body).Decode(&out) + resp.Body.Close() + if st := get(out.Token); st != http.StatusOK { + t.Fatalf("a session token in the query string: %d", st) + } +} + +// Temporary credentials cannot mint new ones: a stolen session token would +// otherwise be renewable forever, surviving the revocation of the user's keys. +func TestSessionTokensCannotBeRenewed(t *testing.T) { + h := awstest.New(t) + c := h.AWSJSON(t, "sts", "get-session-token")["Credentials"].(map[string]any) + out, err := h.AWSAs(t, c["AccessKeyId"].(string), c["SecretAccessKey"].(string), c["SessionToken"].(string), "sts", "get-session-token") + if err == nil || !strings.Contains(out, "AccessDenied") { + t.Fatalf("renewing a session token: %v %s", err, out) + } +} + +func TestLoginRejectsHugeBodies(t *testing.T) { + h := awstest.New(t) + body := `{"username":"root","password":"` + strings.Repeat("a", 200<<10) + `"}` + resp, err := http.Post(h.URL+"/api/v1/auth/login", "application/json", bytes.NewReader([]byte(body))) + if err != nil { + t.Fatal(err) + } + resp.Body.Close() + if resp.StatusCode != http.StatusBadRequest { + t.Fatalf("status %d", resp.StatusCode) + } +} diff --git a/cli/internal/svc/iam/passrole_spelling_test.go b/cli/internal/svc/iam/passrole_spelling_test.go new file mode 100644 index 00000000..19db7e70 --- /dev/null +++ b/cli/internal/svc/iam/passrole_spelling_test.go @@ -0,0 +1,44 @@ +package iam_test + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/homecloudhq/homecloud/cli/internal/awsapi/awstest" + "github.com/homecloudhq/homecloud/cli/internal/httpx" +) + +// A scoped iam:PassRole policy must judge the role the caller really names: a +// bare name, or an ARN whose path part matches an Allow pattern, is still the +// role "admin" and must hit the Deny on it. +func TestPassRoleIsJudgedOnTheResolvedRole(t *testing.T) { + h := awstest.New(t) + acct := h.Env.AccountID + trust := `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":{"Service":"lambda.amazonaws.com"},"Action":"sts:AssumeRole"}]}` + h.AWS(t, "iam", "create-role", "--role-name", "admin", "--assume-role-policy-document", trust) + h.AWS(t, "iam", "create-role", "--role-name", "dev-app", "--assume-role-policy-document", trust) + h.Native(t, "POST", "/api/v1/iam/policies", map[string]any{"name": "pass-dev", "document": map[string]any{"Version": "2012-10-17", + "Statement": []any{ + map[string]any{"Effect": "Allow", "Action": "iam:PassRole", "Resource": "arn:aws:iam::" + acct + ":role/dev-*"}, + map[string]any{"Effect": "Deny", "Action": "iam:PassRole", "Resource": "arn:aws:iam::" + acct + ":role/admin"}, + map[string]any{"Effect": "Allow", "Action": "iam:PassRole", "Resource": "*"}, + }}}) + akid, secret := h.User(t, "dev", "pass-dev") + r := httptest.NewRequest(http.MethodGet, "/", nil) + r.Header.Set("Authorization", "Bearer "+akid+":"+secret) + p, err := h.IAM.Authenticate(r) + if err != nil { + t.Fatal(err) + } + for _, res := range []string{"admin", "arn:aws:iam::" + acct + ":role/admin", "arn:aws:iam::" + acct + ":role/dev-/admin", "arn:aws:iam::" + acct + ":role/x/admin"} { + if p.Permits("iam:PassRole", res, httpx.Access{}) { + t.Errorf("iam:PassRole on %q was allowed despite the Deny on role/admin", res) + } + } + for _, res := range []string{"dev-app", "arn:aws:iam::" + acct + ":role/dev-app"} { + if !p.Permits("iam:PassRole", res, httpx.Access{}) { + t.Errorf("iam:PassRole on %q was refused", res) + } + } +} diff --git a/cli/internal/svc/iam/roles.go b/cli/internal/svc/iam/roles.go index a30b51fd..0db01de6 100644 --- a/cli/internal/svc/iam/roles.go +++ b/cli/internal/svc/iam/roles.go @@ -98,6 +98,18 @@ func roleName(ref string) string { return ref } +// resolveResource maps the role a caller named in an iam:PassRole check (a bare +// name, or an ARN with a wrong path or account part) to the role's real ARN, so +// an Allow or Deny written for the real ARN applies whatever the spelling. +func (s *Service) resolveResource(action, resource string) string { + if action == "iam:PassRole" { + if r, err := s.GetRole(resource); err == nil { + return r.ARN + } + } + return resource +} + // GetRole returns a role by name or ARN. func (s *Service) GetRole(ref string) (Role, error) { r, err := store.Get[Role](s.env.Store, cRoles, roleName(ref)) @@ -141,7 +153,7 @@ func (s *Service) rolePrincipal(r Role, t tempCred) *httpx.Principal { AccountID: s.env.AccountID, UserName: "assumed-role/" + r.Name + "/" + t.SessionName, ARN: s.env.ARN("sts", "assumed-role/"+r.Name+"/"+t.SessionName), AccessKey: t.AccessKeyID, RoleName: r.Name, SessionName: t.SessionName, - Context: s.roleContext(r, t), + Context: s.roleContext(r, t), ResolveResource: s.resolveResource, } p.Can = func(action, resource string) bool { return decide(docs, boundary, action, resource, CondContext(p.Context)) == allow @@ -285,8 +297,11 @@ func (s *Service) ServiceRolePrincipal(ref, service, sessionName string) (*httpx // SessionToken issues temporary credentials carrying a user's own permissions. func (s *Service) SessionToken(p *httpx.Principal, seconds int) (Credentials, error) { - if p.RoleName != "" { - return Credentials{}, core.Errf(http.StatusForbidden, "AccessDenied", "GetSessionToken cannot be called with temporary role credentials") + if p.RoleName != "" || strings.HasPrefix(p.AccessKey, tempKeyPrefix) { + // As in AWS, only long-term credentials can mint session tokens; otherwise a + // stolen session could be renewed forever, outliving the revocation of the + // user's access keys. + return Credentials{}, core.Errf(http.StatusForbidden, "AccessDenied", "GetSessionToken cannot be called with temporary credentials") } ttl, err := stsTTL(seconds, nil) if err != nil { diff --git a/cli/internal/svc/iam/routes.go b/cli/internal/svc/iam/routes.go index 8ddb2029..84993158 100644 --- a/cli/internal/svc/iam/routes.go +++ b/cli/internal/svc/iam/routes.go @@ -16,7 +16,7 @@ import ( func (s *Service) Routes(r *httpx.Router) { iamRes := func(kind string) httpx.Opt { return httpx.Res("arn:aws:iam::{account}:" + kind) } - r.Handle("POST /api/v1/auth/login", "", s.login, httpx.Public()) + r.Handle("POST /api/v1/auth/login", "", s.login, httpx.Public(), httpx.SmallBody()) r.Handle("POST /api/v1/auth/logout", "sts:Logout", s.logout) r.Handle("GET /api/v1/auth/whoami", "sts:GetCallerIdentity", s.whoami) @@ -70,15 +70,23 @@ func (s *Service) login(c *httpx.Ctx) (any, error) { return nil, err } ip := httpx.ClientIP(c.R) - if s.throttled(ip, false) { + // Count the attempt before checking the password, so a burst of parallel + // requests cannot all pass the throttle before the first failure is recorded; + // a successful sign-in gives its attempt back. + if s.throttled(ip, true) { + s.releaseAttempt(ip) return nil, core.Errf(http.StatusTooManyRequests, "TooManyRequests", "too many failed sign-in attempts; try again in a few minutes") } u, err := store.Get[User](s.env.Store, cUsers, in.Username) - if err != nil || u.PasswordHash == "" || bcrypt.CompareHashAndPassword([]byte(u.PasswordHash), []byte(in.Password)) != nil { - s.throttled(ip, true) + hash := u.PasswordHash + if err != nil || hash == "" { + hash = dummyHash() // spend the same time whether or not the user exists + } + if bcrypt.CompareHashAndPassword([]byte(hash), []byte(in.Password)) != nil || err != nil || u.PasswordHash == "" { time.Sleep(300 * time.Millisecond) return nil, core.Errf(http.StatusUnauthorized, "AuthFailure", "incorrect user name or password") } + s.releaseAttempt(ip) token := "hcs_" + core.NewSecret(40) exp := time.Now().Add(sessionTTL) if err := store.Put(s.env.Store, cSessions, hashSecret(token), session{Token: "", UserName: u.Name, UserID: u.ID, Expires: exp}); err != nil { diff --git a/cli/internal/svc/lambda/apigw_aws_test.go b/cli/internal/svc/lambda/apigw_aws_test.go index accaffbf..40f13766 100644 --- a/cli/internal/svc/lambda/apigw_aws_test.go +++ b/cli/internal/svc/lambda/apigw_aws_test.go @@ -45,6 +45,7 @@ func gwHarness(t *testing.T, docker bool) (*awstest.Harness, *lambda.Service) { } l := lambda.New(h.Env, cw, v) l.Roles = roles{h.IAM} + l.HTTP = &http.Client{CheckRedirect: func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }} // tests proxy to 127.0.0.1 l.RegisterAWS() l.Routes(h.Router) return h, l diff --git a/cli/internal/svc/lambda/apigw_serve.go b/cli/internal/svc/lambda/apigw_serve.go index 5a8628d4..f4b7e27d 100644 --- a/cli/internal/svc/lambda/apigw_serve.go +++ b/cli/internal/svc/lambda/apigw_serve.go @@ -350,7 +350,7 @@ func (s *Service) proxyHTTP(c *httpx.Ctx, integ doc, r Route, p string, params m for _, h := range hopHeaders { req.Header.Del(h) } - cli := &http.Client{CheckRedirect: func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }} + cli := s.HTTP resp, err := cli.Do(req) if err != nil { if tooBig := (*http.MaxBytesError)(nil); errors.As(err, &tooBig) { diff --git a/cli/internal/svc/lambda/apigw_ssrf_test.go b/cli/internal/svc/lambda/apigw_ssrf_test.go new file mode 100644 index 00000000..2bd5cf3a --- /dev/null +++ b/cli/internal/svc/lambda/apigw_ssrf_test.go @@ -0,0 +1,29 @@ +package lambda_test + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/homecloudhq/homecloud/cli/internal/core" +) + +// An HTTP_PROXY integration must not be a way to read loopback services (the +// HomeCloud API, MinIO), the host provider's metadata service or the Docker +// bridge: the API is callable by anyone, so whoever can create an integration +// would otherwise get a read-through proxy into the host. +func TestAPIGatewayProxyRefusesInternalTargets(t *testing.T) { + h, l := gwHarness(t, false) + l.HTTP = core.SafeClient(0, 0) // what the server uses + secret := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Write([]byte("internal-secret")) })) + defer secret.Close() + + for _, target := range []string{secret.URL, "http://169.254.169.254/latest/meta-data/", "http://[::ffff:127.0.0.1]:1/", "http://[::1]:1/", "http://0.0.0.0:1/"} { + api := h.AWSJSON(t, "apigatewayv2", "create-api", "--name", "p", "--protocol-type", "HTTP", "--target", target) + id := api["ApiId"].(string) + code, body := gwCall(t, h, api["ApiEndpoint"].(string), "GET", "/", nil) + if code == http.StatusOK || body == "internal-secret" { + t.Fatalf("%s: proxied to an internal address: %d %q (api %s)", target, code, body, id) + } + } +} diff --git a/cli/internal/svc/lambda/aws.go b/cli/internal/svc/lambda/aws.go index a3297b5e..513d1048 100644 --- a/cli/internal/svc/lambda/aws.go +++ b/cli/internal/svc/lambda/aws.go @@ -342,6 +342,22 @@ func (s *Service) s3Code(q *awsapi.Req, bucket, key, version string) ([]byte, er // ---- functions ---- +// authorizeImage requires the caller to be allowed to pull an image of +// HomeCloud's own registry: the function runs it, and its code can read it. +func (s *Service) authorizeImage(q *awsapi.Req, image string) error { + repo, ok := core.LocalImageRepo(image, s.env.Cfg.ECRPort) + if !ok { + return nil + } + arn := s.env.ARN("ecr", "repository/"+repo) + for _, action := range []string{"ecr:BatchGetImage", "ecr:GetDownloadUrlForLayer"} { + if err := q.Check(action, arn); err != nil { + return err + } + } + return nil +} + func (s *Service) awsCreateFunction(q *awsapi.Req, _ map[string]string) error { var in struct { awsConfigIn @@ -363,6 +379,9 @@ func (s *Service) awsCreateFunction(q *awsapi.Req, _ map[string]string) error { if err := q.Authorize("lambda:CreateFunction", s.fnARN(in.FunctionName)); err != nil { return err } + if err := s.authorizeImage(q, in.Code.ImageUri); err != nil { + return err + } spec := createSpec{Name: in.FunctionName, configInput: in.config(), PackageType: in.PackageType, ImageURI: in.Code.ImageUri, Publish: in.Publish} spec.Tags = in.Tags switch { @@ -544,6 +563,9 @@ func (s *Service) awsUpdateCode(q *awsapi.Req, p map[string]string) error { if err := q.Authorize("lambda:UpdateFunctionCode", s.fnARN(name)); err != nil { return err } + if err := s.authorizeImage(q, in.ImageUri); err != nil { + return err + } spec := codeSpec{ImageURI: in.ImageUri, Publish: in.Publish, Revision: in.RevisionId} switch { case in.ZipFile != nil: diff --git a/cli/internal/svc/lambda/lambda.go b/cli/internal/svc/lambda/lambda.go index 47a11d10..0e1a1fcc 100644 --- a/cli/internal/svc/lambda/lambda.go +++ b/cli/internal/svc/lambda/lambda.go @@ -180,6 +180,9 @@ type Service struct { // ResolveImage maps an image URI (e.g. an ECR repository URI) to one the // local Docker engine can pull. ResolveImage func(uri string) string + // HTTP carries HTTP_PROXY integration requests; it refuses loopback, link-local + // and metadata addresses at dial time (tests point it at local backends). + HTTP *http.Client } // Credentials are temporary credentials for a function's execution role. @@ -199,7 +202,7 @@ type RoleSource interface { func New(env *svc.Env, cw *cloudwatch.Service, v *vpc.Service) *Service { s := &Service{env: env, cw: cw, vpc: v, pools: map[string]*pool{}, async: make(chan *asyncEvent, 10000), - urlKey: []byte(core.NewSecret(32)), now: time.Now} + urlKey: []byte(core.NewSecret(32)), now: time.Now, HTTP: core.SafeClient(0, 0)} if v != nil { v.RegisterMembers(s.fwMembers) } @@ -271,10 +274,29 @@ func (ci codeInput) zip() ([]byte, error) { return zipFiles(ci.Files) } +// maxUnzipped and maxZipEntries bound what a package may expand to. The zip is +// unpacked in memory on every cold start, so a small archive of zeros must be +// refused when it is uploaded, not when a function first runs. +const ( + maxUnzipped = 5 * maxCodeBytes + maxZipEntries = 20000 +) + func checkZip(b []byte) error { - if _, err := zip.NewReader(bytes.NewReader(b), int64(len(b))); err != nil { + zr, err := zip.NewReader(bytes.NewReader(b), int64(len(b))) + if err != nil { return core.BadRequest("Could not unzip uploaded file. Please check your file, then try to upload again. (%v)", err) } + if len(zr.File) > maxZipEntries { + return core.BadRequest("The package has more than %d files.", maxZipEntries) + } + var total uint64 + for _, f := range zr.File { + total += f.UncompressedSize64 + if total > maxUnzipped { + return core.BadRequest("Unzipped size must be smaller than %d bytes", int64(maxUnzipped)) + } + } return nil } @@ -322,14 +344,15 @@ func unzip(b []byte) (map[string][]byte, error) { if err != nil { return nil, err } - data, err := io.ReadAll(io.LimitReader(rc, 4*maxCodeBytes+1)) + // A header may understate a file's size: read no more than the budget left. + data, err := io.ReadAll(io.LimitReader(rc, maxUnzipped-total+1)) rc.Close() if err != nil { return nil, err } total += int64(len(data)) - if total > 5*maxCodeBytes { - return nil, core.BadRequest("Unzipped size must be smaller than %d bytes", 5*maxCodeBytes) + if total > maxUnzipped { + return nil, core.BadRequest("Unzipped size must be smaller than %d bytes", int64(maxUnzipped)) } out[clean] = data } diff --git a/cli/internal/svc/lambda/zipbomb_test.go b/cli/internal/svc/lambda/zipbomb_test.go new file mode 100644 index 00000000..c4a5c7ab --- /dev/null +++ b/cli/internal/svc/lambda/zipbomb_test.go @@ -0,0 +1,65 @@ +package lambda + +import ( + "archive/zip" + "bytes" + "io" + "strings" + "testing" +) + +type zeros int64 + +func (z *zeros) Read(p []byte) (int, error) { + if *z <= 0 { + return 0, io.EOF + } + n := int64(len(p)) + if n > int64(*z) { + n = int64(*z) + } + clear(p[:n]) + *z -= zeros(n) + return int(n), nil +} + +// A few kilobytes of zip can expand to gigabytes; the package is unpacked in +// memory at every cold start, so the expansion must be refused at upload. +func TestCheckZipRefusesDecompressionBombs(t *testing.T) { + var buf bytes.Buffer + zw := zip.NewWriter(&buf) + w, _ := zw.Create("big.bin") + z := zeros(maxUnzipped + 1<<20) + if _, err := io.Copy(w, &z); err != nil { + t.Fatal(err) + } + zw.Close() + if buf.Len() > maxCodeBytes { + t.Fatalf("test archive is %d bytes", buf.Len()) + } + if err := checkZip(buf.Bytes()); err == nil || !strings.Contains(err.Error(), "Unzipped size") { + t.Fatalf("a bomb of %d bytes (zipped %d) was accepted: %v", maxUnzipped+1<<20, buf.Len(), err) + } + if _, err := unzip(buf.Bytes()); err == nil { + t.Fatal("unzip expanded a bomb") + } + + many := new(bytes.Buffer) + zw = zip.NewWriter(many) + for i := 0; i <= maxZipEntries; i++ { + zw.Create(string(rune('a'+i%26)) + strings.Repeat("x", i%7) + "/" + strings.Repeat("y", i/26%50) + string(rune(0x4e00+i))) + } + zw.Close() + if err := checkZip(many.Bytes()); err == nil { + t.Fatal("an archive with too many entries was accepted") + } + + var ok bytes.Buffer + zw = zip.NewWriter(&ok) + w, _ = zw.Create("index.py") + w.Write([]byte("def handler(e, c): return 1")) + zw.Close() + if err := checkZip(ok.Bytes()); err != nil { + t.Fatal(err) + } +} diff --git a/cli/internal/svc/s3/aws.go b/cli/internal/svc/s3/aws.go index 6f0f32e0..971a4ada 100644 --- a/cli/internal/svc/s3/aws.go +++ b/cli/internal/svc/s3/aws.go @@ -150,7 +150,7 @@ func (s *Service) claimsUnsigned(r *http.Request) bool { return v2 || s.knownBucket(b) } p := r.URL.Path - for _, pre := range []string{"/api/", "/website/", "/lambda-url/", "/apigw/", "/cognito/", "/_next/"} { + for _, pre := range []string{"/api/", "/website/", "/lambda-url/", "/apigw/", "/cognito/", "/_next/", "/lambda-code/", "/_s3/"} { if strings.HasPrefix(p, pre) { return false } @@ -466,6 +466,26 @@ func (s *Service) serveAWS(a *s3req) error { return err } } + // Uploads can set retention or a legal hold through headers: AWS needs the + // matching permissions for that, not just s3:PutObject. + if op.object && (op.action == "s3:PutObject" || op.name == "CopyObject" || op.name == "CreateMultipartUpload") { + h := q.R.Header + need := "" + if h.Get("X-Amz-Object-Lock-Mode") != "" || h.Get("X-Amz-Object-Lock-Retain-Until-Date") != "" { + need = "s3:PutObjectRetention" + } + if h.Get("X-Amz-Object-Lock-Legal-Hold") != "" { + need = "s3:PutObjectLegalHold" + } + if need != "" { + if err := s.authorize(a, need, res); err != nil { + return err + } + if err := s.authorize(a, op.action, res); err != nil { // recorded as the call's action + return err + } + } + } if _, err := s.cl(); err != nil { return err } @@ -993,6 +1013,15 @@ func (s *Service) deleteObjects(a *s3req) error { return awsapi.Errorf(http.StatusBadRequest, "MalformedXML", "the XML you provided was not well-formed") } for _, o := range in.Objects { + // Keys in the body are forwarded as they are, so they need the same check + // as the key in the URL: "allowed/../secret" is authorized as a key under + // allowed/ but may name another object. + if err := checkKey(o.Key); err != nil { + return err + } + if len(o.Key) > 1024 || strings.ContainsRune(o.Key, 0) { + return awsapi.Errorf(http.StatusBadRequest, "KeyTooLongError", "the object key is invalid") + } act := "s3:DeleteObject" if o.VersionID != "" { act = "s3:DeleteObjectVersion" diff --git a/cli/internal/svc/s3/s3.go b/cli/internal/svc/s3/s3.go index 89069f65..ba2200a2 100644 --- a/cli/internal/svc/s3/s3.go +++ b/cli/internal/svc/s3/s3.go @@ -858,19 +858,31 @@ func (s *Service) website(c *httpx.Ctx) (any, error) { if err != nil { return nil, err } - if !s.isPublic(c.R.Context(), cl, bucket) { - return nil, core.Errf(http.StatusForbidden, "AccessDenied", "bucket %q hosts a website but does not allow public reads", bucket) - } + // The site is served with the server's own storage credentials, so each key + // must be one the bucket policy lets an anonymous caller read: a policy that + // merely mentions "*" and s3:GetObject (for one prefix, with a Deny or a + // condition) does not make the whole bucket public. + policy, _ := s.bucketPolicy(c.R.Context(), bucket) + acc := httpx.Access{Policy: policy, Keys: httpx.RequestContext(c.R)} + public := func(k string) bool { + return policy != "" && httpx.PermitsAnonymous("s3:GetObject", "arn:aws:s3:::"+bucket+"/"+k, acc) + } + denied := core.Errf(http.StatusForbidden, "AccessDenied", "bucket %q hosts a website but does not allow public reads of this object", bucket) if key == "" || strings.HasSuffix(key, "/") { key += m.IndexDocument } + if !public(key) { + return nil, denied + } if _, err := cl.StatObject(c.R.Context(), bucket, key, minio.StatObjectOptions{}); err != nil { - if _, err2 := cl.StatObject(c.R.Context(), bucket, key+"/"+m.IndexDocument, minio.StatObjectOptions{}); err2 == nil { - http.Redirect(c.W, c.R, c.R.URL.Path+"/", http.StatusFound) - c.MarkWritten() - return nil, nil + if public(key + "/" + m.IndexDocument) { + if _, err2 := cl.StatObject(c.R.Context(), bucket, key+"/"+m.IndexDocument, minio.StatObjectOptions{}); err2 == nil { + http.Redirect(c.W, c.R, c.R.URL.Path+"/", http.StatusFound) + c.MarkWritten() + return nil, nil + } } - if m.ErrorDocument != "" { + if m.ErrorDocument != "" && public(m.ErrorDocument) { return nil, s.stream(c, cl, bucket, m.ErrorDocument, "", false, http.StatusNotFound) } return nil, core.Errf(http.StatusNotFound, "NoSuchKey", "%s not found", key) diff --git a/cli/internal/svc/s3/sigv2.go b/cli/internal/svc/s3/sigv2.go index d4a22ce1..432e6a8a 100644 --- a/cli/internal/svc/s3/sigv2.go +++ b/cli/internal/svc/s3/sigv2.go @@ -97,6 +97,19 @@ func (s *Service) sigV2(a *s3req) (bool, error) { return true, awsapi.Errorf(http.StatusForbidden, "SignatureDoesNotMatch", "The request signature we calculated does not match the signature you provided. Check your key and signing method.") } + // SigV2 only signs the sub-resources listed above. Any other one the request + // carries (?retention, ?publicAccessBlock, ...) is not covered by the signature, + // so whoever holds a presigned URL could append it to change the operation. + for k := range a.query { + if v2SubResources[k] || valueParams[k] { + continue + } + for _, sub := range append(append([]subOps{}, bucketSubs...), objectSubs...) { + if sub.sub == k { + return true, awsapi.Errorf(http.StatusForbidden, "AccessDenied", "the ?%s sub-resource cannot be signed with signature version 2: use signature version 4", k) + } + } + } a.q.P = p return true, nil } diff --git a/cli/internal/svc/s3/website_security_test.go b/cli/internal/svc/s3/website_security_test.go new file mode 100644 index 00000000..9b0c8daa --- /dev/null +++ b/cli/internal/svc/s3/website_security_test.go @@ -0,0 +1,108 @@ +package s3 + +import ( + "io" + "net/http" + "os" + "strings" + "testing" +) + +func getStatus(t *testing.T, url string) (int, string) { + t.Helper() + resp, err := http.Get(url) + if err != nil { + t.Fatal(err) + } + defer resp.Body.Close() + b, _ := io.ReadAll(resp.Body) + return resp.StatusCode, string(b) +} + +// The website endpoint reads with the server's own storage credentials, so it +// may only serve keys the bucket policy lets an anonymous caller read: a policy +// that grants one prefix, or carries a Deny, does not publish the whole bucket. +func TestWebsiteServesOnlyWhatThePolicyAllows(t *testing.T) { + h, _ := newHarness(t) + b := bucketName() + h.AWS(t, "s3", "mb", "s3://"+b) + dir := t.TempDir() + _ = os.WriteFile(dir+"/f", []byte("content"), 0o600) + h.AWS(t, "s3", "cp", dir+"/f", "s3://"+b+"/pub/index.html") + h.AWS(t, "s3", "cp", dir+"/f", "s3://"+b+"/pub/secret.txt") + h.AWS(t, "s3", "cp", dir+"/f", "s3://"+b+"/private/data.txt") + h.AWS(t, "s3api", "put-bucket-website", "--bucket", b, "--website-configuration", `{"IndexDocument":{"Suffix":"index.html"}}`) + h.AWS(t, "s3api", "put-bucket-policy", "--bucket", b, "--policy", `{"Version":"2012-10-17","Statement":[ + {"Effect":"Allow","Principal":"*","Action":"s3:GetObject","Resource":"arn:aws:s3:::`+b+`/pub/*"}, + {"Effect":"Deny","Principal":"*","Action":"s3:GetObject","Resource":"arn:aws:s3:::`+b+`/pub/secret.txt"}]}`) + + if st, body := getStatus(t, h.URL+"/website/"+b+"/pub/index.html"); st != 200 || body != "content" { + t.Fatalf("allowed page: %d %q", st, body) + } + for _, p := range []string{"/private/data.txt", "/pub/secret.txt", "/index.html"} { + if st, body := getStatus(t, h.URL+"/website/"+b+p); st != 403 || strings.Contains(body, "content") { + t.Fatalf("%s: %d %q", p, st, body) + } + } +} + +// A SigV2 presigned URL signs only the sub-resources SigV2 knows; appending +// another one (?retention, ?publicAccessBlock, ...) must not reuse the signature. +func TestSigV2PresignedURLCannotGainSubresources(t *testing.T) { + h, _ := newHarness(t) + b := bucketName() + h.AWS(t, "s3", "mb", "s3://"+b) + out := h.Python(t, ` +s3 = boto3.client("s3", config=botocore.config.Config(signature_version="s3")) +print(s3.generate_presigned_url("get_object", Params={"Bucket": "`+b+`", "Key": "f"}, ExpiresIn=300)) +`) + u := strings.TrimSpace(out) + if !strings.Contains(u, "AWSAccessKeyId=") { + t.Skipf("not a SigV2 URL: %s", u) + } + for _, sub := range []string{"retention", "publicAccessBlock", "legal-hold"} { + st, body := getStatus(t, u+"&"+sub) + if st != 403 || !strings.Contains(body, "signature version 2") { + t.Fatalf("?%s appended to a SigV2 URL: %d %s", sub, st, body) + } + } +} + +// Locking an object through upload headers needs the retention / legal hold +// permissions, not just s3:PutObject. +func TestObjectLockHeadersNeedTheirOwnPermissions(t *testing.T) { + h, _ := newHarness(t) + b := bucketName() + h.AWS(t, "s3", "mb", "s3://"+b) + dir := t.TempDir() + _ = os.WriteFile(dir+"/f", []byte("x"), 0o600) + ak, sk := h.User(t, "uploader") + h.Native(t, "PUT", "/api/v1/iam/users/uploader/inline-policies/one", map[string]any{ + "Version": "2012-10-17", "Statement": []any{map[string]any{"Effect": "Allow", "Action": "s3:PutObject", + "Resource": []string{"arn:aws:s3:::" + b + "/*"}}}}) + out, err := h.AWSAs(t, ak, sk, "", "s3api", "put-object", "--bucket", b, "--key", "k", "--body", dir+"/f", "--object-lock-legal-hold-status", "ON") + mustFail(t, out, err, "s3:PutObjectLegalHold") + out, err = h.AWSAs(t, ak, sk, "", "s3api", "put-object", "--bucket", b, "--key", "k", "--body", dir+"/f", + "--object-lock-mode", "COMPLIANCE", "--object-lock-retain-until-date", "2999-01-01T00:00:00Z") + mustFail(t, out, err, "s3:PutObjectRetention") +} + +// Keys in a DeleteObjects body are forwarded as they are, so "allowed/../x" must +// not be authorized as a key under allowed/. +func TestDeleteObjectsRejectsDotSegments(t *testing.T) { + h, _ := newHarness(t) + b := bucketName() + h.AWS(t, "s3", "mb", "s3://"+b) + dir := t.TempDir() + _ = os.WriteFile(dir+"/f", []byte("x"), 0o600) + h.AWS(t, "s3", "cp", dir+"/f", "s3://"+b+"/keep") + ak, sk := h.User(t, "deleter") + h.Native(t, "PUT", "/api/v1/iam/users/deleter/inline-policies/one", map[string]any{ + "Version": "2012-10-17", "Statement": []any{map[string]any{"Effect": "Allow", "Action": "s3:*", + "Resource": []string{"arn:aws:s3:::" + b, "arn:aws:s3:::" + b + "/allowed/*"}}}}) + out, err := h.AWSAs(t, ak, sk, "", "s3api", "delete-objects", "--bucket", b, "--delete", `{"Objects":[{"Key":"allowed/../keep"}]}`) + mustFail(t, out, err, "InvalidArgument") + if out := h.AWS(t, "s3", "ls", "s3://"+b); !strings.Contains(out, "keep") { + t.Fatalf("object deleted through a dot segment: %s", out) + } +} diff --git a/cli/internal/svc/secrets/rotation.go b/cli/internal/svc/secrets/rotation.go index fbb3cbca..07c841de 100644 --- a/cli/internal/svc/secrets/rotation.go +++ b/cli/internal/svc/secrets/rotation.go @@ -109,6 +109,10 @@ func (s *Service) rotate(az Authz, ref string, in RotateInput) (Secret, string, if !strings.HasPrefix(fn, "arn:") { fn = s.env.ARN("lambda", "function:"+fn) } + fn = core.CanonicalARN(fn) + if !core.IsLocalARN(fn, s.env.AccountID) { + return sec, "", core.Errf(http.StatusBadRequest, "InvalidParameterException", "The rotation function must be a Lambda function of this account and region.") + } // Secrets Manager invokes the function on the caller's behalf. if err := az("lambda:InvokeFunction", fn); err != nil { return sec, "", err diff --git a/cli/internal/svc/sns/aws.go b/cli/internal/svc/sns/aws.go index 7511ecb5..286e4823 100644 --- a/cli/internal/svc/sns/aws.go +++ b/cli/internal/svc/sns/aws.go @@ -549,7 +549,7 @@ func (s *Service) awsSetSubscriptionAttributes(q *awsapi.Req) (any, error) { return nil, err } name, value := q.Param("AttributeName"), q.Param("AttributeValue") - _, err = store.Update(s.env.Store, cSubs, sub.ARN, func(x *Subscription) error { return s.setSubAttribute(x, name, value) }) + _, err = store.Update(s.env.Store, cSubs, sub.ARN, func(x *Subscription) error { return s.setSubAttribute(x, name, value, q.Check) }) return nil, err } diff --git a/cli/internal/svc/sns/aws_test.go b/cli/internal/svc/sns/aws_test.go index faeecc64..aa5c75ea 100644 --- a/cli/internal/svc/sns/aws_test.go +++ b/cli/internal/svc/sns/aws_test.go @@ -469,6 +469,24 @@ func TestIAMDenied(t *testing.T) { } } +// A subscription's dead-letter queue receives messages on the subscriber's +// behalf, so naming one needs sqs:SendMessage on it, as subscribing a queue does. +func TestRedrivePolicyNeedsSendMessageOnTheQueue(t *testing.T) { + h, _, _ := harness(t) + acct := h.Env.AccountID + arn := h.AWSJSON(t, "sns", "create-topic", "--name", "t")["TopicArn"].(string) + h.AWS(t, "sqs", "create-queue", "--queue-name", "victim") + h.AWS(t, "sqs", "create-queue", "--queue-name", "mine") + sub := h.AWSJSON(t, "sns", "subscribe", "--topic-arn", arn, "--protocol", "sqs", "--notification-endpoint", "arn:aws:sqs:us-east-1:"+acct+":mine")["SubscriptionArn"].(string) + akid, secret := h.User(t, "topics-only", "SNSFullAccess") + rp := `{"deadLetterTargetArn":"arn:aws:sqs:us-east-1:` + acct + `:victim"}` + out, err := h.AWSAs(t, akid, secret, "", "sns", "set-subscription-attributes", "--subscription-arn", sub, "--attribute-name", "RedrivePolicy", "--attribute-value", rp) + if err == nil || !strings.Contains(out, "AuthorizationError") { + t.Fatalf("redrive policy naming a queue the caller may not write to: %v %s", err, out) + } + h.AWS(t, "sns", "set-subscription-attributes", "--subscription-arn", sub, "--attribute-name", "RedrivePolicy", "--attribute-value", rp) // root may +} + // TestNativeAPI checks the native routes and their interplay with the AWS API. func TestNativeAPI(t *testing.T) { h, _, _ := harness(t) diff --git a/cli/internal/svc/sns/sns.go b/cli/internal/svc/sns/sns.go index 819b4998..ebd30c7d 100644 --- a/cli/internal/svc/sns/sns.go +++ b/cli/internal/svc/sns/sns.go @@ -355,7 +355,7 @@ type subscribeInput struct { var phoneRe = regexp.MustCompile(`^\+?[0-9]{5,15}$`) // setSubAttribute validates and applies one subscription attribute. -func (s *Service) setSubAttribute(sub *Subscription, name, value string) error { +func (s *Service) setSubAttribute(sub *Subscription, name, value string, authorize func(action, resource string) error) error { switch name { case "RawMessageDelivery": if value != "true" && value != "false" { @@ -400,9 +400,14 @@ func (s *Service) setSubAttribute(sub *Subscription, name, value string) error { if json.Unmarshal([]byte(value), &rp) != nil || !strings.HasPrefix(core.CanonicalARN(rp.DeadLetterTargetArn), "arn:"+core.Partition+":sqs:") { return errInvalid("RedrivePolicy: deadLetterTargetArn must be an Amazon SQS queue ARN") } - if _, ok := s.sqs.QueueARN(sqs.NameFromARN(rp.DeadLetterTargetArn)); !ok { + dlq, ok := s.sqs.QueueARN(sqs.NameFromARN(rp.DeadLetterTargetArn)) + if !ok || !core.IsLocalARN(rp.DeadLetterTargetArn, s.env.AccountID) { return errInvalid("RedrivePolicy: dead-letter queue %s does not exist", rp.DeadLetterTargetArn) } + // Failed deliveries are written to the queue on the subscriber's behalf. + if err := authorize("sqs:SendMessage", dlq); err != nil { + return err + } sub.RedrivePolicy = value case "DeliveryPolicy": if value != "" { @@ -497,7 +502,7 @@ func (s *Service) subscribe(t Topic, in subscribeInput, authorize func(action, r if k == "FilterPolicyScope" { continue } - if err := s.setSubAttribute(&sub, k, v); err != nil { + if err := s.setSubAttribute(&sub, k, v, authorize); err != nil { return Subscription{}, err } } @@ -1349,7 +1354,7 @@ func (s *Service) updateSub(c *httpx.Ctx) (any, error) { } for _, k := range []string{"RawMessageDelivery", "FilterPolicy", "FilterPolicyScope"} { if v, ok := attrs[k]; ok { - if err := s.setSubAttribute(x, k, v); err != nil { + if err := s.setSubAttribute(x, k, v, c.Authorize); err != nil { return err } } diff --git a/cli/internal/svc/sqs/aws.go b/cli/internal/svc/sqs/aws.go index 3823da84..80aa2e76 100644 --- a/cli/internal/svc/sqs/aws.go +++ b/cli/internal/svc/sqs/aws.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "log" "net/http" "net/url" "reflect" @@ -867,7 +868,8 @@ func entryError(id string, err error) batchError { if errors.As(e, &ae) { return batchError{Id: id, SenderFault: ae.Status < 500, Code: ae.Code, Message: ae.Message} } - return batchError{Id: id, Code: "InternalError", Message: err.Error()} + log.Printf("sqs: batch entry %s: %v", id, err) + return batchError{Id: id, Code: "InternalError", Message: core.InternalErrorMessage} } func isNoQueue(err error) bool { diff --git a/cli/internal/web/web.go b/cli/internal/web/web.go index aa21f0c4..bd421c1e 100644 --- a/cli/internal/web/web.go +++ b/cli/internal/web/web.go @@ -14,7 +14,23 @@ var dist embed.FS // Handler serves the console. Unknown paths fall back to their .html page or // to index.html so client-side routes survive a reload. -func Handler() http.Handler { +func Handler() http.Handler { return withSecurityHeaders(handler()) } + +// withSecurityHeaders forbids framing the console (clickjacking of a signed-in +// administrator), plugins and foreign form targets, and keeps the console's +// URLs out of Referer headers. +func withSecurityHeaders(h http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hd := w.Header() + hd.Set("X-Frame-Options", "DENY") + hd.Set("Content-Security-Policy", "frame-ancestors 'none'; object-src 'none'; base-uri 'self'; form-action 'self'") + hd.Set("X-Content-Type-Options", "nosniff") + hd.Set("Referrer-Policy", "no-referrer") + h.ServeHTTP(w, r) + }) +} + +func handler() http.Handler { root, _ := fs.Sub(dist, "dist") files := http.FileServerFS(root) if _, err := fs.Stat(root, "index.html"); err != nil { diff --git a/cli/internal/web/web_test.go b/cli/internal/web/web_test.go new file mode 100644 index 00000000..ae3f60a9 --- /dev/null +++ b/cli/internal/web/web_test.go @@ -0,0 +1,19 @@ +package web + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +func TestConsoleCannotBeFramed(t *testing.T) { + w := httptest.NewRecorder() + Handler().ServeHTTP(w, httptest.NewRequest(http.MethodGet, "/", nil)) + if w.Header().Get("X-Frame-Options") != "DENY" || !strings.Contains(w.Header().Get("Content-Security-Policy"), "frame-ancestors 'none'") { + t.Fatalf("console can be framed: %v", w.Header()) + } + if w.Header().Get("Referrer-Policy") != "no-referrer" { + t.Fatal("console sends Referer headers") + } +} diff --git a/console/app/login/page.tsx b/console/app/login/page.tsx index 419994e5..535130cf 100644 --- a/console/app/login/page.tsx +++ b/console/app/login/page.tsx @@ -10,8 +10,21 @@ import { Label } from "@/components/ui/label" import { Logo } from "@/components/console/topbar" import { BASE_PATH, errorMessage, getSession, login } from "@/lib/api" +// safeNext only lets a same-site path through. Browsers read "\" as "/" and drop +// tabs and newlines inside URLs, so "/\evil.example" and "//evil.example" +// both mean "//evil.example": refuse them by character, then confirm by resolving +// the path against this origin. function safeNext(next: string | null): string { if (!next || !next.startsWith("/") || next.startsWith("//") || next.startsWith("/login")) return "/" + // eslint-disable-next-line no-control-regex + if (/[\\\u0000-\u001f\u007f]/.test(next)) return "/" + if (typeof window !== "undefined") { + try { + if (new URL(next, window.location.origin).origin !== window.location.origin) return "/" + } catch { + return "/" + } + } return next } diff --git a/console/node_modules b/console/node_modules new file mode 120000 index 00000000..2a560b7b --- /dev/null +++ b/console/node_modules @@ -0,0 +1 @@ +/Users/drk1rd/Documents/GitHub/homecloud/console/node_modules \ No newline at end of file diff --git a/docs/install-server.md b/docs/install-server.md index abdf60e7..78060b09 100644 --- a/docs/install-server.md +++ b/docs/install-server.md @@ -483,6 +483,15 @@ that door open: Console sign-in is throttled after 10 failures per client IP in 5 minutes, but that is no substitute. - Keep the API on loopback behind a proxy where you can, and firewall the ports HomeCloud publishes ([Firewall](#7-firewall)). +- **Outbound requests made for users** (SNS and alarm webhooks, API Gateway `HTTP_PROXY` integrations) + never reach loopback, link-local (including the provider's `169.254.169.254` metadata service), this + host's own addresses or other metadata endpoints, checked when the connection is made. Private LAN + ranges stay reachable because homelabs call their own services; on a multi-user VPS set + `HOMECLOUD_DENY_PRIVATE_TARGETS=1` in the service environment to block RFC 1918 and unique-local + addresses too. +- **`homecloud:CreateBackup` is account-root.** A backup contains `master.key` and everything it + decrypts, so grant that action (and `homecloud:*`) only to administrators, and encrypt backups before + they leave the machine. - Keep the host patched (`unattended-upgrades` on Ubuntu and Debian) and upgrade HomeCloud regularly. ## 14. Troubleshooting diff --git a/docs/security-audit-2026-10.md b/docs/security-audit-2026-10.md new file mode 100644 index 00000000..7d8884a2 --- /dev/null +++ b/docs/security-audit-2026-10.md @@ -0,0 +1,257 @@ +# Security audit, October 2026 + +Pre-launch review of HomeCloud before it is exposed on public VPSes. Scope, in priority order: +authentication (SigV4, sessions, sign-in, unsigned operations), authorization (actions and resources, +`iam:PassRole`, confused deputies), SSRF, container and host isolation, secrets handling and the web +console. Every fix below has a regression test and its own commit on the `security-audit` branch. + +Findings marked *fixed* are closed. *Accepted* findings are left on purpose, with the reasoning. Nothing +in the VM code (`svc/ec2/vm*.go`, `svc/ec2/vm/`) was reviewed or changed. + +## Summary + +| # | Severity | Finding | Status | +| --- | --- | --- | --- | +| 1 | High | AWS handler buffered up to 100 MB of request body before checking the signature | Fixed | +| 2 | High | API Gateway `HTTP_PROXY` integrations were an unrestricted, response-reading proxy into the host | Fixed | +| 3 | High | Inline object views ran scripts with the viewer's session token in the URL | Fixed | +| 4 | High | Native ECS task definitions (and CloudFormation) skipped `iam:PassRole` | Fixed | +| 5 | High | Native EC2 launch, launch templates and Auto Scaling skipped `iam:PassRole` for instance profiles | Fixed | +| 6 | Medium | Sign-in throttle could be bypassed with parallel requests; user existence observable by timing | Fixed | +| 7 | Medium | Open redirect after console sign-in (`/\host`, `//host`) | Fixed | +| 8 | Medium | `iam:PassRole` and target ARNs judged on the caller's spelling, dodging Deny statements | Fixed | +| 9 | Medium | Outbound webhook blocklist missed IPv6-embedded IPv4, metadata and host-local addresses | Fixed | +| 10 | Medium | Lambda packages could decompress to gigabytes at every cold start | Fixed | +| 11 | Medium | Unexpected errors returned Go error text (paths, Docker output) to clients and CloudTrail | Fixed | +| 12 | Medium | Lambda and ECS could run images of any private ECR repository without `ecr:` permissions | Fixed | +| 13 | Low | SNS subscription dead-letter queue needed no `sqs:SendMessage` | Fixed | +| 14 | Low | `?access_token=` accepted access key secrets; temporary credentials could mint new ones | Fixed | +| 15 | Low | SigV4 accepted requests that did not sign `host` / `x-amz-target` | Fixed | +| 16 | Low | Console could be framed (clickjacking); public routes accepted 64 MB bodies | Fixed | +| 17 | Info | `homecloud:CreateBackup` is account-root (the archive contains `master.key`) | Accepted | +| 18 | Info | Sealed secrets use one key with no per-purpose AAD | Accepted | +| 19 | Info | SigV4 requests can be replayed inside the 15 minute skew window | Accepted | +| 20 | Info | Step Functions machines without a role act as a trusted principal | Accepted | +| 21 | Info | RFC 1918 targets stay reachable by default for webhooks and proxy integrations | Accepted | +| 22 | Info | Docker pulls user-chosen images from arbitrary registries | Accepted | +| 23 | High | Website hosting served any key of a bucket whose policy merely mentioned `*` and `s3:GetObject` | Fixed | +| 24 | High | Function / HTTP API responses could replace the sandbox CSP and run script on the console origin | Fixed | +| 25 | Medium | `DeleteObjects` body keys skipped the `..` check | Fixed | +| 26 | Medium | Cognito: no per-account limit on password guesses, none on self sign-up | Fixed | +| 27 | Medium | SigV2 presigned URLs accepted unsigned sub-resources (`?retention`, ...) | Fixed | +| 28 | Low | Object-lock upload headers needed only `s3:PutObject` | Fixed | +| 29 | Low | A bucket named `lambda-code` broke `/lambda-code/` downloads | Fixed | +| 30 | Medium | New Cognito user pools allow open, auto-confirmed self sign-up | Accepted | +| 31 | Low | S3 Block Public Access settings are stored but not enforced | Accepted | +| 32 | Low | Cognito `ForgotPassword`/`ConfirmSignUp` reveal whether a user exists | Accepted | +| 33 | Low | SNS unsubscribe link needs only the subscription ARN | Accepted | +| 34 | Low | Self-signed TLS bootstrap can leave a certificate without its key after a crash | Accepted | +| 35 | Low | `CopyObject` source parsing may differ from MinIO's (not confirmed) | Accepted | + +## Fixed + +### 1. Body buffered before authentication (awsapi) + +`awsapi.Handler` read up to `maxBody` (100 MB) into memory and hashed it before it looked at the +signature. Anyone who could reach the port could hold 100 MB per connection open with an unsigned +request or a signature naming a made-up access key. The handler now checks the signature's freshness and +the access key first, verifies the signature before reading the body whenever the client declared +`X-Amz-Content-Sha256` (and always for presigned URLs), and caps unsigned (public operation) bodies at +1 MB. Native public routes (sign-in, Cognito) use a 64 KB cap instead of 64 MB. Servers also got an idle +timeout and a header size limit. Tests: `awsapi/preauth_test.go`, `svc/iam/login_test.go`. + +### 2. API Gateway HTTP_PROXY SSRF + +`proxyHTTP` used a default `http.Client`, so a caller with `apigateway:*` on an API could point an +integration at loopback services, the host provider's metadata service or the Docker bridge and read +the responses through the public API route. It now uses `core.SafeClient`: the destination is checked +after DNS resolution at dial time (so rebinding and redirects cannot get around it), proxy environment +variables are ignored and redirects are not followed. Test: `svc/lambda/apigw_ssrf_test.go`. + +### 9. Webhook blocklist + +`core.blockedIP` now also covers IPv4-mapped, NAT64 and 6to4 forms, `0.0.0.0/8`, `100.100.100.200`, +`192.0.0.192`, `fd00:ec2::/32` and every address of the host itself (its public address and the Docker +bridge gateways, through which the HomeCloud API and Docker daemon are reachable). See accepted risk 21 +for private ranges. Test: `core/targets_test.go`. + +### 3. Script in inline object views could read the session token + +The console opens an object with `?access_token=` in the URL and the response was sandboxed with +`allow-scripts`, so an uploaded HTML object could read `location.search` and send the viewer's session +away: any user who can write to a bucket could take over an administrator who previewed the object. +Inline views now use `sandbox; default-src 'none'` (no scripts, no outbound loads) and API responses send +`Referrer-Policy: no-referrer`. Hosted websites, function URLs and HTTP APIs keep their scripts; their +URLs carry no token. Test: `server/sandbox_test.go`. + +### 4 and 5. iam:PassRole on native routes + +The PassRole check existed only on the AWS wire API. The native ECS task definition route, the native +EC2 launch route, launch templates carrying an instance profile and Auto Scaling groups launching from +such a template did not check it, so a user with `ecs:RegisterTaskDefinition` + `ecs:RunTask`, or +`ec2:RunInstances`, could run code holding any role that trusts the service and read its credentials. +CloudFormation uses the same native routes. The check now lives in `registerTaskDef` and +`ec2.PassProfile` and is applied on every path. Tests: `svc/ecs/passrole_test.go`, +`svc/autoscaling/passrole_test.go`. + +### 8. Resource spelling + +Policies were evaluated against the resource string as the caller wrote it. `role/dev-/admin` or the bare +name `admin` matched an Allow on `role/dev-*` and missed a Deny on `role/admin`, while IAM resolved +both to the role `admin`. `Principal.Permits` now canonicalizes the resource and IAM resolves role +references to the real ARN for `iam:PassRole`. Deliveries to EventBridge, scheduler, CloudWatch and Step +Functions targets and Secrets Manager rotation functions now reject ARNs of another account or region +instead of delivering to the same-named local resource. Tests: `svc/iam/passrole_spelling_test.go`, +`server/targets_arn_test.go`. + +### 6. Sign-in throttle + +The per-IP throttle counted a failure only after bcrypt had finished, so a burst of parallel guesses all +passed the check. The slot is now reserved before the password check and returned on success. Unknown +users pay the same bcrypt cost as known ones, so response time no longer reveals which user names exist. +Test: `svc/iam/login_test.go`. + +### 7. Console open redirect + +The post-sign-in `next` parameter refused `//host` but accepted `/\host` and `//host`, which browsers +treat as `//host`. It is now rejected on backslashes and control characters and must resolve to the +console's own origin (`console/app/login/page.tsx`). The console has no test runner; the predicate was +checked against the crafted values with Node and `tsc --noEmit` passes. + +### 10. Lambda decompression bombs + +Packages were only checked to be valid zips and then unpacked in memory at every cold start, with a second +copy when loaded into the container. `checkZip` now sums the declared uncompressed sizes against the 250 MB +limit and caps the entry count, and `unzip` never reads more than the budget left. Test: +`svc/lambda/zipbomb_test.go`. + +### 11. Internal error text + +500 responses, SQS batch results and, through the error message, CloudTrail `LookupEvents` carried raw Go +errors (file paths, Docker daemon output, database errors). Clients now get a fixed message; the detail +stays in the server log. Test: `awsapi/preauth_test.go`. + +### 12. ECR image pulls + +Lambda container functions and ECS task definitions pulled from the local registry with no `ecr:` check, +so any user able to create a function or task could run and read another team's private repository. Both +now need `ecr:BatchGetImage` and `ecr:GetDownloadUrlForLayer` on the repository. Test: +`svc/ecs/passrole_test.go`, `core/targets_test.go`. + +### 13. SNS dead-letter queue + +A `RedrivePolicy` only had to name an existing queue; failed deliveries then wrote into it. Naming one now +needs `sqs:SendMessage` on it. Test: `svc/sns/aws_test.go`. + +### 14. Token handling + +`?access_token=` accepted `accessKeyId:secret`; only console session tokens (revocable, expiring) are accepted +in a URL now. `sts:GetSessionToken` refuses temporary credentials, as AWS does, so a stolen session cannot +be renewed forever. Tests: `svc/iam/login_test.go`. + +### 15. SigV4 signed headers + +Requests whose `SignedHeaders` omit `host`, or `x-amz-target` when it is sent, are rejected, as AWS does. +Test: `awsapi/preauth_test.go`. + +### 16. Console headers + +The console is served with `X-Frame-Options: DENY`, `frame-ancestors 'none'`, `object-src 'none'`, +`base-uri`/`form-action 'self'` and `Referrer-Policy: no-referrer`. A script-src policy is not set: the +statically exported Next.js pages rely on inline bootstrap scripts. Test: `web/web_test.go`. + +### 23. Website hosting and bucket policies + +`isPublic` looked for the strings `"*"` and `s3:GetObject` in MinIO's copy of the bucket policy and then +served every key with the server's storage credentials. A policy granting one prefix, or with a Deny or a +condition, exposed the whole bucket. Each requested key (index, redirect probe, error document) is now +evaluated against the bucket policy as an anonymous caller. Test: `svc/s3/website_security_test.go`. + +### 24. Sandbox headers overridden by functions + +`respond` copied the function's headers over the sandbox `Content-Security-Policy`, and the HTTP_PROXY relay +copied upstream headers, so a function author could serve script on the console origin to an administrator +who opened its URL. The sandbox and `nosniff` headers are re-applied when the response is written. Test: +`server/sandbox_test.go`. + +### 25-29. S3 and Cognito hardening + +`DeleteObjects` body keys get the `..`/length checks URL keys already had (25). Password and SRP sign-in count +attempts per account across all addresses, and each pool bounds self sign-ups per window (26). SigV2 URLs +carrying a sub-resource outside SigV2's signed set are refused (27). Retention and legal-hold upload headers +need `s3:PutObjectRetention` / `s3:PutObjectLegalHold` (28). `/lambda-code/` and `/_s3/` are never claimed by +anonymous S3 routing (29). Tests: `svc/s3/website_security_test.go`, `svc/cognito/signup_limit_test.go`. + +## Accepted risks + +**30. Open Cognito sign-up by default.** New pools auto-confirm self sign-ups so the emulator works out of the +box, like AWS's own developer setups. Anyone who knows a client ID can register and obtain tokens that verify +for the pool; an API Gateway JWT authorizer with no audience therefore accepts any of them. On a public server +turn off self sign-up (`SelfSignUp=false`) or auto-confirm for pools that guard real APIs, and set the +authorizer's audience. + +**31. Block Public Access is cosmetic.** The four settings are stored and returned, but public bucket policies +still take effect. Do not rely on them; review bucket policies instead. + +**32. Cognito existence signals.** Recovery and confirmation calls distinguish unknown users. Sign-in itself +does not, and the per-account and per-address limits bound enumeration. + +**33. SNS unsubscribe links.** As in AWS, the link is authorized by knowing the subscription ARN. + +**34. TLS bootstrap.** `--tls-self-signed` writes the certificate before the key; a crash in between needs +the `tls/` directory removed. No security impact. + +**35. CopyObject source parsing.** HomeCloud and MinIO may decode `+` and `%3F` in `x-amz-copy-source` +differently. The difference could not be confirmed in MinIO's code from this repository. Exposure is limited +to principals who can already read some source object, and the cross-bucket test in `aws_security_test.go` +still passes. + +**17. `homecloud:CreateBackup` is account-root.** `GET /api/v1/system/backup` streams an archive containing +`master.key`, the state file (sealed secrets) and volumes. It needs `homecloud:CreateBackup`, which only +administrator policies carry. Requiring the root user would break administrators who are not root, so the +install guide now says to treat the action as root-equivalent and to encrypt backups off the machine. + +**18. One sealing key, no AAD.** `secrets.Seal` uses AES-256-GCM with random 96-bit nonces under one key for +every purpose. Swapping ciphertexts between fields needs write access to the state file, which already means +host compromise (and the key file next to it). Changing the format needs a migration; tracked for a later release. + +**19. Replay.** As in AWS, a captured SigV4 request can be replayed within 15 minutes (presigned URLs until +they expire). The mitigation is TLS, which the install guide requires for anything off a trusted network. + +**20. Role-less Step Functions machines.** They run as a trusted principal; each task's permission was checked +against the creator when the definition was saved, and changing the definition re-checks it against the editor. +Anyone with `states:StartExecution` can start it. AWS requires a role, which HomeCloud deliberately relaxes for +small setups. Use a role (and `iam:PassRole`) for machines that matter. + +**21. Private ranges stay reachable.** Webhooks and proxy integrations to RFC 1918 and unique-local addresses +work by default because the typical user calls their own LAN and VPC. On a shared VPS set +`HOMECLOUD_DENY_PRIVATE_TARGETS=1`. + +**22. Image pulls.** The Docker daemon pulls user-chosen image names from any registry. Registries are +contacted over HTTPS (plain HTTP only for loopback), the response is not returned to the caller, and the +daemon already runs with the privileges of the administrator who set it up. + +## Reviewed, not an issue + +- **Sessions and CSRF.** The console authenticates with `Authorization: Bearer` from local storage; there are + no cookies, so there is no session fixation and no CSRF on native routes. Sessions are stored hashed, expire, + and end on password change or user deletion. CORS `*` is safe for the same reason. +- **Passwords and keys.** bcrypt for console passwords; access key secrets stored as SHA-256 plus a sealed + copy, compared in constant time; `master.key` from `crypto/rand`, mode 0600 in a 0700 directory. +- **CloudTrail records** hold method, path (no query), action, resource, source and error only: no request + parameters or response elements, so no secret values or presigned URLs. +- **Path traversal.** Function, layer and snapshot names are validated before any path join; zip extraction + rejects `..`, never creates symlinks and stages through memory; backup restore rejects `..` and absolute + names; S3 keys never touch the host file system. +- **Container isolation.** Workloads get named volumes only: no bind mounts, `--privileged`, Docker socket, + host network/PID or devices; ECS volumes and mount points are rejected. +- **CloudFormation `TemplateURL`** only extracts a bucket and key and reads through the caller's own + credentials; the URL host is never dialed. +- **Console XSS.** React escapes all rendered names, tags and log lines; the only `dangerouslySetInnerHTML` + is the chart theme built from static configuration. + +## Reported for the VM worker (not touched) + +None. The VM code was excluded from this audit and has not been reviewed; it needs its own pass before +VM-backed instances are offered on public servers. `svc/ec2/imds.go` (the metadata service, outside the VM +files) is guarded by a per-process key held by the helper container and looks correct.