Potential inconsistencies in internal error handling

[mfroeschl]

Dear libsrtp team,

I have noticed the following potential inconsistencies when handling srtp_err_status_t results of functions (or macros) such as srtp_auth_update() :

For example, in the function srtp_protect_mki(), the result of srtp_auth_update() is checked and in case of error the function aborts:

        /* run auth func over packet */
        status = srtp_auth_update(session_keys->rtp_auth, (uint8_t *)auth_start,
                                  *pkt_octet_len);
        if (status)
            return status;

(srtp.c#L2335)

In the function srtp_unprotect_mki() however, the result of srtp_auth_update() is not checked and in case of error the function continues:

        /* now compute auth function over packet */
        status = srtp_auth_update(session_keys->rtp_auth, (uint8_t *)auth_start,
                                  *pkt_octet_len - tag_len - mki_size);

        No check?

        /* run auth func over ROC, then write tmp tag */
        status = srtp_auth_compute(session_keys->rtp_auth, (uint8_t *)&est, 4,
                                   tmp_tag);

(srtp.c#L2605)

Can you please evaluate if this is intentional, or an oversight? If it is indeed intentional, please kindly consider making this more obvious, e.g. by adding a comment.

Below is a list of similar locations:

• srtp.c: return value of srtp_auth_start() is sometimes checked, sometimes ignored
• aes_gcm_ossl.c: return value of EVP_CIPHER_CTX_ctrl() is sometimes checked, sometimes ignored
• aes_gcm_ossl.c: return value of EVP_Cipher() is sometimes checked, sometimes ignored
• hmac_ossl.c: return value of HMAC_CTX_init() is ignored
  • (Also, HMAC_CTX_init() seems to be deprecated in OpenSSL 1.1)

Thank you so much for your time!

[pabuhler]

I think it is an oversight. For the unprotect function, the the function should fail regardless as it will compare the output of auth update with the pre calculated value, but for consistency and correctness I think we should add the checks. The return from other calls should also be checked.

The call to HMAC_CTX_init() seams to only be used when OPENSSL_VERSION_NUMBER < 0x10100000L, so it is never used in OpenSSL 1.1 .

Will you create a PR ?

[mfroeschl]

Hello Pascal,

thanks so much for your reply.

Will you create a PR ?

OK, I will give it a shot.

The call to HMAC_CTX_init() seams to only be used when OPENSSL_VERSION_NUMBER < 0x10100000L, so it is never used in OpenSSL 1.1 .

True, I totally missed the surrounding #if.

Also, after checking again it turns out the return type of HMAC_CTX_init() is actually void:

void HMAC_CTX_init(HMAC_CTX *ctx);

(Reference)

So the current implementation is correct, please forget about my comments regarding HMAC_CTX_init().

[mfroeschl]

Hello Pascal,

while working on my PR I have encountered two more questions, could you kindly give me your opinion?

Regarding the return value of EVP_CIPHER_CTX_ctrl()

The official documentation does not mention what the int return value actually means.

However, looking at the code in evp_enc.c, the value 0 seems to indicate an error, e.g. here:

    if (ctx == NULL || ctx->cipher == NULL) {
        EVPerr(EVP_F_EVP_CIPHER_CTX_CTRL, EVP_R_NO_CIPHER_SET);
        return 0;
    }

So in my PR I would treat 0 as error and everything else as success, which seems in line with existing error checks in libsrtp such as here:

    if (!EVP_CIPHER_CTX_ctrl(c->ctx, EVP_CTRL_GCM_SET_IVLEN, 12, 0)) {
        return (srtp_err_status_init_fail);
    }

Regarding the return value of EVP_Cipher()

According to the documentation:

EVP_Cipher() returns the amount of encrypted / decrypted bytes, or -1 on failure, if the flag EVP_CIPH_FLAG_CUSTOM_CIPHER is set for the cipher.
EVP_Cipher() returns 1 on success or 0 on failure, if the flag EVP_CIPH_FLAG_CUSTOM_CIPHER is not set for the cipher.

As far as I can tell, libsrtp does not explicitly set the EVP_CIPH_FLAG_CUSTOM_CIPHER flag, but maybe I am missing something?

In any case, in the cases where libsrtp does check the return value of EVP_Cipher() it seems to treat the return value of EVP_Cipher() as if "EVP_CIPH_FLAG_CUSTOM_CIPHER flag is set", e.g. by checking the amount of encrypted / decrypted bytes, such as here:

    rv = EVP_Cipher(c->ctx, NULL, aad, aad_len);
    if (rv != aad_len) {
        return (srtp_err_status_algo_fail);
    } else {
        return (srtp_err_status_ok);
    }

Or by treating non-zero as error such as here:

    /*
     * Check the tag
     */
    if (EVP_Cipher(c->ctx, NULL, NULL, 0)) {
        return (srtp_err_status_auth_fail);
    }

Looking at the actual code of EVP_Cipher() in evp_lib.c however, it seems to be somewhat more complicated than stated by the documentation:

int EVP_Cipher(EVP_CIPHER_CTX *ctx, unsigned char *out,
               const unsigned char *in, unsigned int inl)
{
    if (ctx->cipher->prov != NULL) {
        /*
         * If the provided implementation has a ccipher function, we use it,
         * and translate its return value like this: 0 => -1, 1 => outlen
         *
         * Otherwise, we call the cupdate function if in != NULL, or cfinal
         * if in == NULL.  Regardless of which, we return what we got.
         */
        int ret = -1;
        size_t outl = 0;
        size_t blocksize = EVP_CIPHER_CTX_block_size(ctx);

        if (ctx->cipher->ccipher != NULL)
            ret =  ctx->cipher->ccipher(ctx->provctx, out, &outl,
                                        inl + (blocksize == 1 ? 0 : blocksize),
                                        in, (size_t)inl)
                ? (int)outl : -1;
        else if (in != NULL)
            ret = ctx->cipher->cupdate(ctx->provctx, out, &outl,
                                       inl + (blocksize == 1 ? 0 : blocksize),
                                       in, (size_t)inl);
        else
            ret = ctx->cipher->cfinal(ctx->provctx, out, &outl,
                                      blocksize == 1 ? 0 : blocksize);

        return ret;
    }

    return ctx->cipher->do_cipher(ctx, out, in, inl);
}

All in all, it seems to me that for EVP_Cipher() it is hard to predict which path will be taken and how the return value should consequently be interpreted. You need to be aware of ctx->cipher->prov and ctx->cipher->ccipher as well as the in param at the time of the function call, and also know the meaning of the return values of ccipher(), cupdate(), cfinal() and do_cipher(), respectively. Maybe that is why the official documentation mentions that:

This function is historic and shouldn't be used in an application, please consider using EVP_CipherUpdate() and EVP_CipherFinal_ex instead

Thus, I'm not confident making any changes to the current error handling of EVP_Cipher() in libsrtp, as I am worried any change may have undesired side effects. I would rather leave this part to someone who has more experience with the intrinsics of EVP_Cipher(), so I will exclude this from the scope of my PR.

[mfroeschl]

@pabuhler Please see PR #498

[mfroeschl]

@pabuhler

Adding the checks for EVP_CIPHER_CTX_ctrl() return values now causes the cipher_driver.c test to fail, see: https://github.com/cisco/libsrtp/pull/498#issuecomment-654597589

The official documentation does not mention what the int return value actually means. However, looking at the code in evp_enc.c, the value 0 seems to indicate an error, e.g. here: So in my PR I would treat 0 as error and everything else as success, which seems in line with existing error checks in libsrtp

It would seem that this assumption was invalid, and further research about the exact meaning of EVP_CIPHER_CTX_ctrl() return value seems to be required. (I really wish this was better documented in OpenSSL...)

What do you think it the best option? Decipher the undocumented return value, or revert the recent changes in aes_gcm_ossl.c?

[pabuhler]

closing as #498 is merged