verify() doesn't clear underlying OpenSSL errors on failure

dur-randir commented 3 years ago

Consider the following perl program

use Crypt::OpenSSL::X509;
use Crypt::OpenSSL::RSA;
use Net::SSLeay;

my $cert = Crypt::OpenSSL::RSA->new_public_key(Crypt::OpenSSL::X509->new_from_string(<<"EOF")->pubkey());
-----BEGIN CERTIFICATE-----
MIIEpjCCAo4CCQDg0Gknph8e/DANBgkqhkiG9w0BAQsFADAUMRIwEAYDVQQDDAls
b2NhbGhvc3QwIBcNMjAwMjA3MTUyNTQxWhgPMjEyMDAxMTQxNTI1NDFaMBQxEjAQ
BgNVBAMMCWxvY2FsaG9zdDCCAiIwDQYJKoZIhvcNAQEBBQADggIPADCCAgoCggIB
AKXsIUDTlEbTr1A9SuXPyRgw1yzTBKXTctvR0My26QD7ye46qLn2Tif1xynocspr
UqezlMaDdLxuMPhNv7FWDiMYeQzJ7UFem/SPQ/3w0MzrejOQ4zYSWp9ac+Xb6MsK
0z0pdYkpEzTq1H+T+zv5FbbWi4xIXmbKULgNMs2DsKFs3Sl0Ezl/kKGrWNjni6G5
3HfWWNg4UeBzvxqLiNJsTiXF/6AMIv7le/aUhGGu5aFfoeNx0F0lu/jfii/fYFqR
xNqcI8RELexNmSkksxl2XsjtfHTcbVN1GBkSvv+vCg0oTgaU/DZdImEXJXPIrfJ0
ui2JHtv4XdNTHy3MS/fTGys56ycIFtLELpGJI/Bcb1nlSDEVL0XgEEf3KYd9fNQt
oZTxPuWixjwL4S+CPUItGxCTetu1rUvRcXC/IKkrlCTr+ZketB5zzd0n4YOZuq90
TSycSqhFLB9JAEhUzMYvyqteou7CrYAkwTGJGA9twgdSEWeOOpXkxg/IbnoUEeQz
sRBvTfNnRWV5vqy7Pv/8nL6kgnU+tbuuoo80XDMNlOS5LShsf8I2GNGaNyDaz9qz
HtmRR7DOI3AbTROVR22yAmWtRwQ/hrBr/Qr0RcuzJ9gpjQ0kYL4ZCgpowWjsoeP1
17bFayRoG2bVJ8j4MPz8Jcu0Ah8WB6uK8wRumvGFpjWxAgMBAAEwDQYJKoZIhvcN
AQELBQADggIBAEFRIiOLC9/j1s6mR/RdDMhaIQ7a6sWfTmhya5fPokAfDnueRt8J
Dc1LgBAZvrA7Wm20E20zlULKK4Uwq43tJTY5bekZj4s9rDrxKB+EGC5UUHLNTlf/
9ApCuZr+IzcPON8KYXxjLk7aaAZvVMmrvMFFYWTh+gUwckovdB2k5XqmHM8AvAz5
IxnaZtOssbU4JL/r4KYXP8uYMUmUhQFusybNiKs6TJtikhEUOyvcYC5OlcX9xDsv
SojiVtR/EDbQrT2dMHHsA6NYy4JJdLCu5a4++PDEpufK8kmBSr0fyhT+dKiTEtl6
32Ku5WsKFnleoQsb5fLHItar10rAMV6OUdWZsjDHXQyvkkw9Ol3PrTEqmRd9g7KP
HUa//25reBP3ycaifcAXHJS48dmcXdpVqb3Yxz8xQy9wQrq5MNsJw1p5sHkJnqsn
GekplXOzRnlfe8c/abQiRGuKZneu3nvApR8aUK/S78T4m34h5kCuEOEA68u/IjG4
FqVZckVyGVmsFUgLuZZkVCGzonCHd2w1ZUPkjaLOWvB8ZGaby2autOLOJLbDVLFw
3anaPjecPHsz1S5BZhO04IfzrrPamSdSSplvZfpenCfsP5t3D6JQgQ+AEtLUV6YY
tx3+ckPVZTSyisIViUlE1ILy/tZkJ2maKvg9E/iyCu8Yl/nrrKGfN20O
-----END CERTIFICATE-----
EOF

warn 'ERR: '.Net::SSLeay::ERR_get_error()."\n";
unless ($cert->verify('payload', 'abc')) {
   warn "failed\n";
}
warn 'ERR: '.Net::SSLeay::ERR_get_error()."\n";

After the failed verify() call internal OpenSSL error stack is left non-empty with errors in it. That's a problem for end user, since that stack is global and isn't cleared by OpenSSL itself. So if someone peeks into it after the next openssl call, he'll find errors in it, but will assume that it's from theirs call, while in fact they're from verify().

In RSA.xs there's the following block

        case 0:
            CHECK_OPEN_SSL(ERR_peek_error());
            XSRETURN_NO;
            break;

which should presumably handle this situation, but CHECK_OPEN_SSL macro unwraps into

#define CHECK_OPEN_SSL(p_result) if (!(p_result)) croakSsl(__FILE__, __LINE__);
void croakSsl(char* p_file, int p_line)
{
    const char* errorReason;
    /* Just return the top error on the stack */
    errorReason = ERR_reason_error_string(ERR_get_error());
    ERR_clear_error();
    croak("%s:%d: OpenSSL error: %s", p_file, p_line, errorReason);
}

but ERR_peek_error() returns a positive error code, which makes (!p_result) condition to fail, in turn making croakSelf() and thus ERR_clear_error() to not get called.

I propose two possible solutions for this:

change CHECK_OPEN_SSL invocation to CHECK_OPEN_SSL(ERR_peek_error() == 0), making verify() croak on errors instead of returning false
remove CHECK_OPEN_SSL invocation (as it's effectively a no-op here) and call ERR_clear_error() unconditionally instead, retaining return value of verify()

dur-randir commented 3 years ago

@toddr, I can provide a pull request for either of those variants, if you tell which one you think is an appropriate fix for this issue.

toddr commented 3 years ago

I'm open to either solution. Do what you think is the clearest error to the developer?

dur-randir commented 3 years ago

Internally, I've patches this using second approach, as it retains public API of verify().

toddr commented 3 years ago

I look forward to the PR. Thanks!

toddr / Crypt-OpenSSL-RSA

verify() doesn't clear underlying OpenSSL errors on failure #29