Pre-launch security audit: 29 fixes

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)

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)
+	}
+}

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 {

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.

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 ("<account>.dkr.ecr.<region>.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")