2

I've been watching a thread on the OpenSSL mailing list. The thread is titled CBC ciphers + TLS 1.0 protocol does not work in OpenSSL 1.0.2d.

OpenSSL 1.0.2d had intermittent problems due to the following. It showed up under Microsoft's WinCE compiler. The idea is to propagate the high bit to all other bits:

#define DUPLICATE_MSB_TO_ALL(x) ( (unsigned)( (int)(x) >> (sizeof(int)*8-1) ) )
#define DUPLICATE_MSB_TO_ALL_8(x) ((unsigned char)(DUPLICATE_MSB_TO_ALL(x)))

static unsigned char constant_time_eq_8(unsigned a, unsigned b)
{
    unsigned c = a ^ b;
    c--;
    return DUPLICATE_MSB_TO_ALL_8(c);
}

OpenSSL attempts to follow C89. I believe this is implementation defined behavior due to shifting of a negative value on a 2's compliment machine.

However, what the OP found was it was affected by optimizations. Without optimizations the code produced correct results. With optimizations the code produced incorrect results.

My question is, when relying on implementation defined behavior, is it legal or expected to see results change depending on optimizations?

Community
  • 1
  • 1
jww
  • 97,681
  • 90
  • 411
  • 885
  • "I believe this is" - there are no signed shifts in the code you've posted. – Karoly Horvath Dec 15 '15 at 21:15
  • @KarolyHorvath - my bad... I pulled the wrong snippet out of the postings (its a long thread). It has been updated. – jww Dec 15 '15 at 21:18
  • OpenSSL is full of horrible code like this .. imho the sooner it's abandoned the better. Who knows how many vulnerabilites are lurking due to this sort of junk – M.M Dec 15 '15 at 21:22
  • @M.M: Maybe it was commented out or something? The asterisks are present in the linked mailing list message. – user2357112 Dec 15 '15 at 21:23
  • 1
    Possibly a compiler bug where the optimizer used the host machine's `>>` definition , not the target machine's one... hard to say just based on this though – M.M Dec 15 '15 at 21:27
  • @M.M - in fairness to OpenSSL... the project is improving code and processes. In fact, the reason this issue surfaced is because the code was cleaned up to avoid what you are talking about. No good deed goes unpunished :) – jww Dec 15 '15 at 21:31
  • Fair enough. Maybe I'm bitter because I once submitted a patch to fix code that leaked memory in Windows but not Linux, and it got rejected with the comment "that should never be `free()`d". So now I just use git to apply my patch to new versions on my development systems only. – M.M Dec 15 '15 at 21:32
  • @M.M. - no big deal. I think we have all felt the frustration over the years. The good thing is, now that the project has funding from the Linux Foundation (and others), they have hired full-time developers and are moving on a lot of the past complaints. They even cleaned up their source code format. My eyes no longer bleed when starring at it :) – jww Dec 15 '15 at 21:38

1 Answers1

2

Implementation-defined means that the implementation must document the behaviour:

implementation-defined behavior

unspecified behavior where each implementation documents how the choice is made

So, consult the documentation for which compiler you have observed the unexpected results on. The documentation may or may not specify the behaviour you observed.

If the observed behaviour differs from the documentation then confirm it with a SSCCE and file a bug report. If the documentation is not present then file a bug report for missing required documentation.

M.M
  • 138,810
  • 21
  • 208
  • 365
  • Thanks @M.M. So I am clear: if Microsoft documents the behavioral change based on optimizations, then its legal. Is that correct? (I don't think its expected; but expected and legal are two different things). – jww Dec 15 '15 at 21:32
  • 1
    It's legal. It is not uncommon (though often unexpected) that different optimization levels change implementation defined or undefined behavior. – mpez0 Dec 15 '15 at 21:35