-
Notifications
You must be signed in to change notification settings - Fork 7
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
Security vulnerability: Use strict equality operator #4
Comments
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 |
Sorry, maintaining this library and meet the security requirement seems to be too much work for me at the moment. PR is appreciated 🙏 |
Thank you for the prompt reply, I'll see if any of my employees can figure out a fix |
This is a security issue ... we must fix it |
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 Clearly, adopting this change will harm many users experience. Unacceptable. |
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? |
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 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 I believe I have found a watertight solution, however. We can redefine the toString operation on the 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);
} |
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. |
@anematode Thank you for the diligent research, they do have indeed the same coverage. Since you're using |
@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. |
@anematode I see, any simulation will be helpful. Feel free to ask your team to work overtime over the weekend & holidays. |
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. |
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 |
@slmjkdbtl a Java port would be tremendous! Right now, I load |
We can make 2 differrent functions: isIsOdd and isIsOddStrict to solve the debate |
@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. |
Ok |
Any updates on this issue? |
@anematode I have open a pr ( #5 ) but it was never pull |
Change the issue's subject to share memes ? |
this poll has an equality @victorbln, like the operator |
The code currently uses the abstract equality operator
==
, meaning that a stringified version of the isOdd function will incorrectly return true.Example:
The text was updated successfully, but these errors were encountered: