Mocha tests not passing on Safari 10

[alexlamsl]

I set up a mocha server according to README.MD and run it on various browsers on the desktop, Chrome on Android all passing grades.

But trying to run it on the latest iOS (tried iPhone SE, iPhone 7 and iPhone 7 Plus) they fail somewhat randomly, in that the same tests don't fail all the time, even on the same phone...

[alexlamsl]

Focusing on just "random" tests alone, I've managed to observe the tests failing most but not all of the time. However, whenever they fail they give the same (wrong) results, and is identical across iPhone 7 and iPad Pro.

[alexlamsl]

Now that Safari 10 is out for OSX/macOS, I can reproduce the issue on a MacBook Pro. So at least we have narrowed this down somewhat (plus I get to do more debugging and less staring at bright screens...)

[alexlamsl]

So I think I'm pretty certain this is a browser bug. If I open their debugger console the arithmetic problem goes away... until the whole browser crashed.

Feel free to close this report as there isn't anything to be fixed in forge IMHO.

[dlongley]

Ok, will close. Hopefully it gets sorted out on their end.

[javierpavon2000]

I'm having the same issue (and #429), pbkdf2 operation result is different in safari IOS10 compared with previous versions and other browsers like chrome.

what was the conclusion? is it a Safari bug?

[dlongley]

what was the conclusion? is it a Safari bug?

Yes, that appears to be the conclusion based on feedback from other people experiencing the issue. Opening the debug console in the browser makes the problem disappear ... there's some odd interaction going on in Safari.

[BSierakowski]

@dlongley I was just about to open an issue through the apple developer program—hopefully we can get some sort of response from them.

[VitorBarbosa]

What is the issue link for this in the apple developer program?

[davidalcoba]

Hello, I'm afraid that this misbehavior is somehow related also to Forge library and not only with Apple.

Find attached a small test in which the same function (pbkdf2 with sha1 and sha256) is called in 3 different crypto libraries: forge, standford and asmcrypto. The failing environments are: MacOS (any with Safari 10) and IOS (10.1 and 10.0.1).

So this is what it is really weird, without calling the derivation function in Forge (unchecking the checkbox) the two others work well in both standford and asmcrypto libraries. But if the derivation is also done in Forge then the results are incorrect for the three of them.

I will continue investigating, but it seems that something used by Forge (and not by the other libraries) has been somehow changed in the Safari webkit. It would be interesting if you guys can take a look and give your opinion because we don't know if Apple considers this a bug, but if not we should start thinking on what are the real differences with the other crypto libraries out there.

Hope it helps!

pbkdf2.zip

[davidalcoba]

Hi again,

I'm afraid that in my previous post I messed up with the implementation of the test so please don't take into consideration. Because of my needs I want to hash the salt before calling the pbkdf2 function, but it seems that I was using the same hash function from Forge for all of them instead of using the appropriate one in each library.

Take a look to this new one instead in which each test only calls to each library independently. The failing environments are the same than mentioned above.

Forge always fails in these environments while the other libraries do it well. What is really weird is that if you open Web Inspector in Safari 10 (just open it, not need to do anything with it) then Forge behaves properly. As stated by @alexlamsl this problem goes away when opening Web Inspector in Safari.

I will continue investigating the issue, but I think that it is worth to keep it opened since a more fundamental problem could be happening in combination with Apple.

Thanks

pbkdf2.zip

[davidlehn]

Yeah, let's leave this open for now since other people will likely have the issue too.

@deivkk: @dlongley and I were playing around with your first code yesterday and reproduced this easily. We were commenting out large swaths of code and it would still appear. Just doing nothing and returning constants in various places can still expose the problem.

Last night I supercharged the example with loops and it's easy to make it always fail at some point. Often the same point. Or actually, the same points. Depending on number of rounds it will transition between a few broken outputs. The same broken outputs every time.

The forge hashing was the reason the other libs were failing. Could just print out the salt hex and see it gets corrupted at some point. So garbage in, garbage out for the other libs. I just updated with hashing from your second code drop. Other libs seem to be stable, forge still fails.

With the simple page reload thing it would fail randomly. But always with the same output (dependent on number of rounds). Doing everything in a loop seems to always have failures at the same point later with the same wrong output. So it's likely this could be reduced to a simpler test case.

Interestingly, you can still log to console, and open it up after your test and see debug info. Though with console open everything works. Since I'm looping it now, you can open console during the test and values will pop back to correct, then fail again once you close the console.

I'd guess this is all due to some code optimization bug in safari that forge code or allocation patterns are exposing. I'll try to debug a bit more and maybe find a quicker more predictable failure code path.

[davidalcoba]

Many thanks for reopening the issue again.

I'd guess this is all due to some code optimization bug in safari that forge code or allocation patterns are exposing.

I am under the same impression.

Anyway I'm finding really difficult to isolate the problem in a super simple test. If you come up with something please let me know so I can provide you with some help.

[davidlehn]

Note that this is still an issue in v16 (oct 26, 2016) of safari preview release: https://developer.apple.com/safari/technology-preview/

[dlongley]

Upon further investigation, we've concluded that this appears to be an optimization bug in Safari 10 that is related to integer signs and/or overflows. The bug only presents itself when hot loops are optimized. It's possible that optimization gets disabled when the console is opened and this would explain why it also is a "fix" for the bug.

We were able to update the forge code to avoid triggering the bug by using some bitwise unsigned right shifts by zero in a few places that should normally be unnecessary. We've marked them as such in comments. We have added a number of tests that demonstrate the problem occurring (which you can see if you use a version of forge older than 0.6.45 on Safari 10 ... these tests now pass on 0.6.45+).

All that remains to do here is come up with a minimal test case to share with Apple. This test case should not require any forge-specific APIs at all. It can probably just be a long running loop that uses the first 16 rounds code from forge's SHA-1 implementation prior to the fix.

[dlongley]

This could probably get more minimal ... but here's a gist without any code from forge (or any other external scripts) that exposes the bug in Safari 10:

https://gist.github.com/dlongley/5b1f4284922ec0deac9abbed5665c7ed

If you put that gist into an html file and run it in Safari, you should see the bug.

Here's the script portion of the html file:

var expected;
var attempts = 5;
for(var x = 0; x < attempts; ++x) {
  if(!_runTest(x, 100)) {
    break;
  }
}

function _runTest(attempt, max) {
  for(var itr = 0; itr < max; ++itr) {
    var s = {
      a: 0,
      b: 0,
      c: 0x80000000, // setting to anything less avoids the bug
      d: 0,
      e: 0
    };
    for(var i = 0; i < 15000; ++i) {
      var t, a, b, c, d, e;

      a = s.a;
      b = s.b;
      c = s.c;
      d = s.d;
      e = s.e;

      for(var j = 0; j < 2; ++j) {
        t = a + e;
        e = d;
        // uncommenting line below with `>>> 0` will avoid the bug
        //c = c >>> 0;
        d = c;
        c = (b >>> 1);
        b = a;
        a = t;
      }

      s.a = a | 0;
      s.b = b | 0;
      s.c = c | 0;
      s.d = d | 0;
      s.e = e | 0;
    }
    // compute the result
    var result = Object.keys(s).reduce(function(a, b) {
      return a + (s[b] >>> 0).toString(16);
    }, '');
    // display and result comparison
    document.getElementById('iteration').innerHTML =
      (attempt + 1) + ', Iteration ' + (itr + 1) + ' / ' + max;
    if(attempt === 0 && itr === 0) {
      expected = result;
      document.getElementById('expected').innerHTML = expected;
    } else if(result !== expected) {
      var output = {iteration: itr, result: result};
      document.getElementById('result').innerHTML =
        ' is ' + output.result + '<br>' +
        '<span style="color: red">FAILURE!</span> ' +
        'Final result does not match first result.';
      return false;
    }
  }

  return true;
}

[davidlehn]

I simplified the test case and took out what I could. Now it will always fail after some random amount of time on Safari 10.0.1 with value assignment alone! There appears to be something deeply wrong with their optimizer.

https://gist.github.com/davidlehn/9297d6306bfef98d4ff5bee43b83e5dd#file-forge-issue-428-check-html

var run = true;
var expected;
var attempt = 0;
var attempts = 5000;
var iterations = 500;
function stop() {
  run = false;
}
function _check() {
  if(run && attempt < attempts) {
    if(_runTest(attempt, iterations)) {
      attempt++;
      setTimeout(_check, 0);
    }
  }
}
setTimeout(_check, 0);

