Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Form hijacking with formaction, formmethod #169

Closed
mozfreddyb opened this issue Aug 15, 2022 · 5 comments
Closed

Form hijacking with formaction, formmethod #169

mozfreddyb opened this issue Aug 15, 2022 · 5 comments

Comments

@mozfreddyb
Copy link
Collaborator

In In https://bugzilla.mozilla.org/show_bug.cgi?id=1783962#c2 (currently kept private because the issue is unrelated and this was a drive-by comment), the reporter noted that the Sanitizer API currently allows the formmethod and formaction attributes.

An injection that uses either of those attributes could hijack an existing input form and use it to exfiltrate data.

This is currently allowed by spec, as we intentionally focus on XSS exclusively. Indeed, hijacking of forms can already be prevented with a CSP.

For now, I want to make sure that our default config does not allow it and eventually, I'd like to discuss if this is something worth incorporating into our threat model.

@mozfreddyb
Copy link
Collaborator Author

On another note, we should make sure form hijacking is specifically mentioned in the spec (e.g., under Security Considerations)

@otherdaniel
Copy link
Collaborator

Agreed to both, adding a note and not allowing these in the default config.

I wonder whether we should drop all form content in the default?

@koto
Copy link

koto commented Aug 22, 2022

If non-XSS vectors are not the focus of the spec, why changing the default allowlist? It seems a bit ad hoc, especially if only form hijacking was to be taken into account. Why not all form-y content like @otherdaniel suggests? Attacker can just create a new form with styles that make it visually obscure the original form, and exfiltrate anything with user interaction. formaction seems only to selectively address exfiltrating automatically filled-in inputs (hidden fields with tokens, or creds inserted by password managers), which feels like a very narrow improvement.

Why only changing the default, but allowing overrides in the config? It feels like a partial solution - we think some threats are real enough to fix in a Web API, but at the same time allow authors to reenable them.

@otherdaniel
Copy link
Collaborator

Why only changing the default, but allowing overrides in the config?

My mental model here is that we have three classes of content: default allowed, allowable (in baseline, but not in default), and perma-blocked, which maps to harmless content, mostly harmless, and dangerous/script-y. I personally feel relatively generous about moving stuff into the "mostly harmless" category, but IMHO the "dangerous" category should be limited to actually script-y content.

I'm not entirely sure whether this is a shared view, so this might need a bit more discussion. Maybe we should also re-start discussion of "profiles" (pre-built configs), to make it easier for devs to pick a config that suits their needs.

@mozfreddyb
Copy link
Collaborator Author

Thanks, folks! I agree with @koto that we shouldn't limit ourselves to only specific form hijacking vectors (with the attributes) but rather consider all forms when making the decision.
Furthermore, I also agree with @otherdaniel that we should keep our focus on XSS attacks first and foremost and thus keep forms allowable (but not allowed by default)

Any other concerns I missed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants