Unsafe Jquery Plugin - Potential False Negative

[Naman-ntc]

Hi,
I was analyzing the results of unsafe jquery plugin query and I found an interesting example.
First, here is a code snippet marked vulnerable in query tests

(function(){
        $.fn.my_plugin = function my_plugin(element, options) {
                this.$element      = $(element);
                this.options       = $.extend({}, options);
                if (this.options.parent) this.$parent = $(this.options.parent) // NOT OK
        };
})

Here is (slightly modified) code I found in Collapse library which doesn't get marked as vulnerable

function ($) {
  var Library = function (element, options) {
    this.$element = $(element)
    this.options = $.extend({}, options)
    if (this.options.parent) {this.$parent = $(this.options.parent)}}
}(window.jQuery);

Also note that in the second example, the if statement acts as a barrier guard and makes it safe. Please let me know if it is indeed safe (and how!) or if it is a false negative

[adityasharad]

The query for unsafe jQuery plugins looks for function values assigned to $.fn.<plugin name>. Your second example has a function that looks like a jQuery plugin, but the code snippet doesn't show this function being assigned as a property of $.fn to register it as a plugin, which I think means there is nothing for the query to find in the snippet.
Could you share the complete snippet if there is actually such an assignment that needs to be detected?

[Naman-ntc]

Hi Aditya,
Happy new year!

The following snippet also doesn't get marked as safe :

function ($) {
  $.fn.my_plugin = function my_plugin(element, options) {
    this.$element = $(element)
    this.options = $.extend({}, options)
    if (this.options.parent) {this.$parent = $(this.options.parent)}}
}(window.jQuery);

On a related note is this code snippet safe? I don't see it getting marked vulnerable under another class as well.
The example is taken from bootstrap-collapse.js (line 33) where it marks the linked line as barrier guard.

[adityasharad]

Happy new year! Let me pass this on to our JavaScript analysis team, who have greater expertise on this query.

[esbena]

The query behaves as I would expect it to on the three provided examples (unsafe, safe, unsafe). The potential barrier guards are not relevant to the results discussed in this issue.

Aditya is right that your second example is not marked as unsafe since there's no indication that Library is a jQuery plugin. The third example uses the $.fn.my_plugin = ... pattern which means that the query identifies it as a jQuery plugin.

---

On a related note is this code snippet safe?

Probably not. I would change the $(this.options.parent) to $(window).find(this.options.parent) or something like that to avoid using $(<taint>).

The query is different from the ordinary XSS query in the sense that it identifies a likely mistake by the plugin author. So an alert from the query just means that there's a likelihood that the plugin may be used in an unsafe manner.

[Naman-ntc]

Hi,
Thanks for the explanation. Actually, I had a typo!
The following codes both get marked as safe (I think the first snippet has syntax error)

function ($) {
  $.fn.my_plugin = function my_plugin(element, options) {
    this.$element = $(element)
    this.options = $.extend({}, options)
    if (this.options.parent) {this.$parent = $(this.options.parent)}}
}(window.jQuery);

and

function test($) {
  $.fn.my_plugin = function my_plugin(element, options) {
    this.$element = $(element)
    this.options = $.extend({}, options)
    if (this.options.parent) {this.$parent = $(this.options.parent)}}
}(window.jQuery);

Also, is the second code (where Library is assigned instead of $.fn.my_plugin) safe or $.find needs to be used there also? And if so should there be a (new?) query to detect that?

[esbena]

The following codes both get marked as safe (I think the first snippet has syntax error)

Correct. In both cases, you are using the wrong syntax.

1. syntax error
2. function f(){}() is not a function call. Try to format it with prettier or something like that.

The following has the expected function call, and is marked as unsafe.

(function test($) {
  $.fn.my_plugin = function my_plugin(element, options) {
    this.$element = $(element)
    this.options = $.extend({}, options)
    if (this.options.parent) {this.$parent = $(this.options.parent)}}
})(window.jQuery);

---

Also, is the second code (where Library is assigned instead of $.fn.my_plugin) safe or $.find needs to be used there also?

Yes, if Library somehow is exposed to clients as a non-standard "jquery plugin", then I would argue that it is unsafe.

And if so should there be a (new?) query to detect that?

Without additional context, I do not see how a query could flag it. As it is Library is an unused variable, so it can not be used unsafely.

[Naman-ntc]

Correct. In both cases, you are using the wrong syntax.
Ahh, thanks for pointing this out. Now it makes sense.

Without additional context, I do not see how a query could flag it. As it is Library is an unused variable, so it can not be used unsafely.
Hmm, I see the problem. Thanks