-
Notifications
You must be signed in to change notification settings - Fork 474
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.
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
Support a not in-place api's for protect/unprtect #693
Changes from 7 commits
4b84591
8c13ee8
7bb4e3a
00b888d
0a0dff6
327bdc2
01b8704
703f2e9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
||
|
@@ -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, ¶m, buf, &out_len, | ||
*enc_len + 16, buf, *enc_len); | ||
rv = PK11_Encrypt(c->key, CKM_AES_GCM, ¶m, dst, &out_len, *dst_len, | ||
src, src_len); | ||
} else { | ||
rv = PK11_Decrypt(c->key, CKM_AES_GCM, ¶m, buf, &out_len, *enc_len, | ||
buf, *enc_len); | ||
rv = PK11_Decrypt(c->key, CKM_AES_GCM, ¶m, 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); | ||
|
@@ -328,8 +330,10 @@ 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; | ||
|
||
|
@@ -339,21 +343,24 @@ static srtp_err_status_t srtp_aes_gcm_nss_encrypt(void *cv, | |
// 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; | ||
*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; | ||
memcpy(c->tag, non_null_dst_buf + (*dst_len - c->tag_size), c->tag_size); | ||
*dst_len -= c->tag_size; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This sets the length in the formal param There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
||
|
@@ -387,11 +394,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]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While no less confusing to me than the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
There was a problem hiding this comment.
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 beNULL
, but the destination buffer might point to valid memory. It appears this line will cause the output to go into the localtagbuf
, even if a validdst
was provided. Is that the wanted effect?There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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()