From cbb1e5feb32beebb8eed76274db70fe137a180ab Mon Sep 17 00:00:00 2001 From: bolkedebruin Date: Thu, 30 Apr 2026 13:33:35 +0200 Subject: [PATCH] Strip ASCII control bytes from rendered RDP string fields (#183) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The .rdp file format is line-delimited (key:type:value\r\n), and an unfiltered \r, \n, or NUL inside a string field is reinterpreted by RDP clients as a directive boundary. A username flowing in from any of OIDC, header auth, NTLM, or the URL-override path could therefore inject arbitrary additional directives — e.g. `alternate shell:s:cmd.exe` — and when RDP signing is enabled the malicious payload is signed as authentic, producing a one-click client-side RCE on every user who opens the file. Strip bytes < 0x20 and 0x7F at the renderer chokepoint (addStructToString), so every source path — caller, template file, ApplyOverrides, anything future — passes through the same filter. Legitimate values (usernames, base64url tokens, hostnames) contain no such bytes, so the filter is a no-op for normal input. Stripping is logged so operators can spot rejected input. Adds TestStringFieldBoundaryHygiene covering CRLF in username, domain and full address; bare LF in alternate shell; and embedded NUL. Co-authored-by: Bolke de Bruin --- cmd/rdpgw/rdp/rdp.go | 31 ++++++++- cmd/rdpgw/rdp/rdp_test.go | 132 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 162 insertions(+), 1 deletion(-) diff --git a/cmd/rdpgw/rdp/rdp.go b/cmd/rdpgw/rdp/rdp.go index ce7872b..b21ea90 100644 --- a/cmd/rdpgw/rdp/rdp.go +++ b/cmd/rdpgw/rdp/rdp.go @@ -232,7 +232,7 @@ func (rb *Builder) addStructToString(st interface{}, sb *strings.Builder) { switch f.Kind() { case reflect.String: sb.WriteString("s:") - sb.WriteString(f.Value().(string)) + sb.WriteString(sanitizeRdpValue(f.Name(), f.Value().(string))) case reflect.Int: sb.WriteString("i:") fmt.Fprintf(sb, "%d", f.Value()) @@ -309,6 +309,35 @@ func initStruct(st interface{}) { } } +// sanitizeRdpValue strips ASCII control bytes (< 0x20 and 0x7F) from a string +// before it is written into an .rdp file. Any control byte in a value can be +// reinterpreted by RDP clients as a directive boundary, so an unfiltered \r, +// \n or NUL inside e.g. a username produces extra signed directives that the +// caller never set. +func sanitizeRdpValue(field, v string) string { + clean := true + for i := 0; i < len(v); i++ { + if c := v[i]; c < 0x20 || c == 0x7f { + clean = false + break + } + } + if clean { + return v + } + var b strings.Builder + b.Grow(len(v)) + for i := 0; i < len(v); i++ { + c := v[i] + if c < 0x20 || c == 0x7f { + continue + } + b.WriteByte(c) + } + log.Printf("rdp: stripped control bytes from field %s", field) + return b.String() +} + func setVariable(f *structs.Field, v string) error { switch f.Kind() { case reflect.String: diff --git a/cmd/rdpgw/rdp/rdp_test.go b/cmd/rdpgw/rdp/rdp_test.go index 5c94b58..0c3c1e1 100644 --- a/cmd/rdpgw/rdp/rdp_test.go +++ b/cmd/rdpgw/rdp/rdp_test.go @@ -2,6 +2,7 @@ package rdp import ( "log" + "sort" "strings" "testing" ) @@ -205,3 +206,134 @@ func TestApplyOverrides_NilValuesNoOp(t *testing.T) { t.Errorf("ApplyOverrides(nil) should be a no-op, got: %v", err) } } + +// TestStringFieldBoundaryHygiene asserts that CR/LF/NUL inside a string +// field cannot introduce additional rendered directives. The .rdp format is +// line-delimited, so an unfiltered \r\n turns the value into an extra +// `key:type:value` line that the caller never set. +func TestStringFieldBoundaryHygiene(t *testing.T) { + cases := []struct { + name string + clean func(*Builder) + dirty func(*Builder) + }{ + { + name: "username with CRLF", + clean: func(b *Builder) { b.Settings.Username = "alice" }, + dirty: func(b *Builder) { + b.Settings.Username = "alice\r\nalternate shell:s:notepad.exe" + }, + }, + { + name: "domain with CRLF", + clean: func(b *Builder) { b.Settings.Domain = "ad" }, + dirty: func(b *Builder) { + b.Settings.Domain = "ad\r\nalternate shell:s:notepad.exe" + }, + }, + { + name: "full address with CRLF", + clean: func(b *Builder) { b.Settings.FullAddress = "host:3389" }, + dirty: func(b *Builder) { + b.Settings.FullAddress = "host:3389\r\nalternate shell:s:notepad.exe" + }, + }, + { + name: "alternate shell with bare LF", + clean: func(b *Builder) { b.Settings.AlternateShell = "explorer.exe" }, + dirty: func(b *Builder) { + b.Settings.AlternateShell = "explorer.exe\nshell working directory:s:c:\\" + }, + }, + { + name: "username with embedded NUL", + clean: func(b *Builder) { b.Settings.Username = "alicebob" }, + dirty: func(b *Builder) { b.Settings.Username = "alice\x00bob" }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + cleanB := NewBuilder() + dirtyB := NewBuilder() + tc.clean(cleanB) + tc.dirty(dirtyB) + + cleanKeys := renderedKeys(cleanB.String()) + dirtyKeys := renderedKeys(dirtyB.String()) + + if extra := keysExtra(dirtyKeys, cleanKeys); len(extra) > 0 { + t.Errorf("dirty value introduced extra rendered keys: %v\n"+ + "clean keys: %v\ndirty keys: %v\nfull dirty output:\n%s", + extra, cleanKeys, dirtyKeys, dirtyB.String()) + } + + out := dirtyB.String() + if strings.ContainsRune(out, 0x00) { + t.Errorf("output contains NUL: %q", out) + } + + // Bare CR or LF is a line break for many RDP parsers (mstsc + // included) even when the renderer uses CRLF. + for i := 0; i < len(out); i++ { + switch out[i] { + case '\r': + if i+1 >= len(out) || out[i+1] != '\n' { + t.Errorf("bare CR at byte %d: %q", i, out) + } + case '\n': + if i == 0 || out[i-1] != '\r' { + t.Errorf("bare LF at byte %d: %q", i, out) + } + } + } + + for i, line := range strings.Split(strings.TrimRight(out, CRLF), CRLF) { + if line == "" { + continue + } + parts := strings.SplitN(line, ":", 3) + if len(parts) != 3 { + t.Errorf("line %d is not of form key:type:value: %q", i, line) + continue + } + switch parts[1] { + case "s", "i", "b": + default: + t.Errorf("line %d has unknown type tag %q: %q", i, parts[1], line) + } + } + }) + } +} + +func renderedKeys(out string) []string { + var keys []string + for _, line := range strings.Split(strings.TrimRight(out, CRLF), CRLF) { + if line == "" { + continue + } + if i := strings.IndexByte(line, ':'); i > 0 { + keys = append(keys, line[:i]) + } + } + sort.Strings(keys) + return keys +} + +// keysExtra is a multiset diff: each occurrence in want consumes one in have. +func keysExtra(want, have []string) []string { + counts := make(map[string]int, len(have)) + for _, k := range have { + counts[k]++ + } + var extra []string + for _, k := range want { + if counts[k] > 0 { + counts[k]-- + continue + } + extra = append(extra, k) + } + return extra +}