Open Bug 1122237 Opened 10 years ago Updated 2 years ago

Treat pages with insecure form action as Mixed Content

Categories

(Core :: DOM: Security, defect, P4)

defect

Tracking

()

People

(Reporter: tanvi, Unassigned)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [domsecurity-backlog])

If an HTTPS page includes a form who's action points to an HTTP destination, use the mixed display icon in the security UI instead of the lock. If the action is a call to a javascript function then the security UI will show the lock, since we can't reasonably parse the javascript to determine the final destination until submit is hit. When submit is hit, if we find that the destination is HTTP, we should change the security UI, however it will probably be too late as the page will navigate. Chrome has implemented this. Firefox instead displays an alert box warning[1] when a user tries to submit an HTTP form from an HTTPS page. This would be in addition to that warning. [1] https://people.mozilla.org/~tvyas/HTTPS-post-HTTP.jpg
This is an optional part of the spec - https://w3c.github.io/webappsec/specs/mixedcontent/#requirements-forms I'm happy to mentor this bug if anyone wants to take it on.
(In reply to Tanvi Vyas [:tanvi] from comment #0) > Chrome has implemented this. Firefox instead displays an alert box > warning[1] when a user tries to submit an HTTP form from an HTTPS page. > This would be in addition to that warning. > > [1] https://people.mozilla.org/~tvyas/HTTPS-post-HTTP.jpg Is the warning enough? The page itself is fully secure unless and until the user fills out a form and tries to submit to HTTP. At which point, we show them the alert box warning. If they never fill out the form, then they do have a fully secure experience that warrants the lock icon. If we are trying to push the web forward and prevent developers from putting HTTP form posts on HTTPS pages, then yes, this is a good way to do that. But [1] is also a good way to do that and has been around for years. Thoughts welcome!
I think we should have degraded UI as that's more likely to cause web developers to act. As for the dialog, users will click through the dialog if they are attempting to complete a task. We could gather telemetry data on Cancel vs Continue to be sure, but I have the feeling it's more of a nuisance than an aid.
We see this happen on between 0.007% and 0.008% of page loads, which seems to be holding pretty stable over the past month. The counter is in beta right now, so I do expect a change relatively soon when the counter hits stable. Once we know what the stable number looks like, we'll start evaluating how to drive it down further, and whether or not we can start being more aggressive with mixed form submissions.
https://www.chromestatus.com/metrics/feature/timeline/popularity/665 is the counter, which I neglected to paste in above. :)
We actually have some telemetry on the click through rate of the dialog - Number 9 is the number of times the dialog appears and Number 10 is the number of times users click through it. http://telemetry.mozilla.org/#filter=release%2F35%2FSECURITY_UI%2Fsaved_session%2FFirefox&aggregates=multiselect-all!Submissions&evoOver=Time&locked=true&sanitize=true&renderhistogram=Graph For FF 34 (FF 35 just went out this week so it's too soon to look at that data), there were 968K reported occurences, and 896K users clicked through. ~92% which is not great. But, 0.008% of page loads is not bad at all! And one could argue that the reason that not many sites follow this bad practice is because Firefox has had this alert style warning for years (maybe forever?). Anyway, thinking about this more... by the time users are submitting a form, they have already spent time and effort entering data and want to complete their task (as Anne mentions). So if we a degraded UI upfront perhaps it will prevent users from ever entering the data or at least prompt them to be mindful of the data they are entering (maybe they will enter less info or some bogus info).
Component: Security → DOM: Security
Whiteboard: [domsecurity-backlog]
> So if we a degraded UI upfront perhaps it will prevent users from ever entering the data or at least prompt them to be mindful of the data they are entering (maybe they will enter less info or some bogus info). This would require us to move out the code for insecure password management into something a little more generic to handle all types of forms. I would like to propose a new component "insecurecontent" to handle these errors in: toolkit/components/insecurecontent/InsecureContentBehaviour{Parent,Child}.jsm - This would move the methods: "setHasInsecureLoginForms" and "hasInsecureLoginForms" from LoginManagerParent and manage state of when these get called. - dom/html/HTMLFormElement.cpp would despatch a "DOMFormHasInsecureAction" event much like it triggers a "DOMFormHasPassword" when "BeforeSetAttr" is called. - browser/base/content/content.js would pick this up and call functions on the new InsecureContentBehaviourChild.jsm This new component would hold any behaviour we wanted in future that is similar to the Mixed Content Blocker but at parse time and not request based. :tanvi, :johannh, :MattN - does the above sound ok? I have some experimental code that has half of this and I don't think moving this code should be too much work if you are all happy. Other possible names for this component is "contentinterventions" named after: https://github.com/WICG/interventions/issues too. > And one could argue that the reason that not many sites follow this bad practice is because Firefox has had this alert style warning for years This is an argument as to why I don't think we should remove this warning, at least in the short term.
Flags: needinfo?(tanvi)
Flags: needinfo?(jhofmann)
Flags: needinfo?(MattN+bmo)
Apparently sites worked around the <input type=password> warning by degrading that to <input type=text> with custom styling. How big is the risk they go from <form action> to <a href> to work around this? I'm guessing it's still worth it even if they do that, assuming most sites are more rationally operated, but we might want to consider also degrading for insecure links at some point (or a subset of those to start, e.g., those with a non-null query).
> Apparently sites worked around the <input type=password> warning by degrading that to <input type=text> with custom styling. Yeah Bug 1413344 is for this issue, that site actually stopped too it seems. > How big is the risk they go from <form action> to <a href> to work around this? Possibly lower given this already would be the case to mitigate the modal warning. I can't see this measure increase the desire to this more than is already present. Fetch would be blocked so large amounts of data aren't going to be exfiltrated. > but we might want to consider also degrading for insecure links at some point Agreed. This is a cat and mouse game as mentioned in the other bug, I feel like closing off more avenues gives a clear indicator of our intent to developers.
Comment 8 sounds like a good plan to me, though off-hand you might need to duplicate DOMFormHasPassword listeners to have one for filling forms and one for handling insecure forms. Matt definitely knows more about how much this code is tied to password manager right now. (In reply to Anne (:annevk) from comment #9) > Apparently sites worked around the <input type=password> warning by > degrading that to <input type=text> with custom styling. How big is the risk > they go from <form action> to <a href> to work around this? > > I'm guessing it's still worth it even if they do that, assuming most sites > are more rationally operated, but we might want to consider also degrading > for insecure links at some point (or a subset of those to start, e.g., those > with a non-null query). I think HTTPS sites that degrade forms to HTTP are extremely rare by now, at least we can prove through telemetry that downgraded login forms are virtually nonexistent (see https://bugzilla.mozilla.org/show_bug.cgi?id=616849#c34). To be frank, I want this change mostly so that we can consider killing the window modal dialog that shows up on mixed form actions, because I get the feeling at this point it does more harm (through providing attackers DOS openings) than good.
Flags: needinfo?(jhofmann)
> though off-hand you might need to duplicate DOMFormHasPassword listeners to have one for filling forms and one for handling insecure forms I was expecting we might need that however the intent is to if we do need other listeners is also to call into the same "insecurecontent" component regardless. > I think HTTPS sites that degrade forms to HTTP are extremely rare by now My push here is just another developer aid, providing url bar warnings is easer to debug.
(In reply to Jonathan Kingston [:jkt] from comment #8) > > So if we a degraded UI upfront perhaps it will prevent users from ever entering the data or at least prompt them to be mindful of the data they are entering (maybe they will enter less info or some bogus info). > > This would require us to move out the code for insecure password management > into something a little more generic to handle all types of forms. Why? That code doesn't need to change AFAICT since this bug isn't specific to passwords. > I would like to propose a new component "insecurecontent" to handle these > errors in: > > toolkit/components/insecurecontent/InsecureContentBehaviour{Parent,Child}.jsm > > - This would move the methods: "setHasInsecureLoginForms" and > "hasInsecureLoginForms" from LoginManagerParent and manage state of when > these get called. > - dom/html/HTMLFormElement.cpp would despatch a "DOMFormHasInsecureAction" > event much like it triggers a "DOMFormHasPassword" when "BeforeSetAttr" is > called. Instead this could work like tracking protection, mixed active, and mixed display content and be implemented in C++. Then we don't need a new component and follow the existing conventions. See SetHasMixedDisplayContentLoaded for an example. Basically HTMLFormElement would call `doc->SetHasInsecureFormAction(true)` if the action is insecure and it would propagate to the UI like the other similar types of insecure content. https://dxr.mozilla.org/mozilla-central/search?q=SetHasMixedDisplayContentLoaded&redirect=false This approach should be more performant as it's all C++ and there's no new events. > - browser/base/content/content.js would pick this up and call functions on > the new InsecureContentBehaviourChild.jsm > > This new component would hold any behaviour we wanted in future that is > similar to the Mixed Content Blocker but at parse time and not request based. > > :tanvi, :johannh, :MattN - does the above sound ok? I have some experimental > code that has half of this and I don't think moving this code should be too > much work if you are all happy. The plan seems a bit complicated considering the low number of affected sites. > Other possible names for this component is "contentinterventions" named > after: https://github.com/WICG/interventions/issues too. > > > And one could argue that the reason that not many sites follow this bad practice is because Firefox has had this alert style warning for years > > This is an argument as to why I don't think we should remove this warning, > at least in the short term. Perhaps it would be worth making that warning scarier as that would be simpler and likely more noticeable than an address bar indicator. A console warning would also be a lower cost solution for this low reward problem.
Flags: needinfo?(MattN+bmo)
(In reply to Johann Hofmann [:johannh] from comment #12) > To be frank, I want this change mostly so that we can consider killing the > window modal dialog that shows up on mixed form actions, because I get the > feeling at this point it does more harm (through providing attackers DOS > openings) than good. Johann, can you expand on what you mean by this? I want to keep the modal warning. It is the only protection we have against <form action=javascript_function()>. We don't know if the form is being submitted to http or https until it is too late to change the UI indicator in the url bar. It comes up so infrequently that I'm not worried about annoying the user. We could modernize it. Reading through this bug, we could change the UI in the urlbar in cases where we can detect an insecure form action, but it seems like a lot of work for something we know does not happen that often. We can do it with a security flag, the way we do for mixed content, insecure passwords, etc. Or we could carry out Jonathan's plan in comment 8; but either way it is a lot of work. So I propose we leave this as a blocker to the https-everything meta bug but give it a low priority. I hear we are allowed to use P4 now, so I'm setting this to a P4.
Flags: needinfo?(tanvi)
Priority: -- → P4
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.