Skip to content

Foxhound: Add taint propagation for HashChangeEvent.oldURL and newURL#363

Draft
Stouelh wants to merge 1 commit into
SAP:mainfrom
Stouelh:main
Draft

Foxhound: Add taint propagation for HashChangeEvent.oldURL and newURL#363
Stouelh wants to merge 1 commit into
SAP:mainfrom
Stouelh:main

Conversation

@Stouelh
Copy link
Copy Markdown

@Stouelh Stouelh commented Apr 24, 2026

Fixes #323

When a hashchange event fires, the oldURL and newURL properties on the
HashChangeEvent were not being marked as taint sources. This means that
if tainted data (e.g. from location.hash or location.href) triggered a
hash change, Foxhound would lose track of the taint flow through those
event properties.

This fix adds MarkTaintSource calls for both init.mOldURL and init.mNewURL
in FireHashchange() in nsGlobalWindowInner.cpp, following the same pattern
used for location.hash and other existing taint sources.

Note: A mochitest is still needed to verify the fix. Opening as draft for
early feedback.

Signed-off-by: Soliman Touelh stouelh@umich.edu

Signed-off-by: Stouelh <124311326+Stouelh@users.noreply.github.com>
@cla-assistant
Copy link
Copy Markdown

cla-assistant Bot commented Apr 24, 2026

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@leeN
Copy link
Copy Markdown
Collaborator

leeN commented May 17, 2026

The changes look fine to me from a code point of view. As you pointed out, mochitests are required, but I have an additional request.

Please add the sources to the configuration all.js. We probably should do some CI checking that this is synced with the sources/sinks we have, i have opened an issue for that #366. :)

My concern with this change is that we have no way to see whether the hash is actually tainted. For example, if I do:

location.hash = "baz";

The event would fire and report this as tainted, even if it's just a static assignment. I think merging this is fine if you set the default state to false in all.js and manually enable it in your setup. I think all automation frameworks allow you to override Firefox preferences.

Would that work for you?

I'd prefer if we'd propagate the taints automatically here, but I think it gets lost by assigning it to an NsIURI. Making that conversion taint-aware might do the trick, long-term, as I think this should, in theory, be propagated automatically. But that is a much more involved change, so I'd be happy to merge this as a temporary fix until we look into why it isn't being propagated properly.

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

Successfully merging this pull request may close these issues.

Missing sources: HashChangeEvent.oldURL and HashChangeEvent.newURL

2 participants