search

DCIT / perl-CryptX

https://metacpan.org/pod/CryptX
Other
35 stars 23 forks source link

regression: 0.081 fails to parse PEMs that 0.080 parsed fine #110

Closed dakkar closed 1 month ago

dakkar commented 2 months ago

version 0.080 happily passed the attached test, 0.081 fails most of it and hangs on the last subtest.

pem-parsing.t

Right now I've added code like this to deal with the problem, I hope you have a better solution:

sub _massage_one_key {
    my ($input_pem) = @_;

    my $was_ref = !!ref($input_pem);

    return $input_pem if $was_ref && ref($input_pem) ne 'SCALAR';

    my $pem = $was_ref ? $$input_pem : $input_pem;

    my ($begin,$body,$end) = $pem =~ /(---+BEGIN[\s\w]+---+)(.+?)(---+END[\s\w]+---+)/;

    return $input_pem unless $begin && $end;

    $body =~ s/\s+//g;
    $body =~ s/\G(.{1,64})/\n$1/g;

    my $output_pem = "${begin}${body}\n${end}";

    return $was_ref ? \$output_pem : $output_pem;
}
karel-m commented 1 month ago

PEM parsing is currently done by libtomcrypt functions. It is possible that the old perl implementation was more relaxed as for the key format.

If I understand correctly you need to load PEM keys like this (without newlines)?

-----BEGIN PUBLIC KEY-----MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE3VU0nT1p5W0zKHDknAgQpsOODuM2/AoZ/6wNqC9AoUCEpQempFg0aBqxleOP0uW0HG1YwCnOF8N0D8Q2RR2mlw==-----END PUBLIC KEY-----

or

-----BEGIN EC PRIVATE KEY-----MHcCAQEEIFF9oAGC6vxNLIU8D+nuvM8ms1QQlPtpGzQTfzEBVB06oAoGCCqGSM49AwEHoUQDQgAE3VU0nT1p5W0zKHDknAgQpsOODuM2/AoZ/6wNqC9AoUCEpQempFg0aBqxleOP0uW0HG1YwCnOF8N0D8Q2RR2mlw==-----END EC PRIVATE KEY-----

or even

-----BEGIN PUBLIC KEY-----
MHcCAQEEIFF9oAGC6vxNLIU8D+nuvM8ms1QQlPtp
GzQTfzEBVB06oAoGCCqGSM49AwEHoUQDQgAE3VU0
nT1p5W0zKHDknAgQpsOODuM2/AoZ/6wNqC9AoUCE
pQempFg0aBqxleOP0uW0HG1YwCnOF8N0D8Q2RR2m
lw==-----END PUBLIC KEY-----

I think we should stick to the standard which AFAIK requires "-----BEGIN LABEL-----" and "-----END LABEL-----" on a separate lines. See https://www.rfc-editor.org/rfc/rfc7468#section-3

ping @sjaeckel

sjaeckel commented 1 month ago

After reading that part of RFC7468 again, I've seen the following line:

The choice of parsing strategy depends on the context of use.

Currently there's only strict parsing implemented and I didn't even think of having support for a relaxed parser when I wrote this. I'm undecided whether that'd be a good example of Postel's Law, where it wouldn't really hurt if we weren't that strict.

And now with my libtomcrypt hat on: Since neither OpenSSL nor OpenSSH accept that relaxed format, I'm pretty sure we're on the safe side with the decision to only support strict parsing.

@dakkar Still I see where you're coming from and I also did something similar in the past, but maybe it's time now to fix the producer of those malformed PEM files? :)

dakkar commented 1 month ago

oh, the producer has mostly been fixed. I would still investigate why a malformed PEM input can make the libtomcrypt PEM parser hang

guimard commented 1 month ago

Hi, did you find the root cause of this issue ? I've the bug with a well formed PEM also (64 char per line)

#!/usr/bin/perl

#use Crypt::JWT qw(encode_jwt);
use Crypt::PK::RSA;

