You. Had. ONE Job!

svieira

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.

Jaloopa

@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!

ben_lubar

subtle package - crypto/subtle - Go Packages

How do you fuck up a function that simple?

svieira

For three versions no less!

bb36e

@svieira version 3.0.1

:wtf:

Dragnslcr

@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?

Jaloopa

@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

@svieira

0_1472229865810_Screen Shot 2016-08-26 at 18.44.07.png

Oopsie indeed.

But wouldn't unit tests have caught this?

secure-compare/test.js at master · vadimdemedes/secure-compare

Constant-time comparison algorithm to prevent timing attacks - vadimdemedes/secure-compare

/**
 * 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.

boomzilla

So...the lesson here is that no one ever actually used this thing?

Maciejasjmj

@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.

sloosecannon

@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));
}

CreatedToDislikeThis

I don't see any problem - its job was secure compare, not bug-free compare - doing both would violate the Single Responsibility principle.

Jaloopa

@sloosecannon Exactly. Turns out I was pretty close to what the actual tests are

Yamikuronue

That was going to bother me all day.

Improved tests by yamikuronue · Pull Request #2 · vadimdemedes/secure-compare

Hi there! I am a QA professional and I occasionally like to suggest test improvements for open-source packages. I saw that PR #1 didn't bother updating the test cases, so I thought I'd lend...

Maciejasjmj

@Yamikuronue you're not covering the "different in UTF8" case? Especially given the whole UTF equivalence mess...

svieira

@Yamikuronue said in You. Had. ONE Job!:

That was going to bother me all day.

:-D That is awesome!

Yamikuronue

@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

@Yamikuronue said in You. Had. ONE Job!:

readme.md

Huh, TIL that works.

CreatedToDislikeThis

@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...

pydsigner

@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....

aliceif

The funny thing is:
According to their npm package site, they copied that function from a different library called cryptiles:

npm: secure-compare

Securely compare two strings, copied from cryptiles. Latest version: 3.0.1, last published: 9 years ago. Start using secure-compare in your project by running `npm i secure-compare`. There are 71 other projects in the npm registry using secure-compare.

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!

Steve_The_Cynic

Did anybody else, on reading the thread title, imagine a small purple bird called Pickles?

coldandtired

@Steve_The_Cynic Does he tell you to burn things?

Steve_The_Cynic

@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...

ScholRLEA

@Steve_The_Cynic I was expecting Angelica, actually.

Dreikin

@ScholRLEA said in You. Had. ONE Job!:

@Steve_The_Cynic I was expecting Angelica, actually.

Angelica - Breaking My Heart (Official Video) – 04:13
— Frontiers Music srl