rnp_op_encrypt_add_password ignores iterations argument

[jpo234]

Description

When specifying the number of iterations for the key derivation function the argument seems to be ignored

Steps to Reproduce

1. specify the number of iterations to be used in rnp_op_encrypt_add_password:

   
   if (rnp_op_encrypt_add_password(encrypt, password, "SHA256", 255, "AES256") != 0) {
       fprintf(stdout, "failed to set password\n");
       rc = 1;
       goto err_out_encrypt;
   }

2. encrypt some data to a file
3. verify the content of the encrypted file:

rnp --list-packets ciphertext.rnp :off 0: packet header 0xc30d (tag 3, len 13) Symmetric-key encrypted session key packet version: 4 symmetric algorithm: 9 (AES-256) s2k specifier: 3 s2k hash algorithm: 8 (SHA256) s2k salt: 0x2cd1000e45aa83ae s2k iterations: 0 (1024) encrypted key: 0x (0 bytes) :off 15: packet header 0xd254 (tag 18, len 84) Symmetrically-encrypted integrity protected data packet

## Expected Behavior

s2k iterations should match the argument

## Actual Behavior

s2k iterations always seems to be 0

[Full source](https://github.com/rnpgp/rnp/files/3911005/enc_main.zip)

[rrrooommmaaa]

Can I take this one?

[ni4]

@rrrooommmaaa This doesn't seem to be an issue but misunderstanding. In RFC 4880 there is a concept of 'iteration bit count', so iteration count is aligned to 2^(10 + bitcount) or something like that. That's what we see in the description - 255 iterations were aligned to zero bit count, which means 1024 iterations.

However, to not just close this, could you please add some CLI tests (using, say, encrypt-then-check-packet-dump approach) which make sure that correct iteration bit count is used? This would be useful to protect against case where some bug in code would cause less-then-expected security for the key derivation function.

[jpo234]

@ni4 @rrrooommmaaa I won't be at my development machine until Monday, but I tried different values for the iterations argument and always got the same s2k iterations number in the dump. I stumbled upon this when comparing the output of GPG (my reference) with my own test program.

If it is determined that the current behavior is correct, I would suggest a better documentation. It currently says:

 * @param iterations number of iterations, used iin key derivation function. Pass 0 for default
 *        value.

[rrrooommmaaa]

@jpo234 iterations number is in the parenthesis. 0 (1024) means 1024 (the minimum). If you pass 1025 in rnp_op_encrypt_add_password(encrypt, password, "SHA256", 1025, "AES256") it would read 1 (1088) Technically, there is a series of allowed values: 1024, 1088, etc... @ni4 I think we should alter the output to display decoded value, possibly like this: 1024 (encoded as 0)

[ni4]

@rrrooommmaaa @jpo234 Agree on displaying actual iteration count first as well as with update of documentation.

[jpo234]

@ni4 @rrrooommmaaa Attached is a gpg encrypted test message (password is test123).

Here is the output from rnp:

rnp --list-packets ciphertext.pgp 
:off 0: packet header 0x8c0d (tag 3, len 13)
Symmetric-key encrypted session key packet
    version: 4
    symmetric algorithm: 9 (AES-256)
    s2k specifier: 3
    s2k hash algorithm: 2 (SHA1)
    s2k salt: 0x49d88734dee8f0f0
    s2k iterations: 255 (65011712)
    encrypted key: 0x (0 bytes)
:off 15: packet header 0xd256 (tag 18, len 86)
Symmetrically-encrypted integrity protected data packet

and from pgpdump:

pgpdump ciphertext.pgp 
Old: Symmetric-Key Encrypted Session Key Packet(tag 3)(13 bytes)
        New version(4)
        Sym alg - AES with 256-bit key(sym 9)
        Iterated and salted string-to-key(s2k 3):
                Hash alg - SHA1(hash 2)
                Salt - 49 d8 87 34 de e8 f0 f0 
                Count - 65011712(coded count 255)
New: Symmetrically Encrypted and MDC Packet(tag 18)(86 bytes)
        Ver 1
        Encrypted data [sym alg is specified in sym-key encrypted session key]
                (plain text + MDC SHA1(20 bytes))

Both agree that the iterations count is 255. This doesn't chime with what has been said so far in this discussion. Clearly 1024 is not the minimum...

[ni4]

@jpo234 please see the https://tools.ietf.org/html/draft-ietf-openpgp-rfc4880bis-08#section-3.7.1 , section 3.7.1.3 for exact encoding rules:

The count is coded into a one-octet number using the following formula:

#define EXPBIAS 6
count = ((Int32)16 + (c & 15)) << ((c >> 4) + EXPBIAS);

So if c == 0, then count will be 16 << 6, i.e. 1024. For c == 255 count will become ( 16 + 15) << (15 + 6) == 31 << 21 == 65011712.

[jpo234]

OK, I just figured out my problem. I misread the output from both rnp and pgpdump. The iterations count is actually 65 million.

I just verified this with the following program derived from the formula in RFC 4880:

#include <cstdint>
#include <iostream>

#define EXPBIAS 6

int
main(int argc, char *argv[])
{
    uint8_t c = 255;
    int32_t count;

    count = ((int32_t)16 + (c & 15)) << ((c >> 4) + EXPBIAS);
    std::cout << count << std::endl;
}

[jpo234]

@ni4

@jpo234 please see the https://tools.ietf.org/html/draft-ietf-openpgp-rfc4880bis-08#section-3.7.1 , section 3.7.1.3 for exact encoding rules:

The count is coded into a one-octet number using the following formula:

#define EXPBIAS 6
count = ((Int32)16 + (c & 15)) << ((c >> 4) + EXPBIAS);

So if c == 0, then count will be 16 << 6, i.e. 1024. For c == 255 count will become ( 16 + 15) << (15 + 6) == 31 << 21 == 65011712.

Yeah, I just figured this out myself.

[ronaldtse]

Regarding displaying additional information, should we use a new ticket or rename this ticket?

[ni4]

@ronaldtse Since this one is mostly discussion I'd create a new one for @rrrooommmaaa.

[ronaldtse]

Perfect. Thanks @ni4 !

[ronaldtse]

@ni4 then should we close this? I noticed we are nearly at 1000 issues! A point to celebrate...

[ni4]

@ronaldtse ...and now we have PR #1000 which will close this issue once merged :-)