You. Had. ONE Job!
-
Sometimes ....
Insecure Comparision
secure-compare is a node implementation of constant-time comparison algorithm to prevent timing attacks for Node.js. In versions prior to 3.0.1, the compare function made sure that the length of the two arguments is the same, and then mistakenly compared the first argument to itself, meaning that the function would return true for any two arguments of the same length.
~ https://snyk.io/vuln/npm:secure-compare:20151024
Words ... fail me.
-
@svieira said in You. Had. ONE Job!:
the function would return true for any two arguments of the same length
Sounds secure to me. An attacker has no way of knowing if the two arguments are actually the same. It's brillant!
-
How do you fuck up a function that simple?
-
For three versions no less!
-
@svieira version 3.0.1
:wtf:
-
@svieira said in You. Had. ONE Job!:
secure-compare is a node implementation of constant-time comparison algorithm to prevent timing attacks for Node.js. In versions prior to 3.0.1, the compare function made sure that the length of the two arguments is the same, and then mistakenly compared the first argument to itself, meaning that the function would return true for any two arguments of the same length.
That's the best part. Did it really take them until 3.0.1 to notice that their function was that broken?
-
@Dragnslcr TDD
public void SucceedsOnsameString() { string stringToTest = "this test should pass"; assert.IsTrue(secure-compare(stringToTest, stringToTest)); } public void FailsOnDifferentString() { string string1 = "this is the first string"; string string2 = "this is the second string"; Assert.IsFalse(secure-compare(string1, string2)); }
-
Oopsie indeed.
But wouldn't unit tests have caught this?
/** * Tests */ describe ('secure-compare', function () { it ('compare', function () { compare('abc', 'abc').should.equal(true); compare('abc', 'ab').should.equal(false); }); });
Ooops. Note that the "fix" didn't do anything to fix the tests.
-
So...the lesson here is that no one ever actually used this thing?
-
@Jaloopa said in You. Had. ONE Job!:
@Dragnslcr TDD
public void SucceedsOnsameString() { string stringToTest = "this test should pass"; assert.IsTrue(secure-compare(stringToTest, stringToTest)); } public void FailsOnDifferentString() { string string1 = "this is the first string"; string string2 = "this is the second string"; Assert.IsFalse(secure-compare(string1, string2)); }
@cartman82 said in You. Had. ONE Job!:
/** * Tests */ describe ('secure-compare', function () { it ('compare', function () { compare('abc', 'abc').should.equal(true); compare('abc', 'ab').should.equal(false); }); });
Ooops. Note that the "fix" didn't do anything to fix the tests.
Once again, the truth proves sillier than anything a human mind can make up.
-
@Jaloopa said in You. Had. ONE Job!:
@Dragnslcr TDD
public void SucceedsOnsameString() { string stringToTest = "this test should pass"; assert.IsTrue(secure-compare(stringToTest, stringToTest)); } public void FailsOnDifferentString() { string string1 = "this is the first string"; string string2 = "this is the second string"; Assert.IsFalse(secure-compare(string1, string2)); }
That would fail properly though.
This wouldn't
function failsOnDifferentString() { var string1="This is the 1st string"; var string2="This is the 2nd string"; assert.isFalse(secure-compare(string1,string2)); }
-
I don't see any problem - its job was secure compare, not bug-free compare - doing both would violate the Single Responsibility principle.
-
@sloosecannon Exactly. Turns out I was pretty close to what the actual tests are
-
That was going to bother me all day.
-
@Yamikuronue you're not covering the "different in UTF8" case? Especially given the whole UTF equivalence mess...
-
-
@Maciejasjmj said in You. Had. ONE Job!:
@Yamikuronue you're not covering the "different in UTF8" case? Especially given the whole UTF equivalence mess...
Nah, I just figured his advertised samples on the readme.md should be passing for a build to be passing
-
-
@Maciejasjmj said in You. Had. ONE Job!:
@Yamikuronue said in You. Had. ONE Job!:
readme.md
Huh, TIL that works.
If by works you mean redirects to a random github project's readme.md that was apparently important enough to become the readme.md...
-
@CreatedToDislikeThis said in You. Had. ONE Job!:
@Maciejasjmj said in You. Had. ONE Job!:
@Yamikuronue said in You. Had. ONE Job!:
readme.md
Huh, TIL that works.
If by works you mean redirects to a random github project's readme.md that was apparently important enough to become the readme.md...
What a waste. Imagine putting a guide to the internet there.... Hey, does anyone have some of the other ones? Hmm I have some ideas....
-
The funny thing is:
According to their npm package site, they copied that function from a different library called cryptiles:
The implementation that was faulty in secure-compare was not a direct copy of what was in cryptiles.
The one in cryptiles stored the character values in temporary variables - unlike the secure-compare implementation.
How can you fuck up so badly?!?!
Straight-up copy+paste would not have contained that bug!
-
Did anybody else, on reading the thread title, imagine a small purple bird called Pickles?
-
@Steve_The_Cynic Does he tell you to burn things?
-
@coldandtired said in You. Had. ONE Job!:
@Steve_The_Cynic Does he tell you to burn things?
No... I was thinking of Dave Kellett's Pickles...
-
@Steve_The_Cynic I was expecting Angelica, actually.
-
@ScholRLEA said in You. Had. ONE Job!:
@Steve_The_Cynic I was expecting Angelica, actually.