Code review comment for lp://staging/~laurynas-biveinis/percona-server/merge-5.6.17

Revision history for this message
Alexey Kopytov (akopytov) wrote :

Laurynas,

  Changes to lock_rec_other_trx_holds_expl() look okay-ish, though they
  could benefit from the trx descriptors optimization by replacing
  pointers to transactions with their IDs. I don’t insist, since it’s a
  debug-only code.

  But the OpenSSL changes are wrong. It looks like the only issue with
  OpenSSL < 0.9.8l that breaks AES_ENCRYPT/AES_DECRYPT compatibility is
  that those old versions incorrectly report the required IV length with
  AES in ECB mode. IV is not required by the ECB cipher, so
  EVP_CIPHER_iv_length() should return 0, whereas old versions
  return 16.

  Which confuses my_aes_needs_iv(), in particular, and makes the server
  believe AES_*() functions require the 3rd argument with
  block_encryption_mode=aes-128-ecb (i.e. an IV).

  Which is also an upstream issue, i.e. a workaround for old OpenSSL
  libs is required. And the workaround is simple: whenever
  EVP_CIPHER_iv_length() is used to get the required IV length, also
  check EVP_CIPHER_mode(). If it’s EVP_CIPH_ECB_MODE, disregard the
  value returned by EVP_CIPHER_iv_length() and assume the
  required IV length to be 0.

  The approach implemented in this MP is rather dangerous: we make the
  server pretend that it accepts IV when required, when in fact it is
  ignored for old OpenSSL versions. Security folks may get upset :)

  Speaking of the new Openssl_version variable, its only purpose in this
  MP was to patch up the test suite to hide the fact that IV is ignored
  for old OpenSSL versions. Which means there’s no need for it
  anymore. Besides, it is misleading: it shows the OpenSSL version that
  the server was built against. Which, in case of dynamic linking may
  differ from the OpenSSL library actually being used. Which can make
  quite a difference in the light of recent events.

review: Needs Fixing

« Back to merge proposal