[openssl-dev] [openssl.org #4148] PCKS1 type 1 Padding check error

[Kurt Roeckx via RT]

On Wed, Nov 18, 2015 at 03:24:51PM +0000, Özgan, Tolgahan Jonas via RT wrote:
> Dear List,
> I have found a BUG in the function
> " RSA_padding_check_PKCS1_type_1 "
[...]
> 
> the pointer p is incremented after the check therefore p is always the first octet of the padded string. In the Case of PKCS1 type 1 padding  always p=0, hence the error.
> Notes:
> Changing the check  to
> if ((num != (flen+1)) || (*(++p) != 01))
> results also in a failure since the next check of p expects p to be "0xff" .

So the problem is that at least RSA_padding_check_PKCS1_type_1() and
RSA_padding_check_PKCS1_type_2() are called with the 0 byte
skipped internally in OpenSSL.  This is because it's the result of
a BN calculation, that is turned back into a char array, but it's
not padded to RSA size. So the expected 0 byte is missing as first
character.  And the current functions depend on that behaviour.

When RSA_padding_check_PKCS1_type_2() was updated to be most
constant time, it internally pads it again, and then also checks
that it's starts with the 0 byte.  That commit also suggested to
use BN_bn2bin_padded from BoringSSL.

So we could change it so it works as you expect and that the 0
byte would be included.  This could potentionally break other
applications that try to use those functions, so maybe the 1.1
release might be the right time to change this.

Looking at all sources in Debian I only find libdigidoc that tried
to use RSA_padding_check_PKCS1_type_1 but it's commented out.  For
RSA_padding_check_PKCS1_type_2 I found yubico-piv-tool that has a
comment about needing +1 (and so fails to check the 0), and
mixmaster where like in the openssl internal cases it's a result
of BN_bn2bin().


Kurt