Security vulnerability: Use strict equality operator

[anematode]

The code currently uses the abstract equality operator ==, meaning that a stringified version of the isOdd function will incorrectly return true.

Example:

const isOdd = require('is-odd');
const isIsOdd = require('is-is-odd');

console.log(isIsOdd(isOdd.toString())); // -> true

[anematode]

Any updates on this issue? We are worried about this exploit in production and would appreciate an update. I can make a pull request if needed

[slmjkdbtl]

Sorry, maintaining this library and meet the security requirement seems to be too much work for me at the moment. PR is appreciated 🙏

[anematode]

Thank you for the prompt reply, I'll see if any of my employees can figure out a fix

[Alexiscomete]

This is a security issue ... we must fix it

[leewardbound]

It has come to my attention that this "security hole" is actually a compatability feature, enabling a whopping 2.5% greater market penetration and generally wider availability to end users.

I propose we close this as #wont-fix, this is the expected behavior and we shouldn't let ONE USER and some speculative idea of "SECURITY" dictate how we build software in 2021. Security is relative to every project - I don't lock my van but nobody's ever broken into it, seems to me like the whole car lock-and-alarm industry is just a cash-cow for lockout services and roadside assistance.

Clearly, adopting this change will harm many users experience. Unacceptable.

[slmjkdbtl]

97.56% means there will be roughly 193 million people be affected around the world, which is something we probably can't afford. Thanks @leewardbound for bringing this to our attention.

Is there any polyfill we can use to achieve the same?

@anematode Do you have any insights on the tradeoff between letting 193 million people crash vs allowing some sneaky non-isOdd being recognized as isOdd?

[anematode]

Hello all,

I have been mulling this question over for a bit. A polyfill was my first instinct, but writing an effective polyfill is proving difficult.

This solution prevents the original attack vector, but a problem remains. A masquerading a could even pretend to be b by overriding its own toString method to be the result of b.toString().

The question is thus how to check whether two variables have the same referent using only the abstract equality operator (known in ECMAScript as the "equality operator", internally calling the abstract operation "isLooselyEqual"). Excluding BigInts, the behavior of the operator has not changed since its inception—as far as I am aware. The relevant abstract operations called in turn by "isLooselyEqual" are "isStrictlyEqual", "toNumber", and "toPrimitive". A close examination of the logic reveals the only relevant operation of a masquerading isOdd function is its conversion to a string, which is called via the toString method. The question is thus whether two functions with identical stringified versions can be distinguished without usage of the strict equality operator. As far as I am aware, doing so is impossible in versions before IE 9, for example, since the isStrictlyEqual abstract operator is not referenced except by functions such as Array.prototype.indexOf.

I believe I have found a watertight solution, however. We can redefine the toString operation on the isOdd function to output a random value. Consider the following code:

var a = { toString: function () { return Math.random() } };

a == a // -> true, because typeof(a) == typeof(a) and they are immediately compared
var masquerading = a.toString()
masquerading == a // -> almost always false, because a.toString() will be different

masquerading = function () {}
masquerading.toString() = { toString: function() { return a.toString(); } }

masquerading == a // -> always false, because typeof(masquerading) == typeof(a) and they are immediately compared

Edit: Upon further reflection, I believe the following polyfill should be sufficient for all the cases we care about.

function isStrictlyEqual(a, b) {
  return (typeof a == typeof b) && (a == b);
}

[anematode]

IMPORTANT UPDATE:

After careful review, I discovered that the abstract equality and strict equality operators actually have equal shares in the market. More precisely, the browsers which support one also support the other. Thus, I believe @leewardbound's assertion that using strict equality will decrease market penetration is moot, since the abstract equality operator is also not available for 2.5% of users.

I will be discussing a polyfill for the abstract equality operator with my team. In the meantime, with this new revelation, I believe the change will not affect any users. The only drawback I forsee is an extra byte in the source, which would marginally increase download times; but, in response to @slmjkdbtl's concern about safety, I believe this extra byte is worth the security improvement.

[slmjkdbtl]

@anematode Thank you for the diligent research, they do have indeed the same coverage.

Since you're using isIsOdd in production, are you able to setup an A/B test in your product with either approach? (My assumption is longer average download time is less severe than the security hole here)

[anematode]

@slmjkdbtl I am a bit wary to set up such a test in production, but I will ask my team to do a simulation of the results. For reference, our code base is roughly 120 kilobytes (not including 300 kb of polyfills that are conditionally loaded), so those with larger or smaller code bases may see different results.

[slmjkdbtl]

@anematode I see, any simulation will be helpful. Feel free to ask your team to work overtime over the weekend & holidays.

[anematode]

Unfortunately, due to some recent exploits—or so I'm told—my team is entirely focused on our (Java-based) backend. I expect to have results by mid-January.

[slmjkdbtl]

It's a shame, but I totally understand. Again thanks for taking the responsibility on this, it's appreciated.

Do you think the Java world have a need for a library like is-is-odd? I'm recently thinking about expanding to the larger open source community.

[leewardbound]

@slmjkdbtl a Java port would be tremendous! Right now, I load isIsOdd using Mozilla's Rhino (https://github.com/mozilla/rhino) in my Java projects, allowing me to leverage the notoriously lightweight, user-friendly JVM to achieve better portability for my apps! With a native Java port, when I distribute my app as a Spring binary, and package it for my userbase (mostly running modded GameBoy Advanced devices), I will be able to load isOdd as an asynchronous JavaScript dependency directly over the satellite uplink, using a normal <script> tag. This will save me probably at least 5-6 whiteboard markers per month, as currently we spend a lot of time in design meetings discussing the dependency loading order in Rhino.

[Alexiscomete]

We can make 2 differrent functions: isIsOdd and isIsOddStrict to solve the debate

[slmjkdbtl]

@Alexiscomete It does kinda solve the problem, but it also adds bloat to the library. Also an additional function means more surface area for bugs and more vulnerabilities. We need to be real careful with this.

[Alexiscomete]

Ok

[anematode]

Any updates on this issue?

[Alexiscomete]

@anematode I have open a pr ( #5 ) but it was never pull

[Alexiscomete]

Change the issue's subject to share memes ? 👍 yes 👎 no

[Alexiscomete]

this poll has an equality @victorbln, like the operator :octocat: