?
avatar paragonie-scott
paragonie-scott
8 Nov 2015

If mbstring.func_overload is enabled (set to 2, 3, or 7 in php.ini), strlen() and substr() will act as of the input strings are Unicode strings rather than raw binary strings. None of these functions are written to handle this, which can lead to unpredictable results (i.e. with JCrypt::timingSafeCompare(), 32 bytes might be expressible as 8 UTF-8 characters... not exactly outside the realm of brute force attacking).

avatar paragonie-scott paragonie-scott - open - 8 Nov 2015
avatar paragonie-scott
paragonie-scott - comment - 9 Nov 2015

@mbabker
https://github.com/paragonie/random_compat/blob/master/lib/byte_safe_strings.php - This is how we solved this problem in random_compat.

Another equally valid solution is to make Joomla refuse to run if mbstring.func_overload isn't set to 0. But users might complain about that.

avatar mbabker
mbabker - comment - 9 Nov 2015

There's a UTF-8 aware StringHelper class which has compatibility with mbstring (and actually when the library's bootstrap runs checks for the mbstring.func_overload condition). So just on a mild hunch because I really don't know what else to try, the pull request I added changes the strlen() calls to go through the StringHelper and ultimately the UTF-8 compatibility stuff so it should "better" catch this condition.

avatar paragonie-scott
paragonie-scott - comment - 9 Nov 2015

Okay, maybe I'm explaining this wrong. Sorry, this is a really weird edge case.

<?php
var_dump(strlen("\xF0\x9D\x92\xB3"));

Demo:

php -dmbstring.func_overload=0 the_above_script.php
# int(4)
php -dmbstring.func_overload=7 the_above_script.php
# int(1)

What we want in cryptography is the number of bytes, not characters. Our workaround uses mb_strlen() (which is usually meant for handling Unicode), but we explicitly pass '8bit' to the encoding parameter to coerce it to always give us the number of bytes, not characters.

This is a concern unique to cryptography. It's not worth it to change how StringHelper works.

I would, however, recommend adding two variant methods:

  • binStrlen -- number of bytes
  • binSubstr -- a binary substring

Feel free to rip/use these helper methods. (Like Joomla, Halite is GPL, so that shouldn't be a problem!)

avatar PhilETaylor
PhilETaylor - comment - 9 Nov 2015

@paragonie-scott Would you be able to provide a mergeable pull request with the changes you are proposing please? Code speaks louder than comments :-)

avatar mbabker
mbabker - comment - 9 Nov 2015

@PhilETaylor Take a look at #8354

Assuming I'm reading right (who knows, it's getting late and dinner's getting cold), that should do the trick for this specific edge case.

avatar paragonie-scott
paragonie-scott - comment - 9 Nov 2015

Go eat dinner. Health is more important than code.

avatar zero-24 zero-24 - change - 9 Nov 2015
Title
JCrypt - mbstring.func_overload
JCrypt - mbstring.func_overload
avatar zero-24 zero-24 - change - 9 Nov 2015
Labels Added: ?
avatar zero-24 zero-24 - change - 9 Nov 2015
Category Libraries
avatar mbabker
mbabker - comment - 12 Nov 2015

Between having #8354 and #8401 merged, that should help a bit with this condition. If I'm not mistaken, the only thing that should be left to consider this "complete" is reviewing where we're validating string lengths in a way that needs to account for the conditions covered in the two methods from #8354 or the Binary library added by #8401

@joomla/security please assist in this.

avatar paragonie-scott paragonie-scott - change - 13 Nov 2015
Status New Closed
Closed_Date 0000-00-00 00:00:00 2015-11-13 17:58:03
Closed_By paragonie-scott
avatar paragonie-scott paragonie-scott - close - 13 Nov 2015
avatar paragonie-scott paragonie-scott - close - 13 Nov 2015
avatar beat
beat - comment - 14 Nov 2015

:+1: code-review of referenced PRs looks ok to me to address this issue.

Add a Comment

Login with GitHub to post a comment