christianriesen / base32 Goto Github PK

https://packagist.org/packages/satooshi/php-coveralls says to use https://packagist.org/packages/php-coveralls/php-coveralls instead

In the decode() method, the first step is to check for an empty string:

if (strlen($base32String) == 0) {
    // Gives an empty string
    return '';
}

However, the second step is to strip out any non-base32 characters. If the string to decode is entirely non-base32 characters (granted, a strange thing to pass into a base32 decode method), the string will be left as an empty string. The empty string would then be split into an array here:

$base32Array = str_split($base32String); // $base32String is empty at this point

which sets $base32Array to be an array with a single item, an empty string (see here for the description of this "bug"). This causes the subsequent foreach loop to iterate once, on an empty string. That causes an error in the first line of the foreach loop:

foreach ($base32Array as $str) {
    $char = self::$decode[$str];

as $str here is an empty string and there is no such key in self::$decode.

It seems as though the approach in this library is to silently remove non-base32 characters (unlike the Python standard library, which throws a TypeError if a non Base32 string is passed to decode), so I will submit a Pull Request that should silently handle this problem.

Line 89:
https://github.com/ChristianRiesen/base32/blob/master/src/Base32.php#L89

Currently:
$base32String .= substr(self::$alphabet, $char, 1);

Would be faster as:
$base32String .= self::$alphabet[$char];

(see http://php.net/manual/en/language.types.string.php#language.types.string.substr)

in composer.lock

The lock file is not up to date with the latest changes in composer.json. You may be getting outdated dependencies. Run update to update them.

Posted from SensioLabsInsight

in README.md, line 59

This file ends with no newline character. It won't render properly on a terminal, and it's considered a bad practice. Add a simple line feed as the last character to fix it.

Christian Riesen <[email protected]> http://christianriesen.com

Acknowledgements
----------------

Base32 is mostly based on the work of https://github.com/NTICompass/PHP-Base32

Posted from SensioLabsInsight

... or \Base32\DecodeException

I found this test suite, the negative cases down the bottom are engineered to detect implementation errors we should add

Base32 hex: https://opensource.apple.com/source/tcl/tcl-87/tcl_ext/tcllib/tcllib/modules/base32/base32hex.testsuite.auto.html
Base32: https://opensource.apple.com/source/tcl/tcl-87/tcl_ext/tcllib/tcllib/modules/base32/base32.testsuite.auto.html

On the first, preg_replace should be removed and preg_match & fail used instead. An RFC respecting implementation probably shouldn't accept all that many possibilities, or if so, it could be put behind an options bitmask/whatever and not the default

If non-alphabet characters are ignored, instead of causing rejection
of the entire encoding (as recommended), a covert channel that can be
used to "leak" information is made possible.

https://tools.ietf.org/html/rfc4648#section-12

The rest might take a bit more work :)

Edit: found something else interesting

Similarly, when the base 16 and base 32 alphabets are handled case
insensitively, alteration of case can be used to leak information or
make string equality comparisons fail.

https://github.com/ChristianRiesen/base32/blob/master/src/Base32.php hmm, here we do strtoupper on input. Instead of doing this, we should require the upper case characters be used. The RFC quote here suggests you could make a covert channel to leak information by encoding it by varying the characters which are upper/lower case. I'm fairly sure that's what they mean about ignoring non-alphabet characters too btw

I think we could make this safer - If we see someone mixing case or using lower-case we should fail to decode and mention DECODE_UPPERCASE and DECODE_LOWERCASE in the error. If we tell them which option to use for the case they want we might avoid them using strtoupper/etc and introducing the issue into their own code

Base32::decode(Base32::encode('10')) // 1
Base32::decode(Base32::encode('test130')) // test13

When unpacking the string, the $format used is 'c*' (signed char) - this is the wrong format, it leads to bytes larger than 0x7f being interpreted as negative integers. The correct format is 'C*' (unsigned char).

The tests cannot pick this up because they are limited to the ASCII space (0x00 - 0x7F).

https://www.php.net/supported-versions.php

I think it's probably time to drop support for < 7.0, and maybe go upto 7.2.

It's also probably worth dropping support for HHVM too, as that's diverged into its own thing - https://hhvm.com/blog/2017/09/18/the-future-of-hhvm.html

Should be tested on PHP 7.3, 7.4 and 8.0...

Probably needs a bit of love and updating to go with that too