function _runTest(attempt, max) {
  function _status(itr) {
    document.getElementById('iteration').innerHTML =
      (attempt + 1) + ', Iteration ' + (itr + 1) + ' / ' + max;
  }
  for(var itr = 0; itr < max; ++itr) {
    var s = {
      a: 0,
      b: 0,
      c: 1
    };
    var t, a, b, c;

    a = s.a;
    b = s.b;
    c = s.c;

    // loop required.  manually unrolling avoids bug.
    for(var j = 0; j < 2; ++j) {
      t = c;
      c = b;
      // uncommenting line below with `>>> 0` will avoid the bug
      //c = c >>> 0;
      b = a;
      a = t;
    }

    s.a = a;
    s.b = b;
    s.c = c;

    // compute the result
    var result = [s.a, s.b, s.c].join('');
    // display and result comparison
    if((attempt + 1) % 100 === 0) {
      _status(itr);
    }
    if(attempt === 0 && itr === 0) {
      expected = result;
      document.getElementById('expected').innerHTML = expected;
    } else if(result !== expected) {
      var output = {iteration: itr, result: result};
      _status(itr);
      document.getElementById('result').innerHTML =
        ' is ' + output.result + '<br>' +
        '<span style="color: red">FAILURE!</span> ' +
        'Final result does not match first result.';
      return false;
    }
  }

  return true;
}

[alexlamsl]

@dlongley just to say that 0.6.45 works on Safari 10 for my use case, so many thanks!

[davidlehn]

More exciting data:

• Test case reduced to just swapping two vars via a temp var or with destructuring expression. (!!!)
• Can still "fix" with some bit twiddling of a var.
• Fails with latest WebKit/JavaScriptCore builds (as of 2016-10-31).
• Fails with jsc tool from WebKit/JavaScriptCore on mac/linux x86-64.
• Passes on i386 32b linux.

https://gist.github.com/davidlehn/9297d6306bfef98d4ff5bee43b83e5dd#file-forge-issue-428-check-swap-html https://gist.github.com/davidlehn/9297d6306bfef98d4ff5bee43b83e5dd#file-forge-issue-428-check-cli-js

var debug = debug || console.log;
// use values high enough for optimizer to run
// usually fails around the same number of inner loops
var test_max = 50000;
var outer_max = 2000;
// use at least 2 inner loops
var inner_max = 2;

// inner loop count
var count = 0;
var test, outer;
for(test = 0; test < test_max; ++test) {
  _test();
}
debug('PASS');

function _status(status, values) {
  debug(
    status +
    ' test:' + (test + 1) +
    ' outer:' + (outer + 1) + '/' + outer_max +
    ' count:' + count +
    (values ? ' values:' + values.join('') : ''));
}

function _test() {
  for(outer = 0; outer < outer_max; ++outer) {
    // var and let fail in or out of loop
    // moving outside of function works.
    var t, a, b;
    // fails with at least ints and bools
    // also fails when shifting [a,b,...,M,N] = [N,a,b,...,M]
    // simple 2 var swap done here as minimal test case
    a = 1;
    b = 0;

    // at least 2 swaps in an inner loop required.
    // manually unrolling avoids bug.
    for(var inner = 0; inner < inner_max; ++inner) {
      //t = b; b = a; a = t;
      t = b;
      b = a;
      // do one of the following to avoid optimization bug:
      // b = b & -1; // FIX
      // b = b >>> 0; // FIX
      // other operations will not avoid optimization bug:
      // b = b + 0; // FAILS
      // b = b * 1; // FAILS
      a = t;
      // also fails with destructuring expression vs above vars
      //[a,b] = [b,a];
      count++;
    }

    // check
    if((inner % 2 === 0 && !(a === 1 && b === 0)) ||
       (inner % 2 === 1 && !(a === 0 && b === 1))) {
      _status('FAIL', [a, b]);
      throw 'FAIL';
    }
    // progress
    //_status('....', [a, b]);
    if((test + 1) % 100 === 0 && (outer + 1) === outer_max) {
      _status('....', [a, b]);
    }
  }
}

[davidalcoba]

Very exciting indeed. Moreover this issue does not happen in the new beta version of IOS 10.2. However it seemed to be also solved in beta version 10.1 but the official release didn't contain the fix.

[dlongley]

WebKit bug here: https://bugs.webkit.org/show_bug.cgi?id=164309

[davidlehn]

This is now fixed in WebKit. Safari with the nightly WebKit build works fine with the original code and with the workarounds. I expect the workarounds will have to stay in place for a long while to support affected browsers. https://bugs.webkit.org/show_bug.cgi?id=164309 https://trac.webkit.org/changeset/208373