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).
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.
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 bytesbinSubstr
-- a binary substringFeel free to rip/use these helper methods. (Like Joomla, Halite is GPL, so that shouldn't be a problem!)
@paragonie-scott Would you be able to provide a mergeable pull request with the changes you are proposing please? Code speaks louder than comments :-)
@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.
Go eat dinner. Health is more important than code.
Title |
|
Labels |
Added:
?
|
Category | ⇒ | Libraries |
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.
Status | New | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2015-11-13 17:58:03 |
Closed_By | ⇒ | paragonie-scott |
code-review of referenced PRs looks ok to me to address this issue.
@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.