my $val = '-----BEGIN RSA PRIVATE KEY-----
MIIEowIBAAKCAQEAs2jsmIoFuWzMkilJaA8//5/T30cnuzX9GImXUrFR2k9EKTMt
GMHCdKlWOl3BV+BTAU9TLz7Jzd/iJ5GJ6B8TrH1PHFmHpy8/qE/S5OhinIpIi7eb
ABqnoVcwDdCa8ugzq8k8SWxhRNXfVIlwz4NH1caJ8lmiERFj7IvNKqEhzAk0pyDr
8hubveTC39xREujKlsqutpPAFPJ3f2ybVsdykX5rx0h5SslG3jVWYhZ/SOb2aIzO
r0RMjhQmsYRwbpt3anjlBZ98aOzg7GAkbO8093X5VVk9vaPRg0zxJQ0Do0YLyzkR
isSAIFb0tdKuDnjRGK6y/N2j6At2HjkxntbtGQIDAQABAoIBADYq6LxJd977LWy3
0HT9nboFPIf+SM2qSEc/S5Po+6ipJBA4ZlZCMf7dHa6znet1TDpqA9iQ4YcqIHMH
6xZNQ7hhgSAzG9TrXBHqP+djDlrrGWotvjuy0IfS9ixFnnLWjrtAH9afRWLuG+a/
NHNC1M6DiiTE0TzL/lpt/zzut3CNmWzH+t19X6UsxUg95AzooEeewEYkv25eumWD
mfQZfCtSlIw1sp/QwxeJa/6LJw7KcPZ1wXUm1BN0b9eiKt9Cmni1MS7elgpZlgGt
xtfGTZtNLQ7bgDiM8MHzUfPBhbceNSIx2BeCuOCs/7eaqgpyYHBbAbuBQex2H61l
Lcc3Tz0CgYEA4Kx/avpCPxnvsJ+nHVQm5d/WERuDxk4vH1DNuCYBvXTdVCGADf6a
F5No1JcTH3nPTyPWazOyGdT9LcsEJicLyD8vCM6hBFstG4XjqcAuqG/9DRsElpHQ
yi1zc5DNP7Vxmiz9wII0Mjy0abYKtxnXh9YK4a9g6wrcTpvShhIcIb8CgYEAzGzG
lorVCfX9jXULIznnR/uuP5aSnTEsn0xJeqTlbW0RFWLdj8aIL1peirh1X89HroB9
GeTNqEJXD+3CVL2cx+BRggMDUmEz4hR59meZCDGUyT5fex4LIsceb/ESUl2jo6Sw
HXwWbN67rQ55N4oiOcOppsGxzOHkl5HdExKidycCgYEAr5Qev2tz+fw65LzfzHvH
Kj4S/KuT/5V6He731cFd+sEpdmX3vPgLVAFPG1Q1DZQT/rTzDDQKK0XX1cGiLG63
NnaqOye/jbfzOF8Z277kt51NFMDYhRLPKDD82IOA4xjY/rPKWndmcxwdob8yAIWh
efY76sMz6ntCT+xWSZA9i+ECgYBWMZM2TIlxLsBfEbfFfZewOUWKWEGvd9l5vV/K
D5cRIYivfMUw5yPq2267jPUolayCvniBH4E7beVpuPVUZ7KgcEvNxtlytbt7muil
5Z6X3tf+VodJ0Swe2NhTmNEB26uwxzLe68BE3VFCsbSYn2y48HAq+MawPZr18bHG
ZfgMxwKBgHHRg6HYqF5Pegzk1746uH2G+OoCovk5ylGGYzcH2ghWTK4agCHfBcDt
EYqYAev/l82wi+OZ5O8U+qjFUpT1CVeUJdDs0o5u19v0UJjunU1cwh9jsxBZAWLy
PAGd6SWf4S3uQCTw6dLeMna25YIlPh5qPA6I/pAahe8e3nSu2ckl
-----END RSA PRIVATE KEY----- ';

# Same issue with this
# $val =~ s/\n/\r\n/g;

my $key = \$val;

my $obj = Crypt::PK::RSA->new($key);
print "Done\n";
guimard commented 1 month ago

Maybe the best could be to revert libtomcrypt integration until this library becomes stable ?

sjaeckel commented 1 month ago

Could you please verify that https://github.com/libtom/libtomcrypt/pull/672 fixes those issues?

guimard commented 1 month ago

Could you please verify that libtom/libtomcrypt#672 fixes those issues?

Hi, confirmed, this fix the issue :1st_place_medal:

karel-m commented 1 month ago

I have released CryptX-0.082_001 with the libtomcrypt fix.

We still do not parse/load these three cases:

# single line:
-----BEGIN PUBLIC KEY-----MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE3VU0nT1p5W0zKHDknAgQpsOODuM2/AoZ/6wNqC9AoUCEpQempFg0aBqxleOP0uW0HG1YwCnOF8N0D8Q2RR2mlw==-----END PUBLIC KEY-----

# tall multi-line
-----BEGIN PUBLIC KEY-----

MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE3VU0nT1p5W0zKHDknAgQpsOODuM2

/AoZ/6wNqC9AoUCEpQempFg0aBqxleOP0uW0HG1YwCnOF8N0D8Q2RR2mlw==

-----END PUBLIC KEY-----

# weird multi-line
-----BEGIN PUBLIC KEY-----
MHcCAQEEIFF9oAGC6vxNLIU8D+nuvM8ms1QQlPtp
GzQTfzEBVB06oAoGCCqGSM49AwEHoUQDQgAE3VU0
nT1p5W0zKHDknAgQpsOODuM2/AoZ/6wNqC9AoUCE
pQempFg0aBqxleOP0uW0HG1YwCnOF8N0D8Q2RR2m
lw==-----END PUBLIC KEY-----
dakkar commented 1 month ago

thank you, the parser no longer hangs! I can deal with malformed PEMs from here ☺

karel-m commented 1 month ago

OK, closing.

h3kker commented 1 week ago

Sorry to possibly re-open: we receive a public key from an API as a JSON string without newlines.

one example: -----BEGIN PUBLIC KEY-----MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEoKkQvXKsWSgAaRb+kLvFlvR486jKGF0UwEaxhUINcTbKD50CSC80zRi5MBVdPb6A+SOiitWgxd5IwpCYFOIqoA----END PUBLIC KEY-----

Obviously this stopped working after updating the module. I've read through the issue and try to massage the the key into something conforming to the standard, but so far I only get FATAL: invalid or unsupported EC key format

My massaged PEM looks like this:

-----BEGIN PUBLIC KEY-----
MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEoKkQvXKsWSgAaRb+kLvFlvR486jKGF0UwEaxhUINcTbKD50CSC80zRi5MBVdPb6A+SOiitWgxd5IwpCYFOIqoA
-----END PUBLIC KEY-----

do you have any hints about whats wrong? or would you consider accepting keys without newlines? it's not that it'd be impossible to send newlines in a json string, but maybe it's common in that context?

sjaeckel commented 1 week ago

My massaged PEM looks like this:

-----BEGIN PUBLIC KEY-----
MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEoKkQvXKsWSgAaRb+kLvFlvR486jKGF0UwEaxhUINcTbKD50CSC80zRi5MBVdPb6A+SOiitWgxd5IwpCYFOIqoA
-----END PUBLIC KEY-----

do you have any hints about whats wrong?

the line length is too long. PEM lines are per rfc7468 max. 64chars long.