Last Comment Bug 764679 - SecReview: Script Debugger: prefs
: SecReview: Script Debugger: prefs
Status: RESOLVED FIXED
[action item]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 16
Assigned To: Panos Astithas [:past]
:
Mentors:
https://wiki.mozilla.org/Security/Rev...
Depends on:
Blocks: 755385
  Show dependency treegraph
 
Reported: 2012-06-13 18:12 PDT by Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]
Modified: 2012-06-28 04:56 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Working patch (2.25 KB, patch)
2012-06-14 04:41 PDT, Panos Astithas [:past]
no flags Details | Diff | Review
Patch v2 (8.55 KB, patch)
2012-06-19 09:11 PDT, Panos Astithas [:past]
dcamp: review+
21: review+
jgriffin: review+
mark.finkle: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2012-06-13 18:12:53 PDT
SecReview tracking bug
2 prefs - allow debugging via localhost / allow debugging via anywhere. Both disabled by default
Comment 1 Panos Astithas [:past] 2012-06-14 00:09:50 PDT
(In reply to Curtis Koenig [:curtisk] from comment #0)
> SecReview tracking bug
> 2 prefs - allow debugging via localhost / allow debugging via anywhere. Both
> disabled by default

The way I remember this requirement is that we wanted one additional pref to the one added (or rather enforced) in bug 758696: devtools.debugger.remote-enabled, that allows connections through the remote protocol, over the loopback or other interfaces. The new pref, which I will suggest we name devtools.debugger.local-only, will force the debugger server to bind to the loopback interface if true, instead of all the interfaces, as it currently does.
Comment 2 Mark Goodwin [:mgoodwin] 2012-06-14 02:40:12 PDT
(In reply to Panos Astithas [:past] from comment #1)
> The way I remember this requirement is that we wanted one additional pref to
> the one added (or rather enforced) in bug 758696:
> devtools.debugger.remote-enabled, that allows connections through the remote
> protocol, over the loopback or other interfaces.

This is my recollection also.
Comment 3 Panos Astithas [:past] 2012-06-14 04:41:13 PDT
Created attachment 633100 [details] [diff] [review]
Working patch

This patch works as advertised in my tests on a B2G phone. I have used a conservative default of having the listener bound to the loopback interface only. This is in accordance with the default we picked in bug 758696 of 'Cancel', for the users who don't know any better. If you feel that the usability gains from binding to all interfaces by default (debugging over WiFi), trump any security concerns, let me know.
Comment 4 Panos Astithas [:past] 2012-06-18 08:53:29 PDT
Comment on attachment 633100 [details] [diff] [review]
Working patch

Passing the review over to Dave, since Rob is on PTO.
Comment 5 Panos Astithas [:past] 2012-06-19 09:11:21 PDT
Created attachment 634463 [details] [diff] [review]
Patch v2

Dave made a good point that we don't need to provide callers with the ability to limit binding on the loopback interface, since nobody seems to want that.
Comment 6 Panos Astithas [:past] 2012-06-19 12:18:59 PDT
Comment on attachment 634463 [details] [diff] [review]
Patch v2

Fennec, B2G and Marionette patches are one-liner cleanups, but let's keep this by the book.
Comment 7 Panos Astithas [:past] 2012-06-20 06:50:20 PDT
https://hg.mozilla.org/integration/fx-team/rev/576e10abf824
Comment 8 Panos Astithas [:past] 2012-06-21 01:05:03 PDT
https://hg.mozilla.org/mozilla-central/rev/576e10abf824

There is no Target Milestone field in this bug, but this was fixed in Firefox 16.
Comment 9 Panos Astithas [:past] 2012-06-22 03:34:36 PDT
Curtis, can I change the product/component of this bug to Developer Tools: Debugger? I can't seem to be able to nominate the patch for aurora uplift as it is.
Comment 10 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2012-06-22 10:44:13 PDT
Panos - sure, as long as we can keep blocking the sec review bug as well I am fine with that, I use these action items as blockers of the review bug so I can track progress and know when things are done.
Comment 11 Panos Astithas [:past] 2012-06-25 03:46:27 PDT
OK, thanks.
Comment 12 Panos Astithas [:past] 2012-06-25 03:52:28 PDT
Comment on attachment 634463 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature
User impact if declined: No visible user impact, but security review has deemed this an important protection against potential abuse
Testing completed (on m-c, etc.): On m-c and fx-team
Risk to taking this patch (and alternatives if risky): Pretty trivial patch on a developer-only feature
String or UUID changes made by this patch: none
Comment 13 Alex Keybl [:akeybl] 2012-06-26 11:19:07 PDT
Comment on attachment 634463 [details] [diff] [review]
Patch v2

[Triage Comment]
Trivial patch, the security team considers this important, approved for Aurora 15.

Note You need to log in before you can comment on or make changes to this bug.