diff --git a/internal/api/handler_accounts.go b/internal/api/handler_accounts.go index 0c6a19dd..9cecffd6 100644 --- a/internal/api/handler_accounts.go +++ b/internal/api/handler_accounts.go @@ -361,6 +361,12 @@ func validateCloudAccountRequest(req CloudAccountRequest) error { return NewClientError(400, "external_id is required") } + if err := validateNoControlChars( + namedField{"name", req.Name}, namedField{"external_id", req.ExternalID}, + ); err != nil { + return err + } + if err := validateEmailFormat(req.ContactEmail); err != nil { return NewClientError(400, "invalid contact_email format") } diff --git a/internal/api/handler_accounts_controlchar_test.go b/internal/api/handler_accounts_controlchar_test.go new file mode 100644 index 00000000..76873ad0 --- /dev/null +++ b/internal/api/handler_accounts_controlchar_test.go @@ -0,0 +1,157 @@ +package api + +import ( + "context" + "encoding/json" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// accountControlChars are the control characters the account write paths +// must reject in single-line identifiers (#970). A tab is included: unlike +// the render guard, which lets one through for the session email, a name or +// external ID has no use for it. +var accountControlChars = map[string]string{ + "LF": "\n", + "CRLF": "\r\n", + "CR": "\r", + "TAB": "\t", + "NUL": "\x00", + "ESC": "\x1b", + "DEL": "\x7f", + "NEL": "\u0085", +} + +// accountBody returns a create/update body for an Azure account (no AWS +// external-ID charset rule applies), with name and external_id as given. +func accountBody(t *testing.T, name, externalID string) string { + t.Helper() + b, err := json.Marshal(map[string]string{"name": name, "provider": "azure", "external_id": externalID}) + require.NoError(t, err) + return string(b) +} + +func requireClientError400(t *testing.T, err error, field string) { + t.Helper() + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a client error, got %v", err) + assert.Equal(t, 400, ce.code) + assert.Contains(t, ce.message, field+" must not contain control characters") +} + +// TestCreateAccount_RejectsControlChars drives POST /api/accounts with a +// control character in name or external_id: the request is refused with a +// 400 naming the field and nothing is stored. +func TestCreateAccount_RejectsControlChars(t *testing.T) { + ctx := context.Background() + for label, c := range accountControlChars { + for _, field := range []string{"name", "external_id"} { + t.Run(field+"/"+label, func(t *testing.T) { + mockAuth := new(MockAuthService) + setupAdminAuth(ctx, mockAuth) + store := new(MockConfigStore) + handler := &Handler{auth: mockAuth, config: store} + + name, ext := "Acme", "acme-prod" + if field == "name" { + name = "Acme" + c + "touch PWNED" + } else { + ext = "acme" + c + "touch PWNED" + } + result, err := handler.createAccount(ctx, adminRequest(accountBody(t, name, ext))) + assert.Nil(t, result) + requireClientError400(t, err, field) + store.AssertNotCalled(t, "CreateCloudAccount") + }) + } + } +} + +// TestUpdateAccount_RejectsControlChars is the PUT /api/accounts/:id +// counterpart: the update is refused before the account is loaded or saved. +func TestUpdateAccount_RejectsControlChars(t *testing.T) { + ctx := context.Background() + const id = "11111111-1111-1111-1111-111111111111" + for label, c := range accountControlChars { + for _, field := range []string{"name", "external_id"} { + t.Run(field+"/"+label, func(t *testing.T) { + mockAuth := new(MockAuthService) + setupAdminAuth(ctx, mockAuth) + store := new(MockConfigStore) + handler := &Handler{auth: mockAuth, config: store} + + name, ext := "Acme", "acme-prod" + if field == "name" { + name = "Acme" + c + "touch PWNED" + } else { + ext = "acme" + c + "touch PWNED" + } + req := adminRequest(accountBody(t, name, ext)) + req.RequestContext.HTTP.Method = "PUT" + req.RequestContext.HTTP.Path = "/api/accounts/" + id + result, err := handler.updateAccount(ctx, req, id) + assert.Nil(t, result) + requireClientError400(t, err, field) + store.AssertNotCalled(t, "UpdateCloudAccount") + }) + } + } +} + +// TestCreateAccount_AcceptsPrintableNames guards against over-rejection: +// spaces, punctuation and non-ASCII letters stay valid and are stored as sent. +func TestCreateAccount_AcceptsPrintableNames(t *testing.T) { + ctx := context.Background() + for _, name := range []string{"Acme Production", "Acme (EU) – Zürich #2", "日本 本番"} { + mockAuth := new(MockAuthService) + setupAdminAuth(ctx, mockAuth) + handler := &Handler{auth: mockAuth, config: new(MockConfigStore)} + result, err := handler.createAccount(ctx, adminRequest(accountBody(t, name, "acme-prod"))) + require.NoError(t, err, name) + assert.Equal(t, name, result.(*config.CloudAccount).Name) + } +} + +// TestSubmitRegistration_RejectsControlChars drives the public POST +// /api/register handler. CR and LF in account_name are still stripped (#544), +// but any other control character in account_name, and any control character +// in external_id, is refused, since approval would reject it anyway. +func TestSubmitRegistration_RejectsControlChars(t *testing.T) { + ctx := context.Background() + submit := func(t *testing.T, name, ext string) error { + t.Helper() + store := new(MockConfigStore) + h := &Handler{config: store} + b, err := json.Marshal(map[string]string{ + "provider": "azure", "external_id": ext, "account_name": name, "contact_email": "ops@example.com", + }) + require.NoError(t, err) + _, err = h.submitRegistration(ctx, adminRequest(string(b))) + if err != nil { + store.AssertNotCalled(t, "CreateAccountRegistration") + } + return err + } + for label, c := range accountControlChars { + t.Run("external_id/"+label, func(t *testing.T) { + requireClientError400(t, submit(t, "Acme", "acme"+c+"touch PWNED"), "external_id") + }) + if label == "LF" || label == "CRLF" || label == "CR" { + continue // stripped from account_name, covered below + } + t.Run("account_name/"+label, func(t *testing.T) { + requireClientError400(t, submit(t, "Acme"+c+"touch PWNED", "acme-prod"), "account_name") + }) + } + t.Run("account_name CR/LF still stripped", func(t *testing.T) { + req := RegistrationRequest{ + Provider: "azure", ExternalID: "acme-prod", AccountName: "Acme\r\ntouch PWNED", ContactEmail: "ops@example.com", + } + require.NoError(t, validateRegistrationRequest(&req)) + assert.Equal(t, "Acmetouch PWNED", req.AccountName) + }) +} diff --git a/internal/api/handler_federation.go b/internal/api/handler_federation.go index d04406f4..ead506cd 100644 --- a/internal/api/handler_federation.go +++ b/internal/api/handler_federation.go @@ -12,6 +12,8 @@ import ( "regexp" "strings" "text/template" + "unicode" + "unicode/utf8" "github.com/google/uuid" @@ -101,7 +103,18 @@ func gcpOIDCIssuerURI(source, tenantID string) string { } // renderTemplate renders a named template from the embedded iacfiles.Templates FS. +// It refuses data holding a line break or other control character; see +// checkRenderDataSingleLine. func renderTemplate(tmplPath string, data federationIaCData) (string, error) { + if err := checkRenderDataSingleLine(data); err != nil { + return "", fmt.Errorf("render template %s: %w", tmplPath, err) + } + return executeTemplate(tmplPath, data) +} + +// executeTemplate parses and executes a template with no checks on data. +// Only renderTemplate calls it outside tests. +func executeTemplate(tmplPath string, data federationIaCData) (string, error) { tmplBytes, err := iacfiles.Templates.ReadFile(tmplPath) if err != nil { return "", fmt.Errorf("read template %s: %w", tmplPath, err) @@ -117,6 +130,41 @@ func renderTemplate(tmplPath string, data federationIaCData) (string, error) { return buf.String(), nil } +// checkRenderDataSingleLine fails closed when any string field of data holds +// a control character other than a horizontal tab. The shell and tfvars +// templates print several fields on "#" comment lines, and shellEscape and +// hclEscape escape quoting metacharacters but not line breaks, so a CR or LF +// there would end the comment and run the rest of the value as shell or HCL +// (#970). The generic bundles fill no stored account fields today, and the +// account write paths reject control characters, so this guard only trips on +// data that bypassed them (a record stored before that validation, a future +// caller that fills account fields, or a bad config value); an error is safer +// than a script that runs something unintended. +// A tab is allowed because it cannot end a line, and the session email can +// legitimately hold one in a quoted local part. It walks the struct by +// reflection so a new field cannot be missed. +func checkRenderDataSingleLine(data federationIaCData) error { + v := reflect.ValueOf(data) + for i := 0; i < v.NumField(); i++ { + f := v.Field(i) + if f.Kind() != reflect.String { + continue + } + if at := strings.IndexFunc(f.String(), isRenderControlChar); at >= 0 { + r, _ := utf8.DecodeRuneInString(f.String()[at:]) + return fmt.Errorf("field %s contains control character %U", v.Type().Field(i).Name, r) + } + } + return nil +} + +// isRenderControlChar reports whether r is a control character that +// checkRenderDataSingleLine refuses: every C0 and C1 control and DEL except +// the horizontal tab. +func isRenderControlChar(r rune) bool { + return r != '\t' && unicode.IsControl(r) +} + // getFederationIaC handles GET /api/federation/iac // Query params: target, source, format — all required. // @@ -404,9 +452,8 @@ func shellEscape(s string) string { // hclEscape escapes a string for use inside a double-quoted HCL string. HCL // also starts an interpolation at "${" and a template directive at "%{", so // both are doubled into their literal "$${" and "%%{" forms (#952). Line -// breaks are not handled: the one user-supplied value, the session email, -// cannot hold one (mail.ParseAddress rejects CR and LF), and the rest come -// from server configuration or are derived by the server. +// breaks are not escaped: renderTemplate refuses any value holding one +// (checkRenderDataSingleLine, #970). func hclEscape(s string) string { return strings.NewReplacer(`\`, `\\`, `"`, `\"`, "${", "$${", "%{", "%%{").Replace(s) } diff --git a/internal/api/handler_federation_controlchar_test.go b/internal/api/handler_federation_controlchar_test.go new file mode 100644 index 00000000..e18198e3 --- /dev/null +++ b/internal/api/handler_federation_controlchar_test.go @@ -0,0 +1,300 @@ +package api + +import ( + "context" + "io/fs" + "os" + "os/exec" + "path/filepath" + "reflect" + "regexp" + "strings" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/iacfiles" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Regression tests for #970: a line break in a value the federation IaC +// templates print on a "#" comment line ends the comment, and the rest of the +// value runs as shell (or HCL in a tfvars file) when the admin runs or +// applies the download. + +// controlCharPayloads are the values that must never reach a rendered +// template. Each starts with ordinary text so the control character is not the +// first byte, and the line-break ones carry a marker command that creates a +// file when it runs. +var controlCharPayloads = map[string]string{ + "LF": "x\ntouch PWNED\n#", + "CRLF": "x\r\ntouch PWNED\r\n#", + "CR": "x\rtouch PWNED", + "VT": "x\vy", + "FF": "x\fy", + "NUL": "x\x00y", + "ESC": "x\x1b[2Jy", + "DEL": "x\x7fy", + "NEL": "x\u0085y", +} + +// hclInjection is an LF payload for the tfvars templates: if it escaped its +// comment line it would add a top-level attribute. +const hclInjection = "x\ninjected_by_test = \"1\"\n#" + +var hclInjectedRe = regexp.MustCompile(`(?m)^injected_by_test\s*=`) + +// federationTemplatePaths lists every embedded federation template. +func federationTemplatePaths(t *testing.T) []string { + t.Helper() + paths, err := fs.Glob(iacfiles.Templates, "templates/*.tmpl") + require.NoError(t, err) + require.NotEmpty(t, paths) + return paths +} + +// escapeForTemplate applies the escaping the handler uses for each kind of +// template: shell for scripts, HCL for tfvars, none for the JSON params. +func escapeForTemplate(path string, data federationIaCData) federationIaCData { + switch { + case strings.HasSuffix(path, ".sh.tmpl"): + return shellEscapeData(data) + case strings.HasSuffix(path, ".tfvars.tmpl"): + return hclEscapeData(data) + } + return data +} + +// benignFederationData fills every string field with a realistic value, so +// each {{if}} branch that tests a field is rendered. +func benignFederationData() federationIaCData { + return federationIaCData{ + AccountName: "Acme Production", + AccountExternalID: "acme-prod", + AccountSlug: "acme-prod", + Source: "azure", + SourceAccountID: "000000000000", + OIDCIssuerURL: "https://login.microsoftonline.com/00000000-0000-0000-0000-000000000002/v2.0", + OIDCIssuerHost: "login.microsoftonline.com/00000000-0000-0000-0000-000000000002/v2.0", + OIDCAudience: "api://AzureADTokenExchange", + OIDCSubjectClaim: "00000000-0000-0000-0000-000000000003", + SubscriptionID: "00000000-0000-0000-0000-000000000001", + TenantID: "00000000-0000-0000-0000-000000000002", + ProjectID: "acme-project", + ServiceAccountEmail: "cudly@acme-project.iam.gserviceaccount.com", + OIDCIssuerURI: "https://login.microsoftonline.com/00000000-0000-0000-0000-000000000002/v2.0", + CUDlyAPIURL: "https://cudly.example.com", + ContactEmail: "ops@example.com", + } +} + +// withField returns a copy of data with the named string field set to value. +func withField(t *testing.T, data federationIaCData, field, value string) federationIaCData { + t.Helper() + f := reflect.ValueOf(&data).Elem().FieldByName(field) + require.True(t, f.IsValid() && f.Kind() == reflect.String, "no string field %s", field) + f.SetString(value) + return data +} + +// federationStringFields lists every string field of federationIaCData. +func federationStringFields() []string { + var names []string + typ := reflect.TypeOf(federationIaCData{}) + for i := 0; i < typ.NumField(); i++ { + if typ.Field(i).Type.Kind() == reflect.String { + names = append(names, typ.Field(i).Name) + } + } + return names +} + +// sandboxRun runs script with bash in an empty working directory, with HOME +// pointing there and PATH holding only a symlink to touch, so the marker +// command can run but no cloud CLI, curl or other tool is reachable (the +// script stops at its first missing command). It reports whether bash -n +// accepted the script and the names of any files the run created. +func sandboxRun(t *testing.T, bashPath, script string) (syntaxOK bool, created []string) { + t.Helper() + touchPath, err := exec.LookPath("touch") + require.NoError(t, err) + root := t.TempDir() + bin, work := filepath.Join(root, "bin"), filepath.Join(root, "work") + require.NoError(t, os.Mkdir(bin, 0o700)) + require.NoError(t, os.Mkdir(work, 0o700)) + require.NoError(t, os.Symlink(touchPath, filepath.Join(bin, "touch"))) + scriptPath := filepath.Join(root, "script.sh") + require.NoError(t, os.WriteFile(scriptPath, []byte(script), 0o600)) + + syntaxOK = exec.Command(bashPath, "-n", scriptPath).Run() == nil + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, bashPath, "--noprofile", "--norc", scriptPath) + cmd.Dir = work + cmd.Env = []string{"PATH=" + bin, "HOME=" + work} + _ = cmd.Run() // the script is expected to fail once it needs a real tool + entries, err := os.ReadDir(work) + require.NoError(t, err) + for _, e := range entries { + created = append(created, e.Name()) + } + return syntaxOK, created +} + +// TestRenderTemplate_RefusesControlCharsInEveryField renders every federation +// template with each control-character payload in each string field, with the +// handler's escaping applied, and requires renderTemplate to refuse it and +// name the field. +func TestRenderTemplate_RefusesControlCharsInEveryField(t *testing.T) { + for _, path := range federationTemplatePaths(t) { + for _, field := range federationStringFields() { + for name, payload := range controlCharPayloads { + data := escapeForTemplate(path, withField(t, benignFederationData(), field, payload)) + out, err := renderTemplate(path, data) + if assert.Error(t, err, "%s: %s with %s must be refused", path, field, name) { + assert.Contains(t, err.Error(), "field "+field) + } + assert.Empty(t, out) + } + } + } +} + +// TestRenderTemplate_UnguardedInjectionIsCaughtByOracle shows what the guard +// prevents, and that the sandbox oracle detects it: without the guard +// (executeTemplate), an LF in a comment-line field ends the comment and the +// marker runs, although bash -n still accepts the script. Every such +// combination must be refused by renderTemplate. +func TestRenderTemplate_UnguardedInjectionIsCaughtByOracle(t *testing.T) { + bashPath, err := exec.LookPath("bash") + if err != nil { + t.Skip("bash not available") + } + injected := map[string]bool{} + for _, path := range federationTemplatePaths(t) { + if !strings.HasSuffix(path, ".sh.tmpl") { + continue + } + for _, field := range federationStringFields() { + data := shellEscapeData(withField(t, benignFederationData(), field, controlCharPayloads["LF"])) + out, err := executeTemplate(path, data) + require.NoError(t, err) + syntaxOK, created := sandboxRun(t, bashPath, out) + if len(created) == 0 { + continue + } + injected[filepath.Base(path)+"/"+field] = true + assert.True(t, syntaxOK, "%s/%s: bash -n alone does not catch the injection", path, field) + _, err = renderTemplate(path, data) + assert.Error(t, err, "%s/%s injects a command and must be refused", path, field) + } + } + // The comment-line fields #970 lists must all be detected; if one is not, + // the oracle (not the templates) is broken. + for _, want := range []string{ + "aws-cfn-deploy.sh.tmpl/AccountName", + "aws-cfn-deploy.sh.tmpl/AccountExternalID", + "aws-cfn-deploy.sh.tmpl/Source", + "aws-cross-account-cfn-deploy.sh.tmpl/AccountName", + "aws-cross-account-cli.sh.tmpl/AccountExternalID", + "aws-wif-cli.sh.tmpl/AccountSlug", + "azure-wif-cli.sh.tmpl/AccountName", + "azure-wif-cli.sh.tmpl/CUDlyAPIURL", + } { + assert.True(t, injected[want], "oracle must detect the injection via %s", want) + } +} + +// TestRenderTemplate_UnguardedTfvarsInjection is the HCL counterpart: without +// the guard an LF on a tfvars comment line adds a top-level attribute, and +// renderTemplate must refuse it. +func TestRenderTemplate_UnguardedTfvarsInjection(t *testing.T) { + for _, path := range federationTemplatePaths(t) { + if !strings.HasSuffix(path, ".tfvars.tmpl") { + continue + } + data := hclEscapeData(withField(t, benignFederationData(), "AccountName", hclInjection)) + out, err := executeTemplate(path, data) + require.NoError(t, err) + assert.Regexp(t, hclInjectedRe, out, "%s: oracle must see the injected attribute", path) + _, err = renderTemplate(path, data) + assert.Error(t, err, "%s must refuse an AccountName with a line break", path) + } +} + +// TestRenderTemplate_BenignDataUnchanged checks the guard changes nothing for +// ordinary values, including a tab in the session email (allowed since #992): +// renderTemplate returns exactly what the unguarded render does, and every +// rendered script passes bash -n. +func TestRenderTemplate_BenignDataUnchanged(t *testing.T) { + bashPath, bashErr := exec.LookPath("bash") + for _, email := range []string{"ops@example.com", "\"a\tb\"@example.com"} { + benign := benignFederationData() + benign.ContactEmail = email + for _, path := range federationTemplatePaths(t) { + data := escapeForTemplate(path, benign) + got, err := renderTemplate(path, data) + require.NoError(t, err, path) + want, err := executeTemplate(path, data) + require.NoError(t, err, path) + assert.Equal(t, want, got, "%s must render byte-identically", path) + assert.NotRegexp(t, hclInjectedRe, got) + if bashErr == nil && strings.HasSuffix(path, ".sh.tmpl") { + syntax := exec.Command(bashPath, "-n") + syntax.Stdin = strings.NewReader(got) + assert.NoError(t, syntax.Run(), "%s must pass bash -n", path) + } + } + } +} + +// federationHandlerWithEmail is federationHandler with a chosen session email, +// standing in for a stored user record that predates or bypassed validation. +func federationHandlerWithEmail(email string) *Handler { + mockAuth := new(MockAuthService) + mockAuth.On("ValidateSession", mock.Anything, "admin-token").Return(&Session{ + UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", + Email: email, + }, nil) + mockAuth.grantAdmin() + h := NewHandler(HandlerConfig{ConfigStore: new(MockConfigStore), AuthService: mockAuth}) + mockSourceIdentity(h, defaultTestSourceIdentity()) + return h +} + +// TestGetFederationIaC_FailsClosedOnControlChar drives the real handler for +// every valid target/source/format combination: a benign request succeeds, +// and the same request with a line break in a value that reaches the +// templates (the session email) fails with a server error instead of +// returning a script or bundle. +func TestGetFederationIaC_FailsClosedOnControlChar(t *testing.T) { + clouds := []string{"aws", "azure", "gcp"} + formats := []string{"cli", "bundle", "cfn", "bicep", "arm"} + checked := 0 + for _, source := range clouds { + t.Setenv("CUDLY_SOURCE_CLOUD", source) + for _, target := range clouds { + for _, format := range formats { + params := map[string]string{"target": target, "source": source, "format": format} + _, err := federationHandlerWithEmail("ops@example.com").getFederationIaC( + context.Background(), federationReqWithDomain(params)) + if _, isClient := IsClientError(err); isClient { + continue // combination not offered + } + require.NoError(t, err, "%v benign", params) + checked++ + res, err := federationHandlerWithEmail("ops@example.com\ntouch PWNED").getFederationIaC( + context.Background(), federationReqWithDomain(params)) + assert.Nil(t, res, "%v must not return content", params) + if assert.Error(t, err, "%v must fail closed", params) { + _, isClient := IsClientError(err) + assert.False(t, isClient, "%v: stored data is a server-side fault, not a 4xx", params) + assert.Contains(t, err.Error(), "field ContactEmail") + } + } + } + } + assert.GreaterOrEqual(t, checked, 10, "most combinations must be exercised") +} diff --git a/internal/api/handler_registrations.go b/internal/api/handler_registrations.go index eea4a26e..8e32820c 100644 --- a/internal/api/handler_registrations.go +++ b/internal/api/handler_registrations.go @@ -423,6 +423,14 @@ func validateRegistrationRequest(req *RegistrationRequest) error { if req.AccountName == "" { return NewClientError(400, "account_name is required") } + // Approval re-validates through validateCloudAccountRequest, which rejects + // control characters; reject the rest here so a submission that could + // never be approved fails at submit time instead (#970). + if err := validateNoControlChars( + namedField{"account_name", req.AccountName}, namedField{"external_id", req.ExternalID}, + ); err != nil { + return err + } if req.ContactEmail == "" { return NewClientError(400, "contact_email is required") } diff --git a/internal/api/validation.go b/internal/api/validation.go index 3675add9..dd6575db 100644 --- a/internal/api/validation.go +++ b/internal/api/validation.go @@ -150,6 +150,25 @@ func validateEmailFormat(email string) error { return nil } +// namedField pairs a request field's JSON name with its value, for +// validateNoControlChars. +type namedField struct{ name, value string } + +// validateNoControlChars returns a 400 naming the first field whose value +// holds a control character (C0 such as CR, LF and tab, DEL, or C1). Account +// names and external IDs are single-line identifiers that the federation IaC +// templates print on shell and HCL comment lines, where a line break would +// end the comment and run the rest as code (#970). They are rejected rather +// than stripped, so a stored identifier is never silently changed. +func validateNoControlChars(fields ...namedField) error { + for _, f := range fields { + if strings.IndexFunc(f.value, unicode.IsControl) >= 0 { + return NewClientError(400, f.name+" must not contain control characters such as line breaks or tabs") + } + } + return nil +} + // credentialPayloadSchema defines the per-credential-type required keys and // optional keys. Any payload key outside the union of required+optional is // rejected. Required keys must be non-empty strings. The schemas mirror what