Closed Bug 755412 Opened 13 years ago Closed 11 years ago

Debugging protocol server should drop connection if packet framing is bad

Categories

(DevTools :: Debugger, defect, P3)

Product:
DevTools
Component:
Debugger
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 26

People

(Reporter: jimb, Assigned: mhordecki)

Details

Attachments

(1 file, 3 obsolete files)

0001-Bug-755412-Debugging-protocol-server-should-drop-con.patch
4.18 KB, patch
jimb
: review+
Details | Diff | Splinter Review
The behavior of DebuggerTransport when presented with incorrectly-framed packets raises the possibility that, if a developer enables the debugger on a known TCP port, a web page could spoof a debugging protocol conversation with a crafted XHR to localhost. To make this more difficult, DebuggerTransport should drop the connection when it receives an improperly framed packet. The debugging protocol as implemented uses a framing layer not documented on https://wiki.mozilla.org/Remote_Debugging_Protocol. The byte stream is broken into a series of packets of the form "<length>:<body>", where <length> is parsed by the JavaScript parseInt function, and gives the length of <body> in bytes. <body> is then passed to an nsIScriptableUnicodeConverter's ConvertToUnicode method, and then to JSON.parse. At present, DebuggerTransport doesn't validate <length> very carefully, so one could imagine attacks in which an XHR's headers got skipped and the body began to be interpreted as debugging requests. While I don't know of a specific exploit right now, there's no reason for this flexibility, so we might as well make life a little harder for folks. Note that the debugging server is only enabled when a preference is set, so this doesn't affect anyone who hasn't asked for this sort of trouble. Furthermore, the protocol isn't designed to be any sort of security boundary; any attacker with the ability to make their own TCP connections could simply connect and start talking, and gain full control of the browser through the chrome debugger. There's no authentication, and no confidentiality or integrity checking whatsoever. This bug only helps protect developers who have opted in against a specific kind of attacker.
Priority: -- → P3
Assignee: nobody → mhordecki
Attached patch Proposed fix. (obsolete) — — Splinter Review
Attachment #776780 - Attachment is patch: true
Attachment #776780 - Attachment mime type: text/x-patch → text/plain
Attachment #776780 - Flags: review?(past)
Comment on attachment 776780 [details] [diff] [review] Proposed fix. Review of attachment 776780 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/server/tests/unit/test_dbgsocket.js @@ +16,4 @@ > > add_test(test_socket_conn); > add_test(test_socket_shutdown); > + add_test(test_socket_conn_drops_after_invalid_packet); One preliminary comment I have is that I'd like this to be a separate test, please. test_dbgsocket.js is quite large as it is and we have a long-standing intermittent failure that I'm still investigating, so I wouldn't want to muddy the waters any further, if possible.
Attached patch Patch v2 (obsolete) — — Splinter Review
Attachment #776780 - Attachment is obsolete: true
Attachment #776780 - Flags: review?(past)
Attachment #777486 - Flags: review?(past)
Comment on attachment 777486 [details] [diff] [review] Patch v2 Review of attachment 777486 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine to me, I only have a few nits, but I'd like Jim to take a quick look to see if there were any more validations in his mind. ::: toolkit/devtools/server/tests/unit/test_dbgsocket_connection_drop.js @@ +1,4 @@ > +/* Any copyright is dedicated to the Public Domain. > + http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +Cu.import("resource://gre/modules/devtools/dbg-server.jsm"); Please add a short comment describing the purpose of this test, like we do in the rest of the tests. @@ +17,5 @@ > +} > + > +function test_socket_conn_drops_after_invalid_packet() { > + // Bug 755412 - checks if the server drops the connection on an improperly > + // framed packet, i.e. when the length header is invalid. Oh, might just move this comment to the top of the file. @@ +37,5 @@ > + transport._outgoing += payload; > + transport._flushOutgoing(); > + }, > + onClosed: function(aStatus) { > + do_test_finished(); How about a do_check_true(true) here to actually have a positive indication of the test's success? ::: toolkit/devtools/server/transport.js @@ +156,5 @@ > > let count = parseInt(this._incoming.substring(0, sep)); > + if (!(Number.isFinite(count) && count > 0)) { > + // This is an improperly framed packet - this connection > + // should be dropped - see bug 755412. Nit: I find "!a || b" to be more readable than "!(a && c)". Also, you don't need to reference the bug number in the code, it can stand fine on its own.
Attachment #777486 - Flags: review?(past)
Attachment #777486 - Flags: review?(jimb)
Attachment #777486 - Flags: review+
Comment on attachment 777486 [details] [diff] [review] Patch v2 Review of attachment 777486 [details] [diff] [review]: ----------------------------------------------------------------- We should do more in there; we want to have proven that the packet has exactly the form specified, and using parseInt to do the validation isn't good enough; see comments. ::: toolkit/devtools/server/transport.js @@ +157,5 @@ > let count = parseInt(this._incoming.substring(0, sep)); > + if (!(Number.isFinite(count) && count > 0)) { > + // This is an improperly framed packet - this connection > + // should be dropped - see bug 755412. > + this.close(); It would be nice to also drop the connection if: 1) this._incoming still has no colon even when it's gotten too large for any reasonable byte count (more than 50 chars, say) 2) this._incoming doesn't match the regexp /^[0-9]+:/. parseInt ignores trailing garbage: parseInt("1aberwatchet") is 1; so without the regexp match we're tolerating more than we intend. This check would also cover the 'count < 0' and 'NaN' cases. 3) the byte count indicates an absurdly large packet (>10MiB, say). This would cover the +Inf case. We do have some tests that make big packets, but I think they only go up to 4MiB; if not, the test can be changed.
Attachment #777486 - Flags: review?(jimb)
Attached patch Patch v3 (obsolete) — — Splinter Review
I've incorporated suggestions from both you and past. What do you think now?
Attachment #777486 - Attachment is obsolete: true
Attachment #789872 - Flags: review?(jimb)
Comment on attachment 789872 [details] [diff] [review] Patch v3 Review of attachment 789872 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/devtools/server/transport.js @@ +160,5 @@ > + } > + > + let count = this._incoming.substring(0, sep); > + // Check for a positive number with no garbage afterwards. > + if (!/^[0-9]+/.exec(count)) { Don't you need a $ here? That'll still pass digits followed by garbage: js> /^[0-9]+/.exec("1aberwatchet") ["1"] js>
Attachment #789872 - Flags: review?(jimb)
Comment on attachment 789872 [details] [diff] [review] Patch v3 Review of attachment 789872 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, with earlier comments addressed.
Attachment #789872 - Flags: review+
Comment on attachment 789872 [details] [diff] [review] Patch v3 Review of attachment 789872 [details] [diff] [review]: ----------------------------------------------------------------- Actually, wait... I'm sorry. I think this needs a bit more care. As I pointed out in comment 7, the regexp isn't tight enough. But the tests cover that case (digits followed by garbage), and don't fail. So the tests are also not tight enough - they should have caught the bug in comment 7. I've cleared review; let's make sure everything's lined up nicely.
Attachment #789872 - Flags: review+
Yup, that was an oversight on my part. Tests were passing due to the fact that +count produced null value when count had extra fluff at the end, and due to the logic of _processIncoming and onDataAvailable the connection was discarded later on anyway. Hence, after correcting the mistake, I think the tests are tight enough for now. I've modified the second test slightly to be more specific about what we're actually testing there (with "123asd" it could also theoretically wait for more data).
Attachment #789872 - Attachment is obsolete: true
Attachment #790958 - Flags: review?(jimb)
Comment on attachment 790958 [details] [diff] [review] 0001-Bug-755412-Debugging-protocol-server-should-drop-con.patch Review of attachment 790958 [details] [diff] [review]: ----------------------------------------------------------------- Okay, looks good.
Attachment #790958 - Flags: review?(jimb) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/7e8931d11672
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/7e8931d11672
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: