Ensure correct error handling for large MPIs

[ni4]

Description

While OpenPGP standard allows MPI lengths up to 64k bits, we allow only smaller ones.
Construct dummy key, signature and PKESK data files and make sure those are loaded with correct error reporting, without AV and range-check failures.
Separate test file should be added to rnp_tests suite.

[ni4]

@killy631 would you like to take this issue? Cannot assign it to you until commented.

[killy631]

OK, I will take

[killy631]

Is there failed or demo?

[killy631]

I meant - failed test in some suite..

[ni4]

@killy631 Nope, there is no one (yet) - the idea is to build the test which will make sure that library works well in such cases (i.e. doesn't crash, has unexpected behavior and so on, and just reports an error).

[ronaldtse]

@ni4 so @killy631 has just dropped off. This task is now open again.

[ronaldtse]

@antonsviridenko is going to take this for trial. Welcome!

[antonsviridenko]

ok, I've started working on it

[antonsviridenko]

Which library functions are supposed to be tested? Only ones exposed in public API inside include/ directory or internal ones from src/librepgp? I used https://github.com/rnpgp/rnp/blob/master/src/tests/streams.cpp#L444 as a reference, but maybe I am moving into the wrong direction?

antonsviridenko@459a4a3

[ni4]

@antonsviridenko while all MPI load is done via get_packet_body_mpi(), single key load will not be good enough to check the whole - we should be sure that later on, if some code will be changed, nothing will fail.
So I'd suggest loading of all key algorithms, with misc key lengths (PGP_MPINT_BITS + 1 should be handled as well as 64k bits).
Another thing which comes to my mind to check - large unknown curve oid.
Basically, pick all edge cases which you think would be worth checking to be protected against buffer overflow/memory overflow/other types of attacks.

[antonsviridenko]

Keyrings in src/tests/data/keyrings directory, they should be considered read-only, am I right? For example, if I want to test importing keys of PGP_MPINT_BITS size, I should use some temporary keyring created in runtime, correct?

[ni4]

@antonsviridenko yeah, those are used in multiple tests, and breaking them will most likely break those tests. Feel free to create separate folder for your data files named according to your test's name.

[antonsviridenko]

Another thing which comes to my mind to check - large unknown curve oid.

What packet type can contain these oid-s? Can you provide some more details? Seems that RFC4880 mentions OID-s only once, in context of hash algorithms.

[ni4]

@antonsviridenko please see sections 5.6.4-5.6.6 of RFC 4880 bis -08. Basically, each EC-related key packet contains curve OID.

[antonsviridenko]

ok, I think I've probably found some bug in dumping signature packet:

1. I made this signature file using 16384 bit RSA key:

https://github.com/antonsviridenko/rnp/blob/master/src/tests/data/test_large_MPIs/rsa-pub-65535bits.pgp.sig

2. Displaying this signature packet, everything seems fine:

#make a copy for later modifications in hex redactor
$ cp rsa-pub-65535bits.pgp.sig rsa-pub-65535bits.pgp.16385sig.sig

$ rnp --list-packets rsa-pub-65535bits.pgp.16385sig.sig 
:off 0: packet header 0xc2c779 (tag 2, len 2105)
Signature packet
    version: 4
    type: 0 (Signature of a binary document)
    public key algorithm: 1 (RSA (Encrypt or Sign))
    hash algorithm: 8 (SHA256)
    hashed subpackets:
        :type 33, len 21
        issuer fingerprint: 0x9a5afe506e9f6bcffc85167894352b599eefa567 (20 bytes)
        :type 2, len 4
        signature creation time: 1574789429 (Tue Nov 26 21:30:29 2019)
        :type 3, len 4
        signature expiration time: 0 (never)
    unhashed subpackets:
        :type 16, len 8
        issuer key ID: 0x94352b599eefa567
    lbits: 0x7dd5
    signature material:
        rsa s: 16383 bits

3. Now I've opened hex redactor and changed signature MPI size at offset 0x3A from 16383 (0x3FFF) to 16384(0x4000):

$ rnp --list-packets rsa-pub-65535bits.pgp.16385sig.sig 
[get_packet_body_mpi() /home/odsk/Ribose/rnp/src/librepgp/stream-packet.cpp:512] wrong mpi bit count
:off 0: packet header 0xc2c779 (tag 2, len 2105)
Signature packet
    version: 4
    type: 0 (Signature of a binary document)
    public key algorithm: 1 (RSA (Encrypt or Sign))
    hash algorithm: 8 (SHA256)
    hashed subpackets:
        :type 33, len 21
        issuer fingerprint: 0x9a5afe506e9f6bcffc85167894352b599eefa567 (20 bytes)
        :type 2, len 4
        signature creation time: 1574789429 (Tue Nov 26 21:30:29 2019)
        :type 3, len 4
        signature expiration time: 0 (never)
    unhashed subpackets:
        :type 16, len 8
        issuer key ID: 0x94352b599eefa567
    lbits: 0x7dd5
    signature material:
        rsa s: 0 bits

It displays error about wrong bit count, but still continues displaying packet contents. For RSA public key packets I tested before, it refused to display them when some inconsistency is detected.
Also see rsa s: 0 bits. However, GnuPG is still able to verify this signature without any warnings:

gpg: Signature made Tue 26 Nov 2019 09:30:29 PM +04
gpg:                using RSA key 9A5AFE506E9F6BCFFC85167894352B599EEFA567
gpg: Good signature from "RSA (Encrypt or Sign) 16384-bit key <odsk@localhost>" [unknown]
gpg: WARNING: This key is not certified with a trusted signature!
gpg:          There is no indication that the signature belongs to the owner.
Primary key fingerprint: 9A5A FE50 6E9F 6BCF FC85  1678 9435 2B59 9EEF A567

4. Changed signature MPI size to 16385(0x4001):

$ rnp --list-packets rsa-pub-65535bits.pgp.16385sig.sig 
[get_packet_body_mpi() /home/odsk/Ribose/rnp/src/librepgp/stream-packet.cpp:498] too large mpi
:off 0: packet header 0xc2c779 (tag 2, len 2105)
Signature packet
    version: 4
    type: 0 (Signature of a binary document)
    public key algorithm: 1 (RSA (Encrypt or Sign))
    hash algorithm: 8 (SHA256)
    hashed subpackets:
        :type 33, len 21
        issuer fingerprint: 0x9a5afe506e9f6bcffc85167894352b599eefa567 (20 bytes)
        :type 2, len 4
        signature creation time: 1574789429 (Tue Nov 26 21:30:29 2019)
        :type 3, len 4
        signature expiration time: 0 (never)
    unhashed subpackets:
        :type 16, len 8
        issuer key ID: 0x94352b599eefa567
    lbits: 0x7dd5
    signature material:
        rsa s: 0 bits

too large MPI but packet contents are still displayed, with 0 bits signature size

5. Now 1 random byte is appended to the file so 16385 bit MPI now has correct expected number of octets:

$ cat /dev/urandom | head -c1 >> rsa-pub-65535bits.pgp.16385sig.sig
$ rnp --list-packets ../src/tests/data/test_large_MPIs/rsa-pub-65535bits.pgp.16385sig.sig 
[get_packet_body_mpi() /home/odsk/Ribose/rnp/src/librepgp/stream-packet.cpp:498] too large mpi
[stream_peek_packet_hdr() /home/odsk/Ribose/rnp/src/librepgp/stream-packet.cpp:611] pkt header read failed

I suspect it should not search for a new packet header here, because now with 1 byte appended signature packet file should be complete. 16385 bit MPI should have 2049 octets containing integer.

6. After appending one more (excess) random byte, it displays expected messages for invalid packet:

$ cat /dev/urandom | head -c1 >> rsa-pub-65535bits.pgp.16385sig.sig
$ rnp --list-packets rsa-pub-65535bits.pgp.16385sig.sig 

[get_packet_body_mpi() /home/odsk/Ribose/rnp/src/librepgp/stream-packet.cpp:498] too large mpi
[stream_dump_packets_raw() /home/odsk/Ribose/rnp/src/librepgp/stream-dump.cpp:1239] failed to process packet

[antonsviridenko]

Seems that signature packet parser goes off by 1 octet somewhere

[ni4]

@antonsviridenko Thanks for finding this out. Confirmed - there is an error in the function stream_parse_signature_body() - on successful signature_read_v4() call variable res contains RNP_SUCCESS, which is not changed later on if get_packet_body_mpi() failed.
Could you please fix it (as well as add test which will ensure correct behavior in this case)?

Regarding MPIs - RFC tells that MPI bit size must correspond to the MPI contents, and we are strictly following this. Probably GnuPG processes such MPIs without error or warning to be compatible with other buggy implementations.
And we do not process 16385-bit MPI since we define maximum MPI size as 16384.

[antonsviridenko]

ok, I've tried to fix it
antonsviridenko@4146406
but now test "rnp_tests.test_ffi_op_verify_sig_count" segfaults somewhere.

What binary should I attach gdb to for debugging? It's bit not clear in these ctests :)

[ni4]

@antonsviridenko for segfault errors usually it is faster to build with sanitizers, so you'll get call stack and a lot of other useful information on failure.
This can be done in Travis container via the following:

./travis.sh
env BUILD_MODE=sanitize GPG_VERSION=stable RNP_TESTS=rnp_tests ci/local.sh

For debugging I used Visual Studio Code and attached to rnp_tests executable. That required some setup of launch.json, like the following:

        {
            "name": "Launch rnp_tests",
            "type": "cppdbg",
            "request": "launch",
            "program": "${workspaceFolder}/../rnp-build/src/tests/rnp_tests",
            "args": ["--gtest_filter=rnp_tests.rnpkeys_generatekey_testSignature", "--gtest_break_on_failure"],
            "stopAtEntry": false,
            "cwd": "${workspaceFolder}/../rnp-build/src/tests/",
            "environment": [{"name": "RNP_TEST_DATA", "value" : "${workspaceFolder}/src/tests/data"}],
            "externalConsole": false,
            "MIMode": "lldb"
        }

[antonsviridenko]

https://github.com/rnpgp/rnp/blob/master/src/librepgp/stream-parse.cpp#L824

I guess returning RNP_SUCCESS here is not an intended action, RNP_ERROR_BAD_FORMAT should be an appropriate error code, am I right?

[ni4]

@antonsviridenko no, it is correct - function returns RNP_SUCCESS in case when signature processed and stream can continue. In this case just one of the signatures will be marked as unknown (for instance, it has newer version, unknown public key/hash algorithm, and so on).

[antonsviridenko]

I got some memory leak report from CI tests,

https://github.com/antonsviridenko/rnp/runs/385090505

[stream_parse_pk_sesskey() /home/runner/work/rnp/rnp/src/librepgp/stream-packet.cpp:1130] failed to get rsa m
[       OK ] rnp_tests.test_large_mpi_rsa_priv (106087 ms)
[----------] 1 test from rnp_tests (106087 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (106087 ms total)
[  PASSED  ] 1 test.

=================================================================
==24845==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 33048 byte(s) in 1 object(s) allocated from:
    #0 0x522c68  (/home/runner/local-builds/rnp-build/src/tests/rnp_tests+0x522c68)
    #1 0x7f6abe6fb2cc  (/home/runner/local-installs/rnp-install/lib/librnp-0.so.0+0x15c2cc)
    #2 0x7f6abe74bcbe  (/home/runner/local-installs/rnp-install/lib/librnp-0.so.0+0x1accbe)
    #3 0x7f6abe749594  (/home/runner/local-installs/rnp-install/lib/librnp-0.so.0+0x1aa594)
    #4 0x7f6abe7486a7  (/home/runner/local-installs/rnp-install/lib/librnp-0.so.0+0x1a96a7)
    #5 0x7f6abe7fe280  (/home/runner/local-installs/rnp-install/lib/librnp-0.so.0+0x25f280)
    #6 0x856586  (/home/runner/local-builds/rnp-build/src/tests/rnp_tests+0x856586)
    #7 0x7f6abde0b77d  (/home/runner/local-builds/rnp-build/lib/libgtestd.so+0x1d577d)
    #8 0x7f6abddb9038  (/home/runner/local-builds/rnp-build/lib/libgtestd.so+0x183038)
    #9 0x7f6abddbb08f  (/home/runner/local-builds/rnp-build/lib/libgtestd.so+0x18508f)
    #10 0x7f6abddbc8c7  (/home/runner/local-builds/rnp-build/lib/libgtestd.so+0x1868c7)
    #11 0x7f6abdddc077  (/home/runner/local-builds/rnp-build/lib/libgtestd.so+0x1a6077)
    #12 0x7f6abde137dd  (/home/runner/local-builds/rnp-build/lib/libgtestd.so+0x1dd7dd)
    #13 0x7f6abdddab73  (/home/runner/local-builds/rnp-build/lib/libgtestd.so+0x1a4b73)
    #14 0x8b1260  (/home/runner/local-builds/rnp-build/src/tests/rnp_tests+0x8b1260)

but checking with valgrind on my machine gives no errors:

valgrind --leak-check=full ctest -R rnp_tests.test_large_mpi_rsa_priv     
==23666== Memcheck, a memory error detector
==23666== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==23666== Using Valgrind-3.15.0.GIT and LibVEX; rerun with -h for copyright info
==23666== Command: ctest -R rnp_tests.test_large_mpi_rsa_priv
==23666== 
Test project /home/odsk/Ribose/rnp/build
    Start 149: setupTestData
1/2 Test #149: setupTestData .......................   Passed    0.05 sec
    Start 110: rnp_tests.test_large_mpi_rsa_priv
2/2 Test #110: rnp_tests.test_large_mpi_rsa_priv ...   Passed   12.11 sec

100% tests passed, 0 tests failed out of 2

Total Test time (real) =  12.65 sec
==23666== 
==23666== HEAP SUMMARY:
==23666==     in use at exit: 0 bytes in 0 blocks
==23666==   total heap usage: 24,060 allocs, 24,060 frees, 3,518,795 bytes allocated
==23666== 
==23666== All heap blocks were freed -- no leaks are possible
==23666== 
==23666== For lists of detected and suppressed errors, rerun with: -s
==23666== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Probably something environment-specific. Any ideas how to solve this?

[ni4]

@antonsviridenko No, it is a real memory leak. centos run gives symbolicated log:

2020-01-11T21:48:35.3820602Z Direct leak of 33048 byte(s) in 1 object(s) allocated from:
2020-01-11T21:48:35.3820979Z     #0 0x5432fb in calloc (/home/rnpuser/local-builds/rnp-build/src/tests/rnp_tests+0x5432fb)
2020-01-11T21:48:35.3821361Z     #1 0x7f0a59c6636c in init_src_common(pgp_source_t*, unsigned long) /__w/rnp/rnp/src/librepgp/stream-common.cpp:338:27
2020-01-11T21:48:35.3821772Z     #2 0x7f0a59cb6d11 in init_encrypted_src(pgp_processing_ctx_t*, pgp_source_t*, pgp_source_t*) /__w/rnp/rnp/src/librepgp/stream-parse.cpp:1870:10
2020-01-11T21:48:35.3822188Z     #3 0x7f0a59cb45d3 in init_packet_sequence(pgp_processing_ctx_t*, pgp_source_t*) /__w/rnp/rnp/src/librepgp/stream-parse.cpp:2175:19
2020-01-11T21:48:35.3822585Z     #4 0x7f0a59cb36d7 in process_pgp_source(pgp_parse_handler_t*, pgp_source_t*) /__w/rnp/rnp/src/librepgp/stream-parse.cpp:2279:15
2020-01-11T21:48:35.3822693Z     #5 0x7f0a59d68fc0 in rnp_decrypt /__w/rnp/rnp/src/lib/rnp.cpp:2732:24
2020-01-11T21:48:35.3823503Z     #6 0x874e10 in rnp_tests_test_large_mpi_rsa_priv_Test::TestBody() /__w/rnp/rnp/src/tests/large-mpi.cpp:113:5
2020-01-11T21:48:35.3824271Z     #7 0x7f0a59378326 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /home/rnpuser/local-builds/rnp-build/src/tests/googletest-src/googletest/src/gtest.cc:2591:14
2020-01-11T21:48:35.3824929Z     #8 0x7f0a59328358 in testing::Test::Run() /home/rnpuser/local-builds/rnp-build/src/tests/googletest-src/googletest/src/gtest.cc:2630:5
2020-01-11T21:48:35.3825568Z     #9 0x7f0a5932a236 in testing::TestInfo::Run() /home/rnpuser/local-builds/rnp-build/src/tests/googletest-src/googletest/src/gtest.cc:2807:11
2020-01-11T21:48:35.3826210Z     #10 0x7f0a5932b9c7 in testing::TestSuite::Run() /home/rnpuser/local-builds/rnp-build/src/tests/googletest-src/googletest/src/gtest.cc:2939:28
2020-01-11T21:48:35.3826865Z     #11 0x7f0a5934a2a7 in testing::internal::UnitTestImpl::RunAllTests() /home/rnpuser/local-builds/rnp-build/src/tests/googletest-src/googletest/src/gtest.cc:5480:44
2020-01-11T21:48:35.3827875Z     #12 0x7f0a59380286 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /home/rnpuser/local-builds/rnp-build/src/tests/googletest-src/googletest/src/gtest.cc:2591:14
2020-01-11T21:48:35.3828537Z     #13 0x7f0a59348f44 in testing::UnitTest::Run() /home/rnpuser/local-builds/rnp-build/src/tests/googletest-src/googletest/src/gtest.cc:5067:10
2020-01-11T21:48:35.3828674Z     #14 0x8cf07c in main /__w/rnp/rnp/src/tests/rnp_tests.cpp:70:12
2020-01-11T21:48:35.3828771Z     #15 0x7f0a57fb8504 in __libc_start_main (/lib64/libc.so.6+0x22504)

And it's source - function init_encrypted_src():

    /* Read the packet-related information */
    errcode = encrypted_read_packet_data(param);
    if (errcode != RNP_SUCCESS) {
        goto finish;
    }

    src->read = param->aead ? encrypted_src_read_aead : encrypted_src_read_cfb;
    src->close = encrypted_src_close;
    src->finish = encrypted_src_finish;
    src->type = PGP_STREAM_ENCRYPTED;

src->close is assigned only after goto finish, so will not be assigned on error. And in src_close() call memory, allocated in init_src_common(), will not be freed.

Thanks for spotting this, and please update PR with this fix.

[antonsviridenko]

here it is
antonsviridenko@4ea3939

Is it ok to close this issue in multiple pull requests, one per key algorithm (i.e. RSA, DSA, EC and so on)? Otherwise it could take a longer time to implement & sync with master branch updates. I think RSA part is completed and can be merged already.

[ronaldtse]

@antonsviridenko sure, this issue can be addressed in multiple PRs. Please go ahead!

[antonsviridenko]

I am looking at DSA now, seems that it makes no sense to try to generate 16384 bit or larger keys.
Basically there should be more complicated set of rules that determine what combination of p and q MPI sizes are acceptable, not just limiting max possible size of one MPI. Maybe it is better to create separate relevant set of tests for DSA related stuff?

https://tools.ietf.org/id/draft-ietf-openpgp-rfc4880bis-08.html#rfc.section.14.6

RFC says
An implementation SHOULD NOT implement DSA keys of size less than 1024 bits. It MUST NOT implement a DSA key with a q size of less than 160 bits. DSA keys MUST also be a multiple of 64 bits, and the q size MUST be a multiple of 8 bits. The Digital Signature Standard (DSS) [FIPS186] specifies that DSA be used in one of the following ways:

1024-bit key, 160-bit q, SHA-1, SHA2--224, SHA2-256, SHA2-384, or SHA2-512 hash
2048-bit key, 224-bit q, SHA2-224, SHA2-256, SHA2-384, or SHA2-512 hash
2048-bit key, 256-bit q, SHA2-256, SHA2-384, or SHA2-512 hash
3072-bit key, 256-bit q, SHA2-256, SHA2-384, or SHA2-512 hash

So it defines key sizes generated by an implementation, but does not explicitly say what p an q sizes CAN be accepted. Now the rnp code does not strictly compare key size to 1024, 2048 or 3072, but checks if it is within range [1024-3072]

[ni4]

@antonsviridenko it would be enough to test that rnp behaves correctly for larger-then-expected DSA key sizes (including too large MPIs of course).

[antonsviridenko]

Looks like generating larger than expected DSA keys requires some efforts. Like patching libbotan in order to remove limitations on p and q max size and then making custom builds. Other possible way is to manually edit DSA key packets in hex editor and increase MPIs. But there are 5 MPIs to be tested per each key (p, q, g, y, x).

Maybe there is some better way to do it?

[ni4]

@antonsviridenko If it takes too much time probably we can skip those cases and switch to other tasks.