764679 - SecReview: Script Debugger: prefs

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

SecReview tracking bug
2 prefs - allow debugging via localhost / allow debugging via anywhere. Both disabled by default

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

(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]]

(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 (he/him) [:past] (please ni?)]

Attachment: 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 (he/him) [:past] (please ni?)]

Comment on attachment 633100 [details] [diff] [review]
Working patch

Passing the review over to Dave, since Rob is on PTO.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: 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 (he/him) [:past] (please ni?)]

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 (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/576e10abf824

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

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 (he/him) [:past] (please ni?)]

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]]]

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 (he/him) [:past] (please ni?)]

OK, thanks.

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

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]]

Comment on attachment 634463 [details] [diff] [review]
Patch v2

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

Comment 14

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e6b0d54297e6