Adloox Analytics Module: apply 'js' param constraint #12618
Conversation
|
How did the bad actor access the page? I wonder if we could track if this config ever changes in a pv after being set? If the bad actor could modify prebid config, why did it need to use the vector to load script instead of just loading it directly? |
| @@ -109,6 +109,10 @@ analyticsAdapter.enableAnalytics = function(config) { | |||
| logError(MODULE, 'invalid js options value'); | |||
| return; | |||
| } | |||
| if (isStr(config.options.js) && !/\.adlooxtracking\.(com|ru)$/.test(parseUrl(config.options.js, { 'noDecodeWholeURL': true }).host)) { | |||
There was a problem hiding this comment.
what if TLD+1 is same as window.location?
There was a problem hiding this comment.
Maybe better to amend parseUrl to use new URL() (instead of <a/>) now you have ~20 other users of this in Prebid.js already? You could then potentially move them over to this; or would you prefer to deprecate this function altogether and have the modules themselves just inline new URL()?
As for the consequences, for us, this might affect an internal test page, but nothing we would not just workaround or fixup with a latter PR.
This is unknown, all I received was a screenshot that was taken by someone and was passed to me by someone else, so it is hearsay by at least a distance of two...probably more.
Not sure what you mean by 'pv' here, but I instead would just deep copy it on initialisation and have modules access this (immutable) copy instead of the original object directly. Alternatively, patch the object (and children objects) to call your getters/setters by way of
This was my thought too, not really sure why they bothered to even involve prebid.js if they had access to the JavaScript context; maybe the objective was to use our module as a smokescreen but then that does not entirely add up either. The screenshot I received showed our domain serving malicious code but it was also claimed someone spoke to us to purge the offending code yet those with asset publishing access never received any such communication. My guess so far is what was seen might have been an IDN homograph attack so it presented as our domain in the screenshot. What was actually purged was something in the publishers site which could explain why no one contacted us and everything got resolved. Fortunately, this did prompt me to review that section of the code. Though we do not know if this was the root cause, or even if this happened, there is no point leaving a gate open needlessly so the PR is to close it. |
The pubs self host the prebid.js blob, but they do not generally self host the verification JS. If they need to, we would just have them patch the module in their own internal fork.
Nevermind...it appears when I run the full test suite....working on it.
|
Right, figured out some more by tracking down the source of the screenshots; a scam tracker posting on X. It was claimed[1] Prebid.js was involved in the loading of our verification script but from another screenshot[2] it was our direct GAM/GPT integration (that chainloads Inspecting the code for the our direct GAM/GPT code ( I suspect the researcher may have seen Though for us, we still wish to lock down the sub-domains allowed in our module, but for yourselves maybe any urgency there was here may be dropped a notch as I am pretty certain our module (and thus Prebid.js) were not involved. That said, maybe you consider users of Thoughts? Maybe you have an alternative view of what may have happened here? Cheers [1] https://x.com/realScamSniffer/status/1871908228805878244 |
It gets tricky if we can't trust configuration - external scripts are not the only attack vector. Off the top of my head I can think of renderers and mock creatives that can be set up with prebid config and would allow arbitrary code execution. I'm sure there's many, many more. |
|
Thanks for all the detailed information! Glad you were vindicated in the end. I'm happy to merge once tests are passing and we'll discuss more in committee |
Stop being clever for my own good with the NOOP function blatting, it is a non-idempotent operation
jimdigriz
force-pushed
the
adloox-subdomain
branch
from
d0a48ac to
0e49bcd
Compare
|
@patmmccann fixed the test, let me know if you need anything else, thanks! |
Type of change
Description of change
The publisher is allowed to provide an override URL for the Adloox Analytics Module which us use as the location to load the verification JS by way of
loadExternalScript.Anything is allowed, whilst in practice only sub-domains of
adlooxtracking.{com,ru}are meaningful.This PR changes the module to only accept sub-domains of
adlooxtracking.{com,ru}for the value of the optionjs.Other information
The trigger for this change is a client had a setup where a bad actor could provide a URL for their own script instead before the module initialised.
At a glance (51Degrees, aaxBlockmeterRtd, browsiRtd, ...) I suspect there are other modules with a similar problem.
Maybe it worth amending
loadExternalScriptto include an enforcer that which validates against a list of module provided (sub)domains to make review/vetting easier? When not supplied, the debug version can state something like "pants may explode" or prompt a code reviewer to require better justification for arbitrary URL external script loading.