Merge various fixes for C-LCOW since conf-aci/0.2.5 (part 3)

cmd/gcs/main.go

@@ -4,6 +4,7 @@
 package main
 
 import (
+	"context"
 	"flag"
 	"fmt"
 	"io"
@@ -30,6 +31,7 @@ import (
 	"github.com/Microsoft/hcsshim/internal/log"
 	"github.com/Microsoft/hcsshim/internal/oc"
 	"github.com/Microsoft/hcsshim/internal/version"
+	"github.com/Microsoft/hcsshim/pkg/amdsevsnp"
 	"github.com/Microsoft/hcsshim/pkg/securitypolicy"
 	oci "github.com/opencontainers/runtime-spec/specs-go"
 )
@@ -414,6 +416,16 @@ func main() {
 
 	mux := bridge.NewBridgeMux()
 	b := bridge.New(mux, *v4)
+	// For confidential containers, we protect ourselves against attacks caused
+	// by concurrent modifications, by processing one request at a time.
+	sequential, err := amdsevsnp.IsSNP()
+	if err != nil {
+		// IsSNP cannot fail on LCOW
+		log.G(context.TODO()).WithError(err).Error("unexpected error from IsSNP")
+		// If it fails, we proceed with forceSequential enabled to be safe
+		sequential = true
+	}
+	b.Sequential = sequential
 	b.AssignHandlers(mux, h)
 
 	// Reconnect loop: dial the host, serve until the connection drops, then

internal/guest/bridge/bridge.go

@@ -177,6 +177,10 @@ type Bridge struct {
 	Handler Handler
 	// EnableV4 enables the v4+ bridge and the schema v2+ interfaces.
 	EnableV4 bool
+	// Setting Sequential to true will force the bridge to only process one
+	// request at a time, except for certain long-running operations (as defined
+	// in asyncMessages).
+	Sequential bool
 
 	// responseChan is the response channel used for both request/response
 	// and publish notification workflows.
@@ -256,6 +260,14 @@ func (b *Bridge) ShutdownRequested() bool {
 	return b.hasQuitPending.Load()
 }
 
+// Identify messages that will be processed asynchronously even in sequential
+// mode.  Note that in sequential mode, these messages will still wait for any
+// in-progress non-async messages to be handled before they are processed, but
+// once they are "acknowledged", the rest will be done asynchronously.
+func alwaysAsync(msgType prot.MessageIdentifier) bool {
+	return msgType == prot.ComputeSystemWaitForProcessV1
+}
+
 // AssignHandlers creates and assigns the appropriate bridge
 // events to be listen for and intercepted on `mux` before forwarding
 // to `gcs` for handling.
@@ -307,6 +319,10 @@ func (b *Bridge) ListenAndServe(bridgeIn io.ReadCloser, bridgeOut io.WriteCloser
 	defer b.disconnectNotifications()
 	defer close(b.quitChan)
 
+	if b.Sequential {
+		log.G(context.Background()).Info("bridge: ForceSequential enabled")
+	}
+
 	// Receive bridge requests and schedule them to be processed.
 	go func() {
 		var recverr error
@@ -409,30 +425,38 @@ func (b *Bridge) ListenAndServe(bridgeIn io.ReadCloser, bridgeOut io.WriteCloser
 	}()
 	// Process each bridge request async and create the response writer.
 	go func() {
-		for req := range requestChan {
-			go func(r *Request) {
-				br := bridgeResponse{
-					ctx: r.Context,
-					header: &prot.MessageHeader{
-						Type: prot.GetResponseIdentifier(r.Header.Type),
-						ID:   r.Header.ID,
-					},
-				}
-				resp, err := b.Handler.ServeMsg(r)
-				if resp == nil {
-					resp = &prot.MessageResponseBase{}
-				}
-				resp.Base().ActivityID = r.ActivityID
-				if err != nil {
-					span := trace.FromContext(r.Context)
-					if span != nil {
-						oc.SetSpanStatus(span, err)
-					}
-					setErrorForResponseBase(resp.Base(), err, "gcs" /* moduleName */)
+		doRequest := func(r *Request) {
+			br := bridgeResponse{
+				ctx: r.Context,
+				header: &prot.MessageHeader{
+					Type: prot.GetResponseIdentifier(r.Header.Type),
+					ID:   r.Header.ID,
+				},
+			}
+			resp, err := b.Handler.ServeMsg(r)
+			if resp == nil {
+				resp = &prot.MessageResponseBase{}
+			}
+			resp.Base().ActivityID = r.ActivityID
+			if err != nil {
+				span := trace.FromContext(r.Context)
+				if span != nil {
+					oc.SetSpanStatus(span, err)
 				}
-				br.response = resp
-				b.responseChan <- br
-			}(req)
+				setErrorForResponseBase(resp.Base(), err, "gcs" /* moduleName */)
+			}
+			br.response = resp
+			b.responseChan <- br
+		}
+
+		for req := range requestChan {
+			if b.Sequential && !alwaysAsync(req.Header.Type) {
+				// This will log warn after 5 seconds if the request is still
+				// being processed,
+				runSequentialRequest(req, doRequest)
+			} else {
+				go doRequest(req)
+			}
 		}
 	}()
 	// Process each bridge response sync. This channel is for request/response and publish workflows.
@@ -495,6 +519,32 @@ func (b *Bridge) ListenAndServe(bridgeIn io.ReadCloser, bridgeOut io.WriteCloser
 	}
 }
 
+// Do handleFn(r), but prints a warning if handleFn does not, or takes too long
+// to return.
+func runSequentialRequest(r *Request, handleFn func(*Request)) {
+	// Note that this is only a context used for triggering the blockage
+	// warning, the request processing still uses r.Context.  We don't want to
+	// cancel the request handling itself when we reach the 5s timeout.
+	timeoutCtx, cancel := context.WithTimeout(r.Context, 5*time.Second)
+	go func() {
+		<-timeoutCtx.Done()
+		if errors.Is(timeoutCtx.Err(), context.DeadlineExceeded) {
+			log.G(timeoutCtx).WithFields(logrus.Fields{
+				// We want to log those even though we're providing r.Context, since if
+				// the request never finishes the span end log will never get written,
+				// and we may therefore not be able to find out about the following info
+				// otherwise:
+				"message-type": r.Header.Type.String(),
+				"message-id":   r.Header.ID,
+				"activity-id":  r.ActivityID,
+				"container-id": r.ContainerID,
+			}).Warnf("bridge: request processing thread in sequential mode blocked on the current request for more than 5 seconds")
+		}
+	}()
+	defer cancel()
+	handleFn(r)
+}
+
 // PublishNotification writes a specific notification to the bridge.
 func (b *Bridge) PublishNotification(n *prot.ContainerNotification) {
 	ctx, span := oc.StartSpan(context.Background(),

internal/guest/bridge/bridge_v2.go

@@ -467,16 +467,10 @@ func (b *Bridge) deleteContainerStateV2(r *Request) (_ RequestResponse, err erro
 		return nil, errors.Wrapf(err, "failed to unmarshal JSON in message \"%s\"", r.Message)
 	}
 
-	c, err := b.hostState.GetCreatedContainer(request.ContainerID)
+	err = b.hostState.DeleteContainerState(ctx, request.ContainerID)
 	if err != nil {
 		return nil, err
 	}
-	// remove container state regardless of delete's success
-	defer b.hostState.RemoveContainer(request.ContainerID)
-
-	if err := c.Delete(ctx); err != nil {
-		return nil, err
-	}
 
 	return &prot.MessageResponseBase{}, nil
 }

internal/guest/network/network.go

@@ -9,6 +9,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"regexp"
 	"strings"
 	"time"
 
@@ -32,6 +33,18 @@ var (
 // maxDNSSearches is limited to 6 in `man 5 resolv.conf`
 const maxDNSSearches = 6
 
+var validHostnameRegex = regexp.MustCompile(`^[a-zA-Z0-9_\-\.]{0,255}$`)
+
+// Check that the hostname is safe. This function is less strict than
+// technically allowed, but ensures that when the hostname is inserted to
+// /etc/hosts, it cannot lead to injection attacks.
+func ValidateHostname(hostname string) error {
+	if !validHostnameRegex.MatchString(hostname) {
+		return errors.Errorf("hostname %q invalid: must match %s", hostname, validHostnameRegex.String())
+	}
+	return nil
+}
+
 // GenerateEtcHostsContent generates a /etc/hosts file based on `hostname`.
 func GenerateEtcHostsContent(ctx context.Context, hostname string) string {
 	_, span := oc.StartSpan(ctx, "network::GenerateEtcHostsContent")

internal/guest/network/network_test.go

@@ -7,6 +7,7 @@ import (
 	"context"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 	"time"
 )
@@ -70,6 +71,40 @@ func Test_GenerateResolvConfContent(t *testing.T) {
 	}
 }
 
+func Test_ValidateHostname(t *testing.T) {
+	validNames := []string{
+		"localhost",
+		"my-hostname",
+		"my.hostname",
+		"my-host-name123",
+		"_underscores.are.allowed.too",
+		"", // Allow not specifying a hostname
+	}
+
+	invalidNames := []string{
+		"localhost\n13.104.0.1 ip6-localhost ip6-loopback localhost",
+		"localhost\n2603:1000::1 ip6-localhost ip6-loopback",
+		"hello@microsoft.com",
+		"has space",
+		"has,comma",
+		"\x00",
+		"a\nb",
+		strings.Repeat("a", 1000),
+	}
+
+	for _, n := range validNames {
+		if err := ValidateHostname(n); err != nil {
+			t.Fatalf("expected %q to be valid, got: %v", n, err)
+		}
+	}
+
+	for _, n := range invalidNames {
+		if err := ValidateHostname(n); err == nil {
+			t.Fatalf("expected %q to be invalid, but got nil error", n)
+		}
+	}
+}
+
 func Test_GenerateEtcHostsContent(t *testing.T) {
 	type testcase struct {
 		name string

internal/guest/runtime/hcsv2/container.go

@@ -77,6 +77,9 @@ type Container struct {
 	// and deal with the extra pointer dereferencing overhead.
 	status atomic.Uint32
 
+	// Set to true when the init process for the container has exited
+	terminated atomic.Bool
+
 	// scratchDirPath represents the path inside the UVM where the scratch directory
 	// of this container is located. Usually, this is either `/run/gcs/c/<containerID>` or
 	// `/run/gcs/c/<UVMID>/container_<containerID>` if scratch is shared with UVM scratch.

internal/guest/runtime/hcsv2/process.go

@@ -99,6 +99,9 @@ func newProcess(c *Container, spec *oci.Process, process runtime.Process, pid ui
 			log.G(ctx).WithError(err).Error("failed to wait for runc process")
 		}
 		p.exitCode = exitCode
+		if p.init {
+			c.terminated.Store(true)
+		}
 		log.G(ctx).WithField("exitCode", p.exitCode).Debug("process exited")
 
 		// Free any process waiters

internal/guest/runtime/hcsv2/sandbox_container.go

@@ -50,6 +50,9 @@ func setupSandboxContainerSpec(ctx context.Context, id, sandboxRoot string, spec
 
 	// Write the hostname
 	hostname := spec.Hostname
+	if err = network.ValidateHostname(hostname); err != nil {
+		return err
+	}
 	if hostname == "" {
 		var err error
 		hostname, err = os.Hostname()

internal/guest/runtime/hcsv2/standalone_container.go

@@ -46,6 +46,9 @@ func setupStandaloneContainerSpec(ctx context.Context, id, rootDir string, spec
 	}()
 
 	hostname := spec.Hostname
+	if err = network.ValidateHostname(hostname); err != nil {
+		return err
+	}
 	if hostname == "" {
 		var err error
 		hostname, err = os.Hostname()