Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:93429
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain paragonie.com designates 209.85.218.53 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <dcb7462b660c9552b85c706968729b7f@k-piste.dy.fi>
References: <dcb7462b660c9552b85c706968729b7f@k-piste.dy.fi>
Date: Sun, 22 May 2016 18:24:15 -0400
Message-ID: <CAKws9z0SAg2HSQ3nuOfBvNcB6czLwKmsRV6DKbMwXSGRja6sgg@mail.gmail.com>
To: PHP Internals <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] base64_decode is buggy, what to fix?
From: scott@paragonie.com (Scott Arciszewski)

On Sun, May 22, 2016 at 6:56 AM, Lauri Kentt=C3=A4 <lauri.kentta@gmail.com>=
 wrote:
> Hello, Internals!
>
> I was fixing #72152 when it became apparent that the base64_decode functi=
on
> is very buggy.
>
>
> - Null byte ends processing.
>
> - "V" produces empty result, while "V=3D" fails. Not very logical.
>
> - Too short padding is allowed, e.g. "VV=3D" works like "VV=3D=3D".
>
> - Extra padding is allowed (like "V=3D=3D=3D=3D=3D").
>
> - Invalid padding is allowed ("=3DVVV=3D", "VV=3DV=3D", "VVV=3D=3D") exce=
pt on the
> second place of a 24-bit run ("V=3DVV=3D" fails).
>
> - In strict mode, space between padding fails:
> "V V=3D=3D" and "VV =3D=3D" and "VV=3D=3D " are allowed,
> "VV=3D =3D" fails.
>
> - In strict mode, after a padding, one character is skipped, so "VVV=3DV"
> decodes to "UU" (should be "UUU"), and "VVVV=3D*" decodes to "UUU" instea=
d of
> failing.
>
>
> For each of the above, what would be the preferred behaviour in default m=
ode
> and strict mode?
>
>
> Affected existing tests:
>
> - ext/openssl/tests/bug61124.phpt uses "kzo w2RMExUTYQXW2Xzxmg=3D=3D" as =
an
> invalid base64 string, based on the invalid padding.
>
> - ext/standard/tests/file/stream_rfc2397_006.phpt tests
> "#Zm9vYmFyIGZvb2Jhcg=3D=3D" and excepts this to be valid, while "#" is cl=
early
> not valid base64. This also raises a question whether fragments should be
> skipped in data uri handling.
>
>
> Suggestions?
>
> I've created a bug-for-bug compatible rewrite of base64_decode [1], with =
all
> the bugs neatly and specifically implemented and missing features comment=
ed
> out, so it's now very simple to fix them one by one.
>
> I've also attached a test script that tests "all" possible combinations o=
f
> data, padding, NUL and other invalid characters, and my first patch indee=
d
> provides identical results to the old implementation.
>
>
> Currently interesting lines in the test results:
> 'base64'         'default'       'strict'
> 'V'              ''              ''
> 'V=3D'             (false)         (false)
> 'VV=3D'            'U'             'U'
> 'VV=3D=3D'           'U'             'U'
> 'V=3D=3D=3D=3D=3D'         (false)         (false)
> '=3DVVV=3D'          'UU'            (false)
> 'VV=3DV=3D'          'UU'            (false)
> 'VVV=3D=3D'          'UU'            'UU'
> 'V=3DVV=3D'          (false)         (false)
> 'V V=3D=3D'          'U'             'U'
> 'VV =3D=3D'          'U'             'U'
> 'VV=3D=3D '          'U'             'U'
> 'VV=3D =3D'          'U'             (false)
> 'VVV=3DV'          'UUU'           'UU'
> 'VVVV=3D*'         'UUU'           'UUU'
> 'VVVVVV=3DV'       'UUUUU'         'UUUU'
> 'VVVVVV=3D*'       'UUUU'          'UUUU'
> 'VVVV=3D=3D=3D*'       'UUU'           'UUU'
> 'VVV=3D=3D=3D=3DV'       'UUU'           'UU'
> 'VVV=3D=3D=3D=3D*'       'UU'            'UU'
> 'VV=3D=3D=3D=3D=3DV'       'UU'            'U'
> 'VV=3D=3D=3D=3D=3D*'       'U'             'U'
> '=3D=3D=3D=3D=3D=3D=3D*'       ''              ''
>
> --
> Lauri Kentt=C3=A4
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php

I'm surprised to see that these functions weren't heavily tested.

I've included some of the RFC 4648 test vectors in my
cache-timing-safe userland implementation at
https://github.com/paragonie/constant_time_encoding -- maybe this
would be useful for base64_decode() as well?

Scott Arciszewski
Chief Development Officer
Paragon Initiative Enterprises <https://paragonie.com>