rnpgp / rnp

RNP: high performance C++ OpenPGP library used by Mozilla Thunderbird
https://www.rnpgp.org
Other
192 stars 54 forks source link

Encryption and decryption of zero length message raise error #2173

Open shemeshg opened 6 months ago

shemeshg commented 6 months ago

Description

I encountered an error when trying to encrypt and decrypt a zero length message using the rnp library. The steps to reproduce the error are as follows

Steps to Reproduce

  1. Run the generate.c example from https://github.com/rnpgp/rnp/blob/main/src/examples/generate.c
  2. Run the encrypt.c example from https://github.com/rnpgp/rnp/blob/main/src/examples/encrypt.c

Ensure ("Encryption succeeded. Encrypted message written to file encrypted.asc")

  1. Change line 42 of encrypt.c from const char * message = "RNP encryption sample message"; to const char * message = "";
  2. Run the encrypt.c example again and see the error message: failed to create input object
  3. Create an empty file with touch myEmptyFile
  4. Encrypt the empty file with rnp or gnupg
  5. Decrypt the encrypted file to memory and see the error message: failed to create input object

Expected Behavior

I expected the encryption and decryption of a zero length message to succeed without any error, or at least to fail gracefully with a meaningful error message.

Actual Behavior

The encryption and decryption of a zero length message failed with a cryptic error message: failed to create input object

More info

Notice that the error happens when encrypt from buffer, ot decrypt to buffer. File to file, is working fine.

ni4 commented 6 months ago

rnp_input_from_memory() explicitly disallows zero-size input (however we should not this in documentation as well):

rnp_result_t
rnp_input_from_memory(rnp_input_t *input, const uint8_t buf[], size_t buf_len, bool do_copy)
try {
    if (!input || !buf) {
        return RNP_ERROR_NULL_POINTER;
    }
    if (!buf_len) {
        return RNP_ERROR_SHORT_BUFFER;
    }

Do you have any practical need to encrypt/sign zero-size messages?

shemeshg commented 6 months ago

The exception does not necessarily bubbles up as RNP_ERROR_SHORT_BUFFER
so I'm not sure catching RNP_ERROR_SHORT_BUFFER is sufficient.

especially on

rnp_output_memory_get_buf(*output.getOutput(), &buf, &buf_len, false);

after rnp_op_verify_execute .

shemeshg commented 6 months ago

I've been thinking maybe rnp_output_to_callback would be a walk-around.

for example:

class RnpStreamToMemory
{
public:
    RnpStreamToMemory() { rnp_output_to_callback(&output, stream_out_writer, NULL, this); }

    const rnp_output_t *getOutput() const { return &output; }

    virtual ~RnpStreamToMemory() { rnp_output_destroy(output); }

    static bool stream_out_writer(void *app_ctx, const void *buf, size_t len)
    {
        RnpStreamToMemory *rnpStreamToMemory = static_cast<class RnpStreamToMemory *>(app_ctx);

        rnpStreamToMemory->oss.write((const char *) buf, len);
        return !rnpStreamToMemory->oss.fail();
    }

    const std::string getOutStr() const
    {
        //
        return oss.str();
    }

private:
    rnp_output_t output = NULL;
    std::ostringstream oss;
};

Though the callback work as expected, the rnp_output_memory_get_buf returns "bad parameter" error code

 RnpStreamToMemory output{};
rnp_output_memory_get_buf(*output.getOutput(), &buf, &buf_len, false);

not sure how to tell the rnp_output_memory_get_buf that the operation was successful.

The return value of the static bool stream_out_writer is used only to notify if further iterations required. and not indicate success

ni4 commented 6 months ago

I think would be logical to update our code to support zero-size input buffer in rnp_input_from_memory() for users convenience.

Though the callback work as expected, the rnp_output_memory_get_buf returns "bad parameter" error code

@shemeshg Yeah, this function should be used only with output, created via rnp_output_to_memory() as other outputs do not have memory buffer.

shemeshg commented 6 months ago

Thanks,

  1. the rnp_output_to_memory solved the reading from file to memory.
  2. Notice the problem is also writing zero len to file.

in continue to the walk-around ofrnp_output_to_memory this operation (other way round) also fail Error reading file

RnpStreamFromMemory input{inStr};
...
...
if (rnp_op_encrypt_create(&encrypt, ffiRaII->getFfi(), *input.getInput(), *output.getOutput())
        != RNP_SUCCESS) {
        throw std::runtime_error("failed to create encrypt operation\n");
    }
..
...
...
// HERE WILLL FAIL
rnp_op_encrypt_execute(encrypt)

for

class RnpStreamFromMemory
{
public:
    RnpStreamFromMemory(const std::string &inStr)
        : iss{inStr}
    {
        rnp_input_from_callback(&input, stream_in_reader, NULL, this);
    }

    const rnp_input_t *getInput() const { return &input; }

    virtual ~RnpStreamFromMemory() { rnp_input_destroy(input); }

    static bool stream_in_reader(void *app_ctx, void *buf, size_t len, size_t *readres)
    {
        RnpStreamFromMemory *rnpStreamFromMemory = static_cast<class RnpStreamFromMemory *>(app_ctx);
        rnpStreamFromMemory->iss.read((char *) buf, len);
        if (rnpStreamFromMemory->iss.fail()) {
            return false;
        }
        *readres = rnpStreamFromMemory->iss.gcount();
        return true;
    }

private:
    rnp_input_t input = NULL;
    std::istringstream iss;
};

with error of Error reading file