Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support a not in-place api's for protect/unprtect #693

Merged
merged 8 commits into from
Jun 12, 2024
Merged

Support a not in-place api's for protect/unprtect #693

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4b84591
change cipher api to support not-in-place io
pabuhler Mar 5, 2024
8c13ee8
introduce not-in-place io api
pabuhler Mar 5, 2024
7bb4e3a
implement not in-place api's for protect/unprotect
pabuhler Mar 25, 2024
00b888d
update wolfssl to support not-in-place io
pabuhler May 5, 2024
0a0dff6
Update api documentation for not in-place io
pabuhler May 23, 2024
327bdc2
add length check of dst buffer to aes icm encrypt
pabuhler May 26, 2024
01b8704
remove unnecessary assumption about the size of dst
pabuhler May 26, 2024
703f2e9
explicitly check dst len is >= tag size
pabuhler Jun 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,7 @@ if(LIBSRTP_TEST_APPS)
${ENABLE_WARNINGS_AS_ERRORS})
target_link_libraries(srtp_driver srtp2)
add_test(srtp_driver srtp_driver -v)
add_test(srtp_driver_not_in_place_io srtp_driver -v -n)

if(NOT (BUILD_SHARED_LIBS AND WIN32))
add_executable(test_srtp test/test_srtp.c)
Expand Down
10 changes: 6 additions & 4 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ described in [RFC 7714](https://tools.ietf.org/html/rfc7714)
supports AES-128 & AES-256, so to use AES-192 or the AES-GCM group of ciphers a
3rd party crypto backend must be configured. For this and performance reasons it
is highly recommended to use a 3rd party crypto backend.

* The `srtp_protect()` function assumes that the buffer holding the
rtp packet has enough storage allocated that the authentication
tag can be written to the end of that packet. If this assumption
Expand Down Expand Up @@ -503,11 +503,13 @@ srtp_create(&session, &policy);
// main loop: get rtp packets, send srtp packets
while (1) {
char rtp_buffer[2048];
size_t len;
size_t rtp_len;
char srtp_buffer[2048];
size_t srtp_len = sizeof(srtp_buffer);

len = get_rtp_packet(rtp_buffer);
srtp_protect(session, rtp_buffer, &len);
send_srtp_packet(rtp_buffer, len);
srtp_protect(session, rtp_buffer, rtp_len, srtp_buffer, &srtp_len);
send_srtp_packet(srtp_buffer, srtp_len);
}
~~~

Expand Down
32 changes: 23 additions & 9 deletions crypto/cipher/aes_gcm_mbedtls.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,8 +281,10 @@ static srtp_err_status_t srtp_aes_gcm_mbedtls_set_aad(void *cv,
* enc_len length of encrypt buffer
*/
static srtp_err_status_t srtp_aes_gcm_mbedtls_encrypt(void *cv,
uint8_t *buf,
size_t *enc_len)
const uint8_t *src,
size_t src_len,
uint8_t *dst,
size_t *dst_len)
{
FUNC_ENTRY();
srtp_aes_gcm_ctx_t *c = (srtp_aes_gcm_ctx_t *)cv;
Expand All @@ -292,16 +294,22 @@ static srtp_err_status_t srtp_aes_gcm_mbedtls_encrypt(void *cv,
return (srtp_err_status_bad_param);
}

errCode = mbedtls_gcm_crypt_and_tag(c->ctx, MBEDTLS_GCM_ENCRYPT, *enc_len,
if (*dst_len < src_len) {
return srtp_err_status_buffer_small;
}

errCode = mbedtls_gcm_crypt_and_tag(c->ctx, MBEDTLS_GCM_ENCRYPT, src_len,
c->iv, c->iv_len, c->aad, c->aad_size,
buf, buf, c->tag_len, c->tag);
src, dst, c->tag_len, c->tag);

c->aad_size = 0;
if (errCode != 0) {
debug_print(srtp_mod_aes_gcm, "mbedtls error code: %d", errCode);
return srtp_err_status_bad_param;
}

*dst_len = src_len;

return (srtp_err_status_ok);
}

Expand Down Expand Up @@ -337,8 +345,10 @@ static srtp_err_status_t srtp_aes_gcm_mbedtls_get_tag(void *cv,
* enc_len length of encrypt buffer
*/
static srtp_err_status_t srtp_aes_gcm_mbedtls_decrypt(void *cv,
uint8_t *buf,
size_t *enc_len)
const uint8_t *src,
size_t src_len,
uint8_t *dst,
size_t *dst_len)
{
FUNC_ENTRY();
srtp_aes_gcm_ctx_t *c = (srtp_aes_gcm_ctx_t *)cv;
Expand All @@ -348,12 +358,16 @@ static srtp_err_status_t srtp_aes_gcm_mbedtls_decrypt(void *cv,
return (srtp_err_status_bad_param);
}

if (*dst_len < (src_len - c->tag_len)) {
return srtp_err_status_buffer_small;
}

debug_print(srtp_mod_aes_gcm, "AAD: %s",
srtp_octet_string_hex_string(c->aad, c->aad_size));

errCode = mbedtls_gcm_auth_decrypt(
c->ctx, (*enc_len - c->tag_len), c->iv, c->iv_len, c->aad, c->aad_size,
buf + (*enc_len - c->tag_len), c->tag_len, buf, buf);
c->ctx, (src_len - c->tag_len), c->iv, c->iv_len, c->aad, c->aad_size,
src + (src_len - c->tag_len), c->tag_len, src, dst);
c->aad_size = 0;
if (errCode != 0) {
return (srtp_err_status_auth_fail);
Expand All @@ -363,7 +377,7 @@ static srtp_err_status_t srtp_aes_gcm_mbedtls_decrypt(void *cv,
* Reduce the buffer size by the tag length since the tag
* is not part of the original payload
*/
*enc_len -= c->tag_len;
*dst_len = (src_len - c->tag_len);

return (srtp_err_status_ok);
}
Expand Down
64 changes: 43 additions & 21 deletions crypto/cipher/aes_gcm_nss.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,8 +281,10 @@ static srtp_err_status_t srtp_aes_gcm_nss_set_aad(void *cv,

static srtp_err_status_t srtp_aes_gcm_nss_do_crypto(void *cv,
bool encrypt,
uint8_t *buf,
size_t *enc_len)
const uint8_t *src,
size_t src_len,
uint8_t *dst,
size_t *dst_len)
{
srtp_aes_gcm_ctx_t *c = (srtp_aes_gcm_ctx_t *)cv;

Expand All @@ -299,13 +301,13 @@ static srtp_err_status_t srtp_aes_gcm_nss_do_crypto(void *cv,
SECItem param = { siBuffer, (unsigned char *)&c->params,
sizeof(CK_GCM_PARAMS) };
if (encrypt) {
rv = PK11_Encrypt(c->key, CKM_AES_GCM, &param, buf, &out_len,
*enc_len + 16, buf, *enc_len);
rv = PK11_Encrypt(c->key, CKM_AES_GCM, &param, dst, &out_len, *dst_len,
src, src_len);
} else {
rv = PK11_Decrypt(c->key, CKM_AES_GCM, &param, buf, &out_len, *enc_len,
buf, *enc_len);
rv = PK11_Decrypt(c->key, CKM_AES_GCM, &param, dst, &out_len, *dst_len,
src, src_len);
}
*enc_len = out_len;
*dst_len = out_len;
srtp_err_status_t status = (srtp_err_status_ok);
if (rv != SECSuccess) {
status = (srtp_err_status_cipher_fail);
Expand All @@ -328,32 +330,41 @@ static srtp_err_status_t srtp_aes_gcm_nss_do_crypto(void *cv,
* enc_len length of encrypt buffer
*/
static srtp_err_status_t srtp_aes_gcm_nss_encrypt(void *cv,
uint8_t *buf,
size_t *enc_len)
const uint8_t *src,
size_t src_len,
uint8_t *dst,
size_t *dst_len)
{
srtp_aes_gcm_ctx_t *c = (srtp_aes_gcm_ctx_t *)cv;

// When we get a non-NULL buffer, we know that the caller is
// prepared to also take the tag. When we get a NULL buffer,
// When we get a non-NULL src buffer, we know that the caller is
// prepared to also take the tag. When we get a NULL src buffer,
// even though there's no data, we need to give NSS a buffer
// where it can write the tag. We can't just use c->tag because
// memcpy has undefined behavior on overlapping ranges.
uint8_t tagbuf[16];
uint8_t *non_null_buf = buf;
if (!non_null_buf && (*enc_len == 0)) {
const uint8_t *non_null_buf = src;
uint8_t *non_null_dst_buf = dst;
if (!non_null_buf && (src_len == 0)) {
non_null_buf = tagbuf;
non_null_dst_buf = tagbuf;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not know why the original code allowed the source buffer to be NULL. But now we have a case where the source buffer could be NULL, but the destination buffer might point to valid memory. It appears this line will cause the output to go into the local tagbuf, even if a valid dst was provided. Is that the wanted effect?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if the src buffer is null, it is an indication that this is being used for authentication only and no data will be encrypted.
The api today does not expect the tag to be written to dst at this point, see #714

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll admit I'm a little confused, but I guess this is an NSS thing more than a libsrtp thing. If there is no source data, what is being authenticated?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in this case the entire packet was pass as aad data previously with srtp_aes_gcm_nss_set_aad()

*dst_len = sizeof(tagbuf);
} else if (!non_null_buf) {
return srtp_err_status_bad_param;
}

srtp_err_status_t status =
srtp_aes_gcm_nss_do_crypto(cv, true, non_null_buf, enc_len);
srtp_err_status_t status = srtp_aes_gcm_nss_do_crypto(
cv, true, non_null_buf, src_len, non_null_dst_buf, dst_len);
if (status != srtp_err_status_ok) {
return status;
}

memcpy(c->tag, non_null_buf + (*enc_len - c->tag_size), c->tag_size);
*enc_len -= c->tag_size;
if (*dst_len < c->tag_size) {
return srtp_err_status_bad_param;
}

memcpy(c->tag, non_null_dst_buf + (*dst_len - c->tag_size), c->tag_size);
*dst_len -= c->tag_size;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sets the length in the formal param dst_len to be whatever it was coming in minus the tag size. What if dst_size was 0 or 1, for example? Since I cannot see any data coming out when the src buffer is NULL, is this just indicating how much buffer would remain if a tag had been produced? (I understand a tag is somehow produced for something and stored in c->tag, but I don't see the relationship with that dst parameter when the src is NULL.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right that this is confusing and if we do #714 "consider merging the gcm encrypt and get tag functions of cipher api" once this is merged it will clean all this up alot.

As it is now though, if the call to srtp_aes_gcm_nss_do_crypto() succeeds then dst_len was large enough for the tag and any encrypted data, it then would have been update to be that size. Since currently the tag is not going to be returned from this function it is truncated by tag size. But I agree that a sanity check against tag size would have be good here.

return srtp_err_status_ok;
}

Expand Down Expand Up @@ -387,11 +398,22 @@ static srtp_err_status_t srtp_aes_gcm_nss_get_tag(void *cv,
* enc_len length of encrypt buffer
*/
static srtp_err_status_t srtp_aes_gcm_nss_decrypt(void *cv,
uint8_t *buf,
size_t *enc_len)
const uint8_t *src,
size_t src_len,
uint8_t *dst,
size_t *dst_len)
{
srtp_err_status_t status =
srtp_aes_gcm_nss_do_crypto(cv, false, buf, enc_len);
uint8_t tagbuf[16];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While no less confusing to me than the encrypt() function, it might be good to replicate much of the comments. That said, it would be good if the comments on encrypt() would speak about which "NULL buffer" (src or dst). The When we get a non-NULL buffer doesn't indicate which buffer.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that is an old comment, will update.

uint8_t *non_null_dst_buf = dst;
if (!non_null_dst_buf && (*dst_len == 0)) {
non_null_dst_buf = tagbuf;
*dst_len = sizeof(tagbuf);
} else if (!non_null_dst_buf) {
return srtp_err_status_bad_param;
}

srtp_err_status_t status = srtp_aes_gcm_nss_do_crypto(
cv, false, src, src_len, non_null_dst_buf, dst_len);
if (status != srtp_err_status_ok) {
int err = PR_GetError();
if (err == SEC_ERROR_BAD_DATA) {
Expand Down
26 changes: 17 additions & 9 deletions crypto/cipher/aes_gcm_ossl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,8 +293,10 @@ static srtp_err_status_t srtp_aes_gcm_openssl_set_aad(void *cv,
* enc_len length of encrypt buffer
*/
static srtp_err_status_t srtp_aes_gcm_openssl_encrypt(void *cv,
uint8_t *buf,
size_t *enc_len)
const uint8_t *src,
size_t src_len,
uint8_t *dst,
size_t *dst_len)
{
srtp_aes_gcm_ctx_t *c = (srtp_aes_gcm_ctx_t *)cv;
if (c->dir != srtp_direction_encrypt && c->dir != srtp_direction_decrypt) {
Expand All @@ -304,7 +306,8 @@ static srtp_err_status_t srtp_aes_gcm_openssl_encrypt(void *cv,
/*
* Encrypt the data
*/
EVP_Cipher(c->ctx, buf, buf, *enc_len);
EVP_Cipher(c->ctx, dst, src, src_len);
*dst_len = src_len;

return (srtp_err_status_ok);
}
Expand Down Expand Up @@ -354,8 +357,10 @@ static srtp_err_status_t srtp_aes_gcm_openssl_get_tag(void *cv,
* enc_len length of encrypt buffer
*/
static srtp_err_status_t srtp_aes_gcm_openssl_decrypt(void *cv,
uint8_t *buf,
size_t *enc_len)
const uint8_t *src,
size_t src_len,
uint8_t *dst,
size_t *dst_len)
{
srtp_aes_gcm_ctx_t *c = (srtp_aes_gcm_ctx_t *)cv;
if (c->dir != srtp_direction_encrypt && c->dir != srtp_direction_decrypt) {
Expand All @@ -364,12 +369,15 @@ static srtp_err_status_t srtp_aes_gcm_openssl_decrypt(void *cv,

/*
* Set the tag before decrypting
*
* explicitly cast away const of src
*/
if (!EVP_CIPHER_CTX_ctrl(c->ctx, EVP_CTRL_GCM_SET_TAG, c->tag_len,
buf + (*enc_len - c->tag_len))) {
if (!EVP_CIPHER_CTX_ctrl(
c->ctx, EVP_CTRL_GCM_SET_TAG, c->tag_len,
(void *)(uintptr_t)(src + (src_len - c->tag_len)))) {
return (srtp_err_status_auth_fail);
}
EVP_Cipher(c->ctx, buf, buf, *enc_len - c->tag_len);
EVP_Cipher(c->ctx, dst, src, src_len - c->tag_len);

/*
* Check the tag
Expand All @@ -382,7 +390,7 @@ static srtp_err_status_t srtp_aes_gcm_openssl_decrypt(void *cv,
* Reduce the buffer size by the tag length since the tag
* is not part of the original payload
*/
*enc_len -= c->tag_len;
*dst_len = src_len -= c->tag_len;

return (srtp_err_status_ok);
}
Expand Down
31 changes: 18 additions & 13 deletions crypto/cipher/aes_gcm_wssl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,8 +321,10 @@ static srtp_err_status_t srtp_aes_gcm_wolfssl_set_aad(void *cv,
* enc_len length of encrypt buffer
*/
static srtp_err_status_t srtp_aes_gcm_wolfssl_encrypt(void *cv,
unsigned char *buf,
size_t *enc_len)
const uint8_t *src,
size_t src_len,
uint8_t *dst,
size_t *dst_len)
{
FUNC_ENTRY();
srtp_aes_gcm_ctx_t *c = (srtp_aes_gcm_ctx_t *)cv;
Expand All @@ -333,17 +335,18 @@ static srtp_err_status_t srtp_aes_gcm_wolfssl_encrypt(void *cv,
}

#ifndef WOLFSSL_AESGCM_STREAM
err = wc_AesGcmEncrypt(c->ctx, buf, buf, *enc_len, c->iv, c->iv_len, c->tag,
err = wc_AesGcmEncrypt(c->ctx, dst, src, src_len, c->iv, c->iv_len, c->tag,
c->tag_len, c->aad, c->aad_size);

c->aad_size = 0;
#else
err = wc_AesGcmEncryptUpdate(c->ctx, buf, buf, *enc_len, NULL, 0);
err = wc_AesGcmEncryptUpdate(c->ctx, dst, src, src_len, NULL, 0);
#endif
if (err < 0) {
debug_print(srtp_mod_aes_gcm, "wolfSSL error code: %d", err);
return srtp_err_status_bad_param;
}
*dst_len = src_len;

return (srtp_err_status_ok);
}
Expand Down Expand Up @@ -392,8 +395,10 @@ static srtp_err_status_t srtp_aes_gcm_wolfssl_get_tag(void *cv,
* enc_len length of encrypt buffer
*/
static srtp_err_status_t srtp_aes_gcm_wolfssl_decrypt(void *cv,
unsigned char *buf,
size_t *enc_len)
const uint8_t *src,
size_t src_len,
uint8_t *dst,
size_t *dst_len)
{
FUNC_ENTRY();
srtp_aes_gcm_ctx_t *c = (srtp_aes_gcm_ctx_t *)cv;
Expand All @@ -407,19 +412,19 @@ static srtp_err_status_t srtp_aes_gcm_wolfssl_decrypt(void *cv,
debug_print(srtp_mod_aes_gcm, "AAD: %s",
srtp_octet_string_hex_string(c->aad, c->aad_size));

err = wc_AesGcmDecrypt(c->ctx, buf, buf, (*enc_len - c->tag_len), c->iv,
c->iv_len, buf + (*enc_len - c->tag_len), c->tag_len,
err = wc_AesGcmDecrypt(c->ctx, dst, src, (src_len - c->tag_len), c->iv,
c->iv_len, src + (src_len - c->tag_len), c->tag_len,
c->aad, c->aad_size);
c->aad_size = 0;
#else
err = wc_AesGcmDecryptUpdate(c->ctx, buf, buf, (*enc_len - c->tag_len),
NULL, 0);
err = wc_AesGcmDecryptUpdate(c->ctx, dst, src, (src_len - c->tag_len), NULL,
0);
if (err < 0) {
debug_print(srtp_mod_aes_gcm, "wolfSSL error code: %d", err);
return (srtp_err_status_algo_fail);
}
err = wc_AesGcmDecryptFinal(c->ctx, buf + (*enc_len - c->tag_len),
c->tag_len);
err =
wc_AesGcmDecryptFinal(c->ctx, src + (src_len - c->tag_len), c->tag_len);
#endif
if (err < 0) {
debug_print(srtp_mod_aes_gcm, "wolfSSL error code: %d", err);
Expand All @@ -430,7 +435,7 @@ static srtp_err_status_t srtp_aes_gcm_wolfssl_decrypt(void *cv,
* Reduce the buffer size by the tag length since the tag
* is not part of the original payload
*/
*enc_len -= c->tag_len;
*dst_len = src_len -= c->tag_len;

return (srtp_err_status_ok);
}
Expand Down
Loading
Loading