diff --git a/README.md b/README.md index c50ae1ac..06a02140 100644 --- a/README.md +++ b/README.md @@ -257,6 +257,9 @@ can remain divergent until the application resynchronizes it. A rejection matching `ui.ErrJsVarTooLarge` instead cancels the associated request, when present, and is terminal for that connection. +`jawsVar` and `JsCall` paths are application-controlled. The browser rejects +exact `__proto__` components; put user data in JSON values, not paths. + The name may refer to an existing application global. For example, browser code can update that object and send either the complete value or one path: diff --git a/SECURITY.md b/SECURITY.md index d9afde2f..611d5f37 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -247,18 +247,21 @@ The `Set` message type allows clients to modify server-side JsVar state (this is **Source code** (`lib/ui/jsvar.go`, `JsVar.JawsInput`): Client sends `Set\tJid\tpath=jsonvalue` → server unmarshals the value and applies it by path (`PathSetter.JawsSetPath` for a `PathSetter`, `jq.SetChecked` when a generic binding has a `ClientCheck`, or `jq.Set` when it does not) → broadcasts an accepted change. -Tested attack payloads: +Tested attack payloads against the audit fixture: | Payload | Result | |---------|--------| -| `__proto__.polluted=true` | Rejected (invalid Go struct path) | +| `__proto__.polluted=true` | Rejected by the fixture schema | | `constructor.prototype.polluted=true` | Rejected | | `../../../etc/passwd="read"` | Rejected | | `X=999; alert(1)` | Rejected (invalid JSON) | | `X=""` | Accepted as string value; rendered in JS variable, not DOM | | `X={"__proto__":{"polluted":true}}` | Rejected (type mismatch) | -Go's type system prevents prototype pollution — `jq.Set` and `jq.SetChecked` validate paths against actual struct fields and enforce type compatibility. +`jawsVar` rejects exact `__proto__` path components before browser property +access. It does not scan values for that name; own `"__proto__"` members remain +data. The selected Go setter (`jq.Set`, `jq.SetChecked`, or an application +`PathSetter`) independently controls accepted server-side paths. **Trust boundary (application responsibility):** the generic JSON path will set *any* exported field matched by its `json` tag, or by its Go name when the tag @@ -420,7 +423,7 @@ this by blocking inline script execution. | Clickjacking | Header inspection | Protected (DENY + CSP) | | Directory traversal | Gobuster, manual probing | No hidden paths | | Information disclosure | Nikto, manual inspection | No leakage | -| Prototype pollution via JsVar | Manual WebSocket testing | Not vulnerable (Go type safety) | +| `__proto__` JsVar/Call paths | Shipped-runtime regression | Rejected before browser property access | | Command injection via WebSocket | Manual testing of all commands | Whitelist enforced | | Protocol fuzzing | Malformed/oversized messages | Handled gracefully | diff --git a/broadcast.go b/broadcast.go index a95ba58c..982cdc2b 100644 --- a/broadcast.go +++ b/broadcast.go @@ -383,10 +383,12 @@ func jsCallData(jsfunc, jsonstr string) string { // target selects which requests or elements receive the Call message. In each // receiving browser, jsfunc is resolved as a path from window and called with // JSON.parse(jsonstr); the matched element is not passed as this or as an -// argument. A nil target calls each active Request once. A nonzero [key.Key] -// target calls the matching active Request once without requiring a matching DOM -// element; a zero key is ignored. Other targets follow [Jaws.Broadcast]'s tag -// rules. +// argument. jsfunc must be an application-controlled dot path. The browser +// rejects an exact "__proto__" component; put user data in jsonstr, not jsfunc. +// +// A nil target calls each active Request once. A nonzero [key.Key] target calls +// the matching active Request once without requiring a matching DOM element; a +// zero key is ignored. Other targets follow [Jaws.Broadcast]'s tag rules. func (jw *Jaws) JsCall(target any, jsfunc, jsonstr string) { jw.broadcastTo(target, what.Call, jsCallData(jsfunc, jsonstr)) } diff --git a/element.go b/element.go index c68b2864..6d228ca4 100644 --- a/element.go +++ b/element.go @@ -328,11 +328,13 @@ func (elem *Element) SetValue(value string) { // // In the receiving browser, jsfunc is resolved as a path from window and called // with JSON.parse(jsonstr); the Element is not passed as this or as an argument. +// jsfunc must be an application-controlled dot path. The browser rejects an +// exact "__proto__" component; put user data in jsonstr, not jsfunc. // // Call this while the [Element] is rendering or updating, when a send pass is // imminent; a call queued directly from an event handler is only flushed when the -// processing loop is next woken (see [Element.queue]). To call JavaScript for -// every element matching a tag, use [Jaws.JsCall]. +// processing loop is next woken. To call JavaScript for every element matching a +// tag, use [Jaws.JsCall]. func (elem *Element) JsCall(jsfunc, jsonstr string) { elem.queue(what.Call, jsCallData(jsfunc, jsonstr)) } diff --git a/lib/assets/jaws.js b/lib/assets/jaws.js index e30592e2..fabb036f 100644 --- a/lib/assets/jaws.js +++ b/lib/assets/jaws.js @@ -471,6 +471,9 @@ function jawsWarnDirtyNoChange(id, operation) { function jawsVar(name, data, operation) { const keys = name.split('.').filter(key => key !== ""); + if (keys.includes("__proto__")) { + throw "jaws: reserved path component: __proto__"; + } if (keys.length > 0) { let obj = window; const lastkey = keys[keys.length - 1]; diff --git a/lib/assets/js_test.go b/lib/assets/js_test.go index e8c8084e..e790c8c0 100644 --- a/lib/assets/js_test.go +++ b/lib/assets/js_test.go @@ -517,6 +517,220 @@ process.stdout.write(jaws.sent[0] || ""); } } +func TestJawsJS_JsVarRejectsProtoPathComponents(t *testing.T) { + raw := runJawsJSSnippet(t, ` +function FakeSocket() { this.readyState = 1; this.sent = []; } +FakeSocket.prototype.send = function(msg) { this.sent.push(msg); }; +WebSocket = FakeSocket; +jaws = new FakeSocket(); + +let directCalls = 0; +let nestedCalls = 0; +let getterReads = 0; +const forbiddenDirect = function() { directCalls++; }; +window.app = {}; +Object.defineProperty(window.app, "__proto__", { + value: forbiddenDirect, + writable: true, + configurable: true, +}); +window.calls = { + safe: function(value) { window.safeArgument = value; }, +}; +Object.defineProperty(window.calls, "__proto__", { + value: { run: function() { nestedCalls++; } }, + writable: true, + configurable: true, +}); +window.getterTarget = {}; +Object.defineProperty(window.getterTarget, "state", { + get: function() { + getterReads++; + return {}; + }, +}); +window.jawsNames.set("app", ["Jid.9"]); + +const windowPrototype = Object.getPrototypeOf(window); +const appPrototype = Object.getPrototypeOf(window.app); +const forbiddenNested = window.calls.__proto__; +function rejectsProto(run) { + try { + run(); + } catch (err) { + return String(err) === "jaws: reserved path component: __proto__"; + } + return false; +} + +const rejected = [ + function() { return jawsVar("__proto__", { polluted: true }); }, + function() { return jawsVar(".app..__proto__.", { polluted: true }); }, + function() { return jawsVar("app.__proto__"); }, + function() { return jawsVar("app.__proto__", {}, "Set"); }, + function() { return jawsVar("app.__proto__", {}, "Call"); }, + function() { return jawsVar("calls..__proto__..run", {}, "Call"); }, + function() { return jawsVar("getterTarget.state.__proto__", {}, "Call"); }, +].map(rejectsProto); +const rejectedSendCount = jaws.sent.length; + +const opaque = JSON.parse('{"__proto__":{"safe":true}}'); +jawsVar("app.__proto", 1); +jawsVar("app.__Proto__", 2, "Set"); +jawsVar("app.__proto___", 3, "Set"); +jawsVar("app.payload", opaque); +jawsVar("calls.safe", opaque, "Call"); + +process.stdout.write(JSON.stringify({ + rejected: rejected, + rejectedSendCount: rejectedSendCount, + directCalls: directCalls, + nestedCalls: nestedCalls, + getterReads: getterReads, + windowPrototypeUnchanged: Object.getPrototypeOf(window) === windowPrototype, + appPrototypeUnchanged: Object.getPrototypeOf(window.app) === appPrototype, + directMemberUnchanged: window.app.__proto__ === forbiddenDirect, + nestedMemberUnchanged: window.calls.__proto__ === forbiddenNested, + objectPrototypePolluted: Object.hasOwn(Object.prototype, "polluted"), + nearNamesWork: window.app.__proto === 1 && window.app.__Proto__ === 2 && window.app.__proto___ === 3, + safeFrameCount: jaws.sent.length - rejectedSendCount, + payloadOwnProto: Object.hasOwn(window.app.payload, "__proto__"), + payloadPrototypeUnchanged: Object.getPrototypeOf(window.app.payload) === Object.prototype, + argumentOwnProto: Object.hasOwn(window.safeArgument, "__proto__"), +})); +`) + + var got struct { + Rejected []bool `json:"rejected"` + RejectedSendCount int `json:"rejectedSendCount"` + DirectCalls int `json:"directCalls"` + NestedCalls int `json:"nestedCalls"` + GetterReads int `json:"getterReads"` + WindowPrototypeUnchanged bool `json:"windowPrototypeUnchanged"` + AppPrototypeUnchanged bool `json:"appPrototypeUnchanged"` + DirectMemberUnchanged bool `json:"directMemberUnchanged"` + NestedMemberUnchanged bool `json:"nestedMemberUnchanged"` + ObjectPrototypePolluted bool `json:"objectPrototypePolluted"` + NearNamesWork bool `json:"nearNamesWork"` + SafeFrameCount int `json:"safeFrameCount"` + PayloadOwnProto bool `json:"payloadOwnProto"` + PayloadPrototypeUnchanged bool `json:"payloadPrototypeUnchanged"` + ArgumentOwnProto bool `json:"argumentOwnProto"` + } + if err := json.Unmarshal([]byte(raw), &got); err != nil { + t.Fatalf("unexpected JSON output %q: %v", raw, err) + } + for i, rejected := range got.Rejected { + if !rejected { + t.Errorf("reserved-path case %d was not rejected", i) + } + } + if len(got.Rejected) != 7 { + t.Fatalf("ran %d reserved-path cases, want 7", len(got.Rejected)) + } + if got.RejectedSendCount != 0 { + t.Errorf("rejected operations sent %d frames", got.RejectedSendCount) + } + if got.DirectCalls != 0 || got.NestedCalls != 0 || got.GetterReads != 0 { + t.Errorf("rejected paths performed work: direct calls %d, nested calls %d, getter reads %d", got.DirectCalls, got.NestedCalls, got.GetterReads) + } + if !got.WindowPrototypeUnchanged || !got.AppPrototypeUnchanged || !got.DirectMemberUnchanged || !got.NestedMemberUnchanged || got.ObjectPrototypePolluted { + t.Errorf("rejected paths changed an object or prototype: %+v", got) + } + if !got.NearNamesWork { + t.Error("nearby, case-distinct path components should remain usable") + } + if got.SafeFrameCount != 2 { + t.Errorf("safe browser writes sent %d frames, want 2", got.SafeFrameCount) + } + if !got.PayloadOwnProto || !got.PayloadPrototypeUnchanged || !got.ArgumentOwnProto { + t.Errorf("__proto__ data member was not preserved as ordinary JSON data: %+v", got) + } +} + +func TestJawsJS_ProtoPathRejectionDoesNotAbandonBatch(t *testing.T) { + raw := runJawsJSSnippet(t, ` +const elements = { + "Jid.9": { dataset: { jawsname: "state" } }, + "Jid.10": { dataset: {} }, +}; +document.getElementById = function(id) { return elements[id] || null; }; + +window.state = { safe: 0, payload: null }; +let forbiddenCalls = 0; +let safeCalls = 0; +let safeArgument; +window.calls = { + safe: function(value) { + safeCalls++; + safeArgument = value; + }, +}; +Object.defineProperty(window.calls, "__proto__", { + value: { run: function() { forbiddenCalls++; } }, + writable: true, + configurable: true, +}); +const statePrototype = Object.getPrototypeOf(window.state); +const forbiddenMember = window.calls.__proto__; +let errors = 0; +console.error = function() { errors++; }; + +jawsMessage({ data: [ + 'Set\tJid.9\t__proto__={"polluted":true}', + 'Call\t\tcalls..__proto__..run={}', + 'Call\tJid.10\tcalls.__proto__.run={}', + 'Set\tJid.9\tsafe=7', + 'Set\tJid.9\tpayload={"__proto__":{"safe":true}}', + 'Call\t\tcalls.safe={"__proto__":{"safe":true}}', +].join("\n") + "\n" }); + +process.stdout.write(JSON.stringify({ + errors: errors, + forbiddenCalls: forbiddenCalls, + safeCalls: safeCalls, + safeValue: window.state.safe, + stateOwnProto: Object.hasOwn(window.state, "__proto__"), + statePrototypeUnchanged: Object.getPrototypeOf(window.state) === statePrototype, + forbiddenMemberUnchanged: window.calls.__proto__ === forbiddenMember, + objectPrototypePolluted: Object.hasOwn(Object.prototype, "polluted"), + payloadOwnProto: Object.hasOwn(window.state.payload, "__proto__"), + argumentOwnProto: Object.hasOwn(safeArgument, "__proto__"), +})); +`) + + var got struct { + Errors int `json:"errors"` + ForbiddenCalls int `json:"forbiddenCalls"` + SafeCalls int `json:"safeCalls"` + SafeValue int `json:"safeValue"` + StateOwnProto bool `json:"stateOwnProto"` + StatePrototypeUnchanged bool `json:"statePrototypeUnchanged"` + ForbiddenMemberUnchanged bool `json:"forbiddenMemberUnchanged"` + ObjectPrototypePolluted bool `json:"objectPrototypePolluted"` + PayloadOwnProto bool `json:"payloadOwnProto"` + ArgumentOwnProto bool `json:"argumentOwnProto"` + } + if err := json.Unmarshal([]byte(raw), &got); err != nil { + t.Fatalf("unexpected JSON output %q: %v", raw, err) + } + if got.Errors != 3 { + t.Errorf("logged %d rejected orders, want 3", got.Errors) + } + if got.ForbiddenCalls != 0 || got.SafeCalls != 1 { + t.Errorf("function calls = forbidden %d, safe %d; want 0 and 1", got.ForbiddenCalls, got.SafeCalls) + } + if got.SafeValue != 7 { + t.Errorf("later Set value = %d, want 7", got.SafeValue) + } + if got.StateOwnProto || !got.StatePrototypeUnchanged || !got.ForbiddenMemberUnchanged || got.ObjectPrototypePolluted { + t.Errorf("rejected batch orders changed an object or prototype: %+v", got) + } + if !got.PayloadOwnProto || !got.ArgumentOwnProto { + t.Errorf("later JSON values lost their own __proto__ member: %+v", got) + } +} + func TestJawsJS_JsVarRoutingTableIsPrototypeSafe(t *testing.T) { raw := runJawsJSSnippet(t, ` function FakeSocket() { this.readyState = 1; this.sent = []; } diff --git a/lib/ui/errjsvar.go b/lib/ui/errjsvar.go index a6f589fa..7ed6ca4b 100644 --- a/lib/ui/errjsvar.go +++ b/lib/ui/errjsvar.go @@ -36,14 +36,9 @@ var ErrJsVarArgumentType = errors.New("expected jaws.UI or JsVarMaker") // the request cancellation cause retains the detailed check error. var ErrJsVarTooLarge = errors.New("jsvar: JSON size check failed") -// ErrIllegalJsVarPath reports that a JsVar path contained a protocol byte. +// ErrIllegalJsVarPath reports that a JsVar path contains a protocol byte. // -// A JsVar path is written verbatim into a what.Set frame (only the value side is -// JSON-encoded), and the client splits frames on '\n', fields on '\t', and the -// JsVar payload at the first '='. A path carrying those bytes could corrupt the -// frame, inject fabricated orders, or make peer browsers parse the value as -// invalid JSON, so [JsVar.JawsSetPath] rejects it before applying or -// broadcasting. [JsVar.JawsInput] applies the same check to the parsed path for -// incoming browser writes. The raw path is deliberately not echoed in the message -// to avoid log injection. +// [JsVar.JawsSetPath] returns it for a path containing a tab, newline, carriage +// return, or equals sign, without applying or broadcasting the change. +// [JsVar.JawsInput] applies the same check to incoming browser writes. var ErrIllegalJsVarPath = errors.New("jsvar: path contains illegal protocol byte (tab, newline, carriage return or equals)") diff --git a/lib/ui/jsvar.go b/lib/ui/jsvar.go index 114cfbd8..83143c2f 100644 --- a/lib/ui/jsvar.go +++ b/lib/ui/jsvar.go @@ -172,6 +172,10 @@ func JSONSizeCheck[T any](maxBytes int) (check JsVarCheck[T]) { // valid bindings. Do not use a browser-owned property such as window.name, or a // global owned by unrelated code. // +// The variable name and browser-side jawsVar paths must be +// application-controlled. The browser rejects exact "__proto__" path +// components; put user data in values, not names or paths. +// // Multiple bindings may share a name. The name is a single browser window // property, and a browser-initiated write to it is delivered to every live // binding of that name; a removed binding stops receiving writes. This lets a @@ -206,10 +210,9 @@ func JSONSizeCheck[T any](maxBytes int) (check JsVarCheck[T]) { // shared between requests. // // Rendering and write broadcasts invoke JSON marshalers while the locker passed -// to [NewJsVar] is held. This protects values retained by either the generic jq -// setter or a [PathSetter] from concurrent users of the same backing state. -// Custom marshaling callbacks reached in either case, including MarshalJSON and -// MarshalText, must not acquire that locker or re-enter the JsVar. +// to [NewJsVar] is held. Custom marshaling callbacks reached in either case, +// including MarshalJSON and MarshalText, must not acquire that locker or re-enter +// the JsVar. // // A JsVar must not be copied after first use. // @@ -347,7 +350,8 @@ func (jsvar *JsVar[T]) setPath(elem *jaws.Element, jsPath string, value any, cli // is JSON-encoded). The client splits frames on '\n', fields on '\t', and the // JsVar payload at the first '='. Reject any path carrying those protocol // bytes before applying or broadcasting it: they either corrupt the frame or - // make peers parse the value as invalid JSON. + // make peers parse the value as invalid JSON. Return the fixed sentinel without + // the raw path so those bytes cannot reach logs through the error. if strings.ContainsAny(jsPath, "\t\n\r=") { return ErrIllegalJsVarPath } @@ -386,10 +390,9 @@ func (jsvar *JsVar[T]) setPath(elem *jaws.Element, jsPath string, value any, cli // for the synchronization model. // // When a write produces a broadcast, value is marshaled while the application -// locker is held because either the generic jq setter or a [PathSetter] may -// retain aliases into value. Custom marshaling callbacks reachable from value, -// including MarshalJSON and MarshalText, must not acquire that locker or re-enter -// the JsVar. +// locker is held. Custom marshaling callbacks reachable from value, including +// MarshalJSON and MarshalText, must not acquire that locker or re-enter the +// JsVar. func (jsvar *JsVar[T]) JawsSetPath(elem *jaws.Element, jsPath string, value any) (err error) { return jsvar.setPath(elem, jsPath, value, false) } diff --git a/request_test.go b/request_test.go index 6836e418..99fd5205 100644 --- a/request_test.go +++ b/request_test.go @@ -2723,22 +2723,32 @@ func TestRequest_JsCallFunctionPathDoesNotBreakWireFraming(t *testing.T) { tests := []struct { name string jsfunc string + want string }{ { name: "tab in function path", jsfunc: "fn\tpart", + want: `fnpart={"a":1}`, }, { name: "newline in function path", jsfunc: "fn\npart", + want: `fnpart={"a":1}`, }, { name: "carriage return in function path", jsfunc: "fn\rpart", + want: `fnpart={"a":1}`, }, { name: "equals in function path", jsfunc: "fn=part", + want: `fnpart={"a":1}`, + }, + { + name: "reserved component after normalization", + jsfunc: "app.__pro to__.run", + want: `app.__proto__.run={"a":1}`, }, } @@ -2757,8 +2767,8 @@ func TestRequest_JsCallFunctionPathDoesNotBreakWireFraming(t *testing.T) { if strings.Contains(wire, "\r") { t.Fatalf("wire message contains embedded carriage return: %q", wire) } - if msg.Data != `fnpart={"a":1}` { - t.Fatalf("Call payload = %q, want %q", msg.Data, `fnpart={"a":1}`) + if msg.Data != tt.want { + t.Fatalf("Call payload = %q, want %q", msg.Data, tt.want) } }) }