Several functions in `CryptX::AuthEnc` deal weirdly with non-simple-string plaintext

[dakkar]

Example:

use strict;
use warnings;
use Test::More;
use Crypt::AuthEnc::GCM qw( gcm_encrypt_authenticate );

sub test_one {
    my ($pt) = @_;
    my ($ct, $tag) = gcm_encrypt_authenticate('AES', '0123456789abcdef', '0123456789ab', '', $pt);
    is($ct, 'R');
}

test_one(0);
test_one('0');
done_testing;

The reason is that SvPOK returns a false value when called on a numeric SV, so the line:

if (SvPOK(plaintext)) pt = (unsigned char *) SvPVbyte(plaintext, pt_len);

doesn't call SvPVbyte, and the rest of the code behaves as if it had been passed an undef or an empty string.

Now, we can argue whether passing a number is a sensible thing to do, but at least it should be documented.

Also, SvPOK will return false for blessed references that overload stringification, which are probably a bigger concern.

I would just remove the if, and unconditionally call SvPVbyte, which handles undef and all the other cases just fine. But there may be good reasons not to do that, that I can't see right now.

[Leont]

I would just remove the if, and unconditionally call SvPVbyte, which handles undef and all the other cases just fine.

It may make more sense to use SvOK instead.

[karel-m]

@Leont I understand that using SvOK might be better here; however, how does SvPVbyte(plaintext, pt_len); handle the situation when plaintext is a numeric (not string) SV?

[Leont]

however, how does SvPVbyte(plaintext, pt_len); handle the situation when plaintext is a numeric (not string) SV?

It stringifies the SV, so after that is has both a numeric and a stringy value.

[karel-m]

The thing is that this pattern:

if (SvPOK(plaintext)) pt = (unsigned char *)SvPVbyte(plaintext, pt_len)

is used on many many places inside CryptX.

IIUC changing it to:

if (SvOK(plaintext)) pt = (unsigned char *)SvPVbyte(plaintext, pt_len)

would mean sort of automatic stringification which might sound like a good idea for integers (as mentioned in the original report) but what about a user accidently passing a HASH reference (or other reference) as plaintext parameter - it will be stringified without warning to a string HASH(0xa00003d90).

I am thinking about

if (SvPOK(plaintext) || SvIOK(plaintext)) pt = (unsigned char *)SvPVbyte(plaintext, pt_len)

but it looks to me like an ugly hack which I do not like.

You know, this is a crypto library which is always about raw bytes/octects, all other things like objects, references, unicode strings, numbers/doubles are trouble makers.

[dakkar]

Then clearly document what happens when passing anything that's not a simple string. Or throw an exception if one is passed. But be consistent!

Currently, if I pass anything that's not a simple string, some functions behave like I passed an empty string (AuthEnc), some stringify (Digest):

use Crypt::Digest::SHA1 'sha1_hex';

my $s;
say sha1_hex(0);
say sha1_hex("0");
say sha1_hex(\$s);
say sha1_hex("".\$s);

Since in Perl the general expectation is that if a sub wants a string, it stringifies the input, I believe that AuthEnc should behave like Digest.

[Leont]

I am thinking about

if (SvPOK(plaintext) || SvIOK(plaintext)) pt = (unsigned char *)SvPVbyte(plaintext, pt_len)

but it looks to me like an ugly hack which I do not like.

SvOK(plaintext) && (!SvROK(plaintext) || SvAMAGIC(plaintext))

Would allow all unblessed defined values, as well as blessed values with overloading.

You know, this is a crypto library which is always about raw bytes/octects, all other things like objects, references, unicode strings, numbers/doubles are trouble makers.

They are, but "all numbers are also valid strings" is a very basic assumption throughout the language.