Closed Bug 1032128 Opened 10 years ago Closed 7 years ago

Add 'allow always' option for known incoming DevTools connections to get security out of the way while still being secure.

Categories

(Firefox OS Graveyard :: Developer Tools, defect)

Product:
Firefox OS Graveyard
Component:
Developer Tools
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: jujjyl, Unassigned)

References

Details

Attachments

(3 files, 4 obsolete files)

MozReview Request: Bug 1032128 - Persist always allowed cert clients. r=past
39 bytes, text/x-review-board-request
past
: review+
janx
: review+
bgrins
: review+
Details
MozReview Request: Bug 1032128 - Receive extended authResult from Gaia. r=janx
39 bytes, text/x-review-board-request
janx
: review+
past
: review+
bgrins
: review+
Details
MozReview Request: Bug 1032128 - Update asyncStorage test to use Gecko quotes. r=bgrins
39 bytes, text/x-review-board-request
bgrins
: review+
janx
: review+
past
: review+
Details
After discussing the FFOS DevTools connection features in email, I am in the conclusion that the current security model is not particularly safe, and the lock screen feature does not improve security. To recap my understanding, the current security features are: 1. No DevTools connections are allowed unless "Developer Options: ADB and DevTools" is enabled. 2. A popup "Accept/Deny" appears on each DevTools connection attempt and the user has to tap each time to acknowledge it. 3. No DevTools connection attempts are processed if the device is locked. The problem in this model is that features 2 and 3 obstruct good user experience, and block support for user automation. To be able to develop, people will set devtools.debugger.prompt-connection to false and disable the phone lock screen. This effectively disables security measures 2 and 3. There are three groups of users: A. People who don't use their FFOS phones for developing ever. B. People who who both develop with their phones, but also use it as their regular personal phone. C. People who don't use the phone for personal uses, but it sits in the dev lab only for development purposes. Feature 1 keeps the group A safe, so we don't really need to consider those. If there is group A user who accidentally enabled that flag, there is a second layer of barrier by feature 2 that keeps the group A still safe. The problem for group B is that they will _never_ restore the device screen lock and devtools.debugger.prompt-connection=true after their dev session is over. The problem for group C is that they need to have these always disabled. Both group B and C will then be vulnerable by incoming unauthorized DevTools connections. Saying that "it was your fault, of course you are insecure since you disabled 2 and 3" is like saying "of course security is supposed to come in your way and make your experience clumsy" and also "user automation is not supported by design". To fix these problems, the security model should instead be: 1'. No DevTools connections are allowed unless "Developer Options: ADB and DevTools" is enabled. 2'. A popup "Accept/Deny" appears on each DevTools connection attempt. The dialog shows the origin of the connection (MAC/IP/wlan-or-usb/etc.) so that the user can make an informed decision of the origin. The dialog has a "[ ] Remember the choice for _this_ origin." that the user can check to always enable or disable a given origin. This is reported at https://bugzilla.mozilla.org/show_bug.cgi?id=1022698 3'. Incoming connection attempts from accepted origins will always be approved, independent of whether the device is locked or not. Reported at https://bugzilla.mozilla.org/show_bug.cgi?id=1009521 4'. Incoming connection attempts from unknown origins will be silently rejected if the device is locked. If the device is not locked, the popup dialog will be shown. 5'. Incoming connection attempts from rejected origins will always be silently rejected without showing a dialog. 6'. Incoming connection attempts from unknown origins will be silently rejected if the confirmation popup dialog is already being shown by some other connection attempt, and is still unacknowledged, i.e. the popups shall not stack or queue up. Reported at https://bugzilla.mozilla.org/show_bug.cgi?id=1022692 7'. A Developer options menu adds an item "Clear all known DevTools origins" that empties the list of remembered incoming connections. With this model, the people from group A will still be doubly safe with features 1' and 2'. Additionally, with 4', group B and C will not need to set devtools.debugger.prompt-connection=false or disable the device lock screen, so they are still safe during and after their dev session. The other items are natural follow-ups to make the feature robust.
In general I'm in favor of such a tweak, but I will CC our security folks who have probably thought more about this. I can't access bug 1022698, so I'd appreciate it if someone could CC me there. As a general comment, group C doesn't stand to loose much from a potential compromise (they don't store valuable information on the device, perhaps with the exception of confidential source code), so I think the current system serves them well. On the other hand, as someone who falls squarely into group B, I have to say I don't find the experience all that bad. I don't disable the prompt on my device, but I occasionally do disable the lock screen. It's virtually impossible to forget that I did that, as I notice it as soon as I pick up the phone. I can see how others can be less patient however, so I support this plan.
The model you've describe above seems reasonable to me. It's quite similar to the way ADB access to (recent) Android devices works. Incidentally, this is essentially the model I plan to have for the upcoming WiFi debugging feature I am working on. It may make sense to first add it for WiFi debugging, and assuming it seems logical there, expand it to other connection types. (Just a thought, maybe doing all at once is better.) Anyway, I do think we'll be moving towards the workflow you've described here one way or another.
Depends on: wifi-rdp
Whiteboard: [good first bug][lang=js]
Hi Dave, I am a newbie and want to start working on this bug. Can you please give some pointers to start with. Regards, SuryaPavan
Hi! I'm interested to fix one bug, but I'm new no bugzilla. I want to work in this bug and try to fix it. Could someone mentor me?
Obviously, I need to improve my filters (I missed SuryaPavan's comment). I put myself down as a mentor, but after re-reading the description I'm not sure I actually know enough. In any case, you're going to need to be able to build FirefoxOS either on one of our supported devices, or on the emulator. You'll need at least 50Gb of free drive space. You'll probably need a linux build machine or VM. I think you might be able to build using a Mac, but I'm not a Mac person, so I'm not sure on that front. This page: https://developer.mozilla.org/en-US/Firefox_OS/Firefox_OS_build_prerequisites has the steps required to build FixrefoxOS. You'll need to pull down approx 30 Gb of repositories, so it may take several hours depending on your internet connection.
I think this bug would be quite hard to make progress on as a mentored bug. It involves an area undergoing active change in DevTools (with WiFi support being added). Also, it might be hard to capture an suggested implementation plan without documenting a lot of context, and even then it could be overwhelming for a new contributor. For these reasons, I'm clearing the good first bug and mentored status.
Mentor: dhylands
Whiteboard: [good first bug][lang=js]
Product: Firefox → Firefox OS
Version: Trunk → unspecified
Attached file MozReview Request: bz://1032128/jryans (obsolete) —
/r/5087 - Bug 1032128 - Update asyncStorage test to use Gecko quotes. r=bgrins /r/5089 - Bug 1032128 - Persist always allowed cert clients. r=past /r/5091 - Bug 1032128 - Receive extended authResult from Gaia. r=janx Pull down these commits: hg pull review -r c3e6f19cc18675ae117d6e8985fdf229aa3a1b9c
Attachment #8575673 - Flags: review?(past)
Attachment #8575673 - Flags: review?(janx)
Attachment #8575673 - Flags: review?(bgrinstead)
This current work only adds "always allow" for WiFi connections, which use certs to authenticate. We currently don't have a way to meaningfully authenticate a USB client, but that work could be done later on.
Comment on attachment 8575673 [details] MozReview Request: bz://1032128/jryans https://reviewboard.mozilla.org/r/5085/#review4143 Ship It!
Attachment #8575673 - Flags: review?(past) → review+
Comment on attachment 8575673 [details] MozReview Request: bz://1032128/jryans r+ for my part. Not sure I like review board...
Attachment #8575673 - Flags: review?(janx) → review+
Comment on attachment 8575673 [details] MozReview Request: bz://1032128/jryans I've marked ship it (twice now) in Review Board on this attachment, but my r? flag isn't being updated to an r+.. Not sure if I'm supposed to do that part in bugzilla, but that's what I'm doing
Attachment #8575673 - Flags: review?(bgrinstead) → review+
Normally you just click "Ship It!" on the parent review and that gets recorded as an r+ in bugzilla.
Clicking "Ship It!" didn't set the r+ for me either, it just added a comment on reviewboard.
Comment on attachment 8575676 [details] [review] [gaia] jryans:always-allow > mozilla-b2g:master Thanks for the new prompt! And sorry to be overly nitty, but I wanted a few things addressed before giving r+. In general, I feel like such a prompt is not ideal: 1) It's missing a fingerprint of the cert/device (on Android when you decide to remember a device you can see its fingerprint). 2) Maybe a "Remember my choice for this device" check box (like on permission prompts) would be better than a separate "Allow Always" option. 3) And maybe we'll eventually want to move our prompts to be web components like <gaia-confirm> or <gaia-menu>. But those are probably follow-ups. Regarding this patch more precisely, I added a few nits on GitHub. Also: 1) The dialog text should be somewhere at the top, not sitting on the options. 2) "OK" looks good on a button that's half the screen's width, but since the button becomes larger and the other option is called "Always Allow", please rename "OK" to "Allow". 3) "Always Allow" might be clearer as "Allow Always". Your call. 4) Maybe it's worth adding a "Deny" option. It's redundant to "Cancel", but can be reassuring.
Attachment #8575676 - Flags: review?(janx) → review-
Assignee: nobody → jryans
Status: NEW → ASSIGNED
(In reply to Jan Keromnes [:janx] from comment #18) > Comment on attachment 8575676 [details] [review] > [gaia] jryans:always-allow > mozilla-b2g:master > > Thanks for the new prompt! And sorry to be overly nitty, but I wanted a few > things addressed before giving r+. > > In general, I feel like such a prompt is not ideal: > 1) It's missing a fingerprint of the cert/device (on Android when you decide > to remember a device you can see its fingerprint). Okay, I've added the client ID. Let me know if there's a better way to present it than just a jumble of hex chars. > 2) Maybe a "Remember my choice for this device" check box (like on > permission prompts) would be better than a separate "Allow Always" option. I think I'll avoid this path, just since we don't happen to persist deny for now. > 3) And maybe we'll eventually want to move our prompts to be web components > like <gaia-confirm> or <gaia-menu>. Yes, maybe this is good down the road. > But those are probably follow-ups. > > Regarding this patch more precisely, I added a few nits on GitHub. Also: > 1) The dialog text should be somewhere at the top, not sitting on the > options. It's now more at the top, purely by virtue of there being more buttons. > 2) "OK" looks good on a button that's half the screen's width, but since the > button becomes larger and the other option is called "Always Allow", please > rename "OK" to "Allow". It now says Allow. > 3) "Always Allow" might be clearer as "Allow Always". Your call. I think the current choice is more natural. > 4) Maybe it's worth adding a "Deny" option. It's redundant to "Cancel", but > can be reassuring. Okay, added.
Comment on attachment 8575676 [details] [review] [gaia] jryans:always-allow > mozilla-b2g:master r+ but please address all the nits, then ensure the tests pass before checking in. Next time, please run `bin/gaia-test` and make sure all tests pass before asking for review.
Attachment #8575676 - Flags: review?(janx) → review+
Attached image wifi-prompt.jpg (obsolete) —
With all nits addressed, the prompt should look much nicer.
(In reply to Jan Keromnes [:janx] from comment #23) > Next time, please run `bin/gaia-test` and make sure all tests pass before > asking for review. I typically use try as the test runner, especially in an unfamiliar area like this, where I have no idea what I may have broken, and running the tests locally seems like a waste of time since there are so many. In most cases, I feel it's fine to review a patch even if the tests are broken in some trivial way, with the obvious understanding that they must fixed before landing. I have certainly given "r+ assuming the tests are fixed" numerous times when it's some simple issue. But some reviewers choose to be more strict about tests, so whatever makes sense to you. Anyway, I'm not sure I'll necessarily remember to only ask review after try passes, as it breaks the async workflow of working on many things at once.
To continue our discussion from IRC, the value of showing the cert fingerprint to the user is fairly limited in my opinion. For the average developer just trying to get some work done, it could waste time if the mere presence of the fingerprint suggests they MUST visually verify it to ensure security. This is wasteful because the next step of scanning the QR code includes the cert fingerprint which the phone requires a match from anyway, so it does the work for you. For the very security conscious user, they may feel better if they can see the fingerprint and do a comparison themselves. So from a UX perspective, I think it's distracting to have the fingerprint. However, as Jan said on IRC, if it's not there, the user does not know that authentication is based on something unique to the computer (the cert) that can be trusted in the future. Here are some possible options: A. Always show the fingerprint, as in Jan's screenshot from comment 24 B. Never show the fingerprint, but state in the dialog that a cert is used and verified by the QR scan on the next screen C. Hide fingerprint behind some kind of "more info" toggle that would reveal fingerprint if tapped, state in the dialog that a cert is used and verified by the QR scan on the next screen Jan and Panos, you've each reviewed pieces of this flow so far, any thoughts on what is best? Perhaps another option not listed above?
Flags: needinfo?(past)
Flags: needinfo?(janx)
I don't see the value of displaying the fingerprint, if the user isn't expected to make a visual comparison himself. I'm not sure what is Jan's argument for showing it, but the "feels better" argument that Ryan states in comment 26 sounds like security theater to me. The dialog already displays the remote endpoint address, so the user can verify that it is an expected debugging connection, not some random computer on the internet trying to get at his device. So option B would be my preferred way forward here and I wouldn't even mention the certificate verification in the dialog. Do we mention that certs are verified and cryptographic hashes calculated in secure web connections in the UI? That's just what the system is supposed to do.
Flags: needinfo?(past)
Is it possible to show both the fingerprint and the IP? Why would you say "because I wouldn't bother verifying the fingerprint, let's not display it so that nobody else can either."?
It is of course possible, as Jan's screenshot from comment 24 proves, but to what end? Should we also display the MAC address in case someone would bother to verify that too? Computers are supposed to serve us and in the UI, less is better.
My thoughts on this: A. Showing the SHA-256 certificate visually shows what's being saved when you say "Always Allow". It's common practice, as you can see on Android's remote debugging prompt: http://i.imgur.com/UrlAtYU.png I tend to agree with Jukka, showing it doesn't hurt because people who don't care won't look at it. B. Never showing the certificate leaves you with a prompt saying "Allow remote debugging connection from 10.243.36.190:52538? [Always Allow] [Allow] [Deny]". You don't know what "Always Allow" will do (always allow every remote connection ever? always allow that particular IP address?). Showing the certificate, even if it's a gimmick, makes it clear what's being saved for the curious. C. A "more info" toggle sounds like a good tradeoff. No visible information that makes you believe you're supposed to compare anything, but if you're uneasy about that "Always Allow" options, showing a certificate under "more info" will show what's being saved. I'm fine with A and C, but I don't like B because currently it would be confusing. (Then again, maybe a checkbox saying something like "Always allow this computer" would make things clearer if you really don't want to show the certificate).
Flags: needinfo?(janx)
The point that Jan brings up with B above is an important one, and I agree that saying "Allow remote debugging connection from 10.243.36.190:52538?" would be bad, if the memorization is not keyed on IP address (which I think it should not be).
Are we all discussing the same option B? In option B, I stated: B. Never show the fingerprint, but state in the dialog that a cert is used and verified by the QR scan on the next screen So, we would show something like: Allow remote debugging connection from 10.243.36.190:52538? The other endpoint's certificate will be authenticated by scanning a QR code on the next screen. [Always Allow] [Allow] [Deny] We can bikeshed the exact text, but doesn't something like this make it clear that a cert is used to authenticate, not just the IP? (In reply to Jukka Jylänki from comment #31) > The point that Jan brings up with B above is an important one, and I agree > that saying "Allow remote debugging connection from 10.243.36.190:52538?" > would be bad, if the memorization is not keyed on IP address (which I think > it should not be). Memorization (always allow) is only available as a choice when a cert is used, and is not based on the IP address. If a cert is not used, then "always allow" is not presented as an option. I believe the actual security is sound, but we just need to work out how it's presented in this dialog. (In reply to Jukka Jylänki from comment #28) > Is it possible to show both the fingerprint and the IP? Why would you say > "because I wouldn't bother verifying the fingerprint, let's not display it > so that nobody else can either."? It's not about "I won't bother", but rather that the next (required!) screen involves scanning a QR code which verifies the cert fingerprint for you anyway. So, at the very least, we should probably state that this will take place so the user can make an informed decision. This suggests either option B (which makes this statement) or C (same, but cert fingerprint is still available behind a "details" toggle).
Flags: needinfo?(jujjyl)
Flags: needinfo?(janx)
Maybe the text "Allow" and "Always Allow" is confusing, because you're not actually allowing the remote debugging, but you're allowing the authentication process to take place (and maybe succeed). Since we're in bikeshed territory, what about: Accept remote debugging connection from 10.243.36.190:52538? This connection requires a QR code to be scanned in order to authenticate the remote device's certificate. [Authenticate] [Authenticate and remember] [Deny] Or maybe something shorter like: [Start scanning] [Scan and remember] [Deny]
Flags: needinfo?(janx)
Alright, I've changed to a version of Jan's suggestion in comment 33, with additional explanation about what remembering means. Also, I removed the "Deny" button (which was redundant with "Cancel" anyway) since it took up too much vertical space on small screens.
Landed on master: https://github.com/mozilla-b2g/gaia/commit/d83b98f9ba88a5027e3637baf51fc98ced6ff0eb
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
CCing l10n.js folks in case they want to double check this bug (I'm quite confused by the amount of .innerHTML used).
Please don't change everything before merging. I'm not happy with the state in which the prompt was landed. - The new explanation text in the prompt is confusing, and it's not my suggestion: "other endpoint" is hard to understand, and "You can also remember the other endpoint to skip scans in the future" is unnecessary and confusing. Please remove the last sentence and don't use "endpoint" or "other endpoint". - You didn't address one of the nits I mentioned in comment 23 [1], without which the button text is misaligned. - There was no need to change the "remoteDebuggerPromptUSB" l10n-id to "remoteDebuggerPromptUSB.innerHTML" because the string didn't change. [1] https://github.com/mozilla-b2g/gaia/pull/28777#discussion_r26485017
Status: RESOLVED → REOPENED
Flags: needinfo?(jryans)
Keywords: leave-open
Resolution: FIXED → ---
Attachment #8575676 - Flags: review+
Ryan, I'm really sorry that I had to request a backout. It's just a few small things to fix, but I didn't want translation to begin on strings that I'd really like to be changed, or developers using a devtools prompt that doesn't look right. It's my fault for giving an r+ too soon, with too many nits, and for keeping it while we discussed important changes to the original commit.
Attached image landed-wifi-prompt.jpg (obsolete) —
As you can see in landed-wifi-prompt.jpg, the button text is too far left and down (it's even more striking in the USB [Allow] [Deny] prompt). When you compare it to wifi-prompt.jpg (which has the CSS change I'm suggesting), you see where button text should normally be. Also, please move the "Scan and Remember" option below the "Scan" option, which is a more natural order. Again, I'm very sorry that I asked Carsten to back you out. Please don't take it the wrong way.
Hmm wait, what is this QR code scan requirement? Does the developer need to point the camera of the phone to scan a code from somewhere? Where does one get the QR code? Can that process be skipped and just (permanently) allow without scanning anything?
(In reply to Jukka Jylänki from comment #41) > Hmm wait, what is this QR code scan requirement? Does the developer need to > point the camera of the phone to scan a code from somewhere? Where does one > get the QR code? The QR code is displayed in WebIDE where the connection is initiated. The QR code plays an important role in the device authentication process. > Can that process be skipped and just (permanently) allow without scanning anything? Not at the moment, but we can discuss such an ability for the future. We would need to be clear that you are giving up on a level of security by choosing to skip it.
Flags: needinfo?(jryans)
In this bug report, I asked for ways to simplify the DevTools connection instead of making it harder. I am not sure this discussion ended up becoming a QR code scan operation? There are other projects than WebIDE that utilize the DevTools connection, for a list, see e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1023084#c14 . Please keep in mind that WebIDE is not the only way that Firefox OS development is performed. We offer embeddable tools to third party middleware, so that for example game engines such as Unity 3D and Unreal Engine could integrate seamlessly Firefox OS deployment and development with their native workflows, identical to how they do Android, iOS and Windows Phone development. With a QR code requirement you would be asking these middleware developers and the other tools listed in the above link to start incorporating QR generation algorithms and new UIs to their tools, which is a tough call. Other vendors don't use QR codes, and I don't think anyone would say they are insecure as a result of that.
Jukka makes a good point: while the current patch is an improvement for Wi-Fi, it doesn't fully address the original bug report, which in my understanding wants: A'. A way to remember the user's choice for a given client certificate in a connection request (be it via USB or Wi-Fi). B'. Process connection requests even when the screen is locked (accepting/rejecting known clients and opening a prompt for unknown clients). C'. Reject all unknown incoming requests while a prompt is being shown (I believe that's already the case). D'. Management of remembered choice per certificate. J. Ryan previously added Wi-Fi as a way to connect (showing a QR code for token+certificate authentication, but that QR code is only for Wi-Fi, not USB) and this patch adds a way to remember user choice, but only for Wi-Fi, and only for accepted connections (partly addresses A'). To fully address the bug report, we need to address points B', C' (if not already done) and especially D'. We also need to fully address A' by allowing a persistent choice for USB connections, and allowing persistent denials of Wi-Fi connections.
Keywords: leave-open
Again, I believe the ideal USB and Wi-Fi prompt we should strive for would have: 1'. [Allow] and [Deny] choices (for Wi-Fi QR scanning: [Scan/Authenticate] and [Deny] choices). 2'. [checkbox] Remember my choice for this device. 3'. Indication of what will be remembered (we debated over showing a certificate vs explaining that there is one). Or a variant of this, where it's not possible to remember a denial, just like Android's remote debugging prompt[1] with its "Always allow from this computer" checkbox. All remembered choices should then become visible/editable somewhere in the Developer menu, e.g. in a "Known Devices" sub-menu with a list of "[Device Name?] [Device Certificate] [Persisted Choice]". Or an easier variant like a "Clear All Known Devices" button, as suggested by Jukka in comment 0 point 7'. [1] Android's awesome remote debugging prompt: http://i.imgur.com/UrlAtYU.png
(In reply to Jukka Jylänki from comment #43) > In this bug report, I asked for ways to simplify the DevTools connection > instead of making it harder. I am not sure this discussion ended up becoming > a QR code scan operation? At this time, "always allow" will only be available via WiFi debugging. The security of the current design relies on authenticating the certificates used. At the moment, the mechanism we offer to do so is scanning a QR code. We can certainly come up with other paths in the future. I felt this bug was a natural fit for the current work to make this possible. From recent comments, it seems that was a mistake, as this bug is really a super-set of the work I was performing for Wi-Fi. I'll move my work to a new bug that this can depend on. > There are other projects than WebIDE that utilize the DevTools connection, > for a list, see e.g. > https://bugzilla.mozilla.org/show_bug.cgi?id=1023084#c14 . Please keep in > mind that WebIDE is not the only way that Firefox OS development is > performed. We offer embeddable tools to third party middleware, so that for > example game engines such as Unity 3D and Unreal Engine could integrate > seamlessly Firefox OS deployment and development with their native > workflows, identical to how they do Android, iOS and Windows Phone > development. With a QR code requirement you would be asking these middleware > developers and the other tools listed in the above link to start > incorporating QR generation algorithms and new UIs to their tools, which is > a tough call. Other vendors don't use QR codes, and I don't think anyone > would say they are insecure as a result of that. Right, I am quite aware. :) I have participated in the bug you mentioned, and I have also assisted the group working on node-firefox, which is just one example of such a client library. No matter what solutions we come up with, I expect they would all require some changes in these client libraries. The one being added at the moment makes use of QR codes, but there are still other client changes to make also about how the authentication takes place at the protocol level. I did not mean to imply "QR codes are required for security". All I mean is that the current design of Wi-Fi debugging uses QR codes for authentication. This particular design would offer reduced security by skipping the QR code scanning process. We can certainly devise other methods in the future as well. Jan has previously suggested a version that uses Firefox Accounts to authenticate.
Keywords: leave-open
Assignee: jryans → nobody
No longer blocks: wifi-rdp
Status: REOPENED → NEW
Depends on: 1145221
Attachment #8575673 - Attachment is obsolete: true
Attachment #8618180 - Flags: review+
Attachment #8618181 - Flags: review+
Attachment #8618182 - Flags: review+
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 10 years ago → 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: