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

pabuhler · 2024-03-25T08:46:51Z

The protect/unprotect api's can now operate both in-place or not in-place, depending on the requirements of the caller.
The length of the out buffer can now be checked to ensure there is sufficient space.
This addresses #322 "provide an new version of protect function that does not corrupt memory" and #569 "Please add a "non inplace" protect and unprotect API"

srtp_driver has been updated so all existing test can be run either in-place or not in-place.

Document new api and error codes
Update examples to use new api
Create separate PR's for all clean up / refactoring commits
Follow up with test cleanup and reuse of CHECK macros

pabuhler · 2024-03-25T08:48:44Z

This PR is set to draft state as it is still a work in progress and can potentially be force updated with breaking changes.

crypto/cipher/aes_gcm_nss.c

paulej · 2024-04-25T19:23:54Z

crypto/cipher/aes_gcm_nss.c

        non_null_buf = tagbuf;
+        non_null_dst_buf = tagbuf;


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?

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

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?

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

crypto/cipher/aes_icm.c

crypto/cipher/null_cipher.c

include/srtp.h

test/srtp_driver.c

A layer to the srtp driver test is added to be able to run the tests both in-place & not-in-place without duplicating the test. The not-in-place functions are currently a minimal implementation using a memcpy.

The protect/unprotect api's can now operate both in-place or not in-place, depending on the requirements of the caller. The length of the out buffer can now be checked to ensure there is sufficient space. Tests are add to verify validation of the output buffer length.

Also update the minimal code example in README.md to reflect the new api.

With the new api the full length of dst is now passed in, if it is too small it should fail.

paulej · 2024-06-11T14:46:38Z

crypto/cipher/aes_gcm_nss.c

        non_null_buf = tagbuf;
+        non_null_dst_buf = tagbuf;


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?

paulej · 2024-06-11T15:00:58Z

crypto/cipher/aes_gcm_nss.c

-    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;


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.)

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.

paulej · 2024-06-11T15:02:15Z

crypto/cipher/aes_gcm_nss.c

 {
-    srtp_err_status_t status =
-        srtp_aes_gcm_nss_do_crypto(cv, false, buf, enc_len);
+    uint8_t tagbuf[16];


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.

I guess that is an old comment, will update.

This is a safety check to ensure we do not wrap dst len when removing tag size. All of this code would get much cleaner if the tag could just be returned instead of cached. see cisco#714

pabuhler marked this pull request as draft March 25, 2024 08:47

pabuhler closed this Mar 25, 2024

pabuhler reopened this Mar 25, 2024

This was referenced Mar 25, 2024

align behavior when using keystream prefix #694

Merged

change rtp hdr xtn api to work with const / non const values #695

Merged

pabuhler added this to the Version 3.0 milestone Mar 28, 2024

paulej reviewed Apr 25, 2024

View reviewed changes

pabuhler force-pushed the not-in-place-api branch from 7f7a1a0 to fa42772 Compare April 26, 2024 10:44

pabuhler mentioned this pull request Apr 26, 2024

consistently use mki size from srtp_stream_ctx_t #703

Merged

pabuhler force-pushed the not-in-place-api branch from fa42772 to 8b50c31 Compare April 30, 2024 20:22

pabuhler added 4 commits May 6, 2024 08:36

change cipher api to support not-in-place io

4b84591

introduce not-in-place io api

8c13ee8

A layer to the srtp driver test is added to be able to run the tests both in-place & not-in-place without duplicating the test. The not-in-place functions are currently a minimal implementation using a memcpy.

update wolfssl to support not-in-place io

00b888d

pabuhler force-pushed the not-in-place-api branch from 1a414fc to 00b888d Compare May 6, 2024 06:37

pabuhler added 3 commits May 23, 2024 22:15

Update api documentation for not in-place io

0a0dff6

Also update the minimal code example in README.md to reflect the new api.

add length check of dst buffer to aes icm encrypt

327bdc2

remove unnecessary assumption about the size of dst

01b8704

With the new api the full length of dst is now passed in, if it is too small it should fail.

pabuhler marked this pull request as ready for review May 27, 2024 05:32

paulej approved these changes Jun 11, 2024

View reviewed changes

explicitly check dst len is >= tag size

703f2e9

This is a safety check to ensure we do not wrap dst len when removing tag size. All of this code would get much cleaner if the tag could just be returned instead of cached. see cisco#714

pabuhler merged commit 24eec55 into cisco:main Jun 12, 2024
39 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

pabuhler commented Mar 25, 2024 •

edited

Loading

pabuhler commented Mar 25, 2024

paulej Apr 25, 2024

pabuhler May 26, 2024

paulej Jun 11, 2024

pabuhler Jun 11, 2024

paulej Jun 11, 2024

paulej Jun 11, 2024

pabuhler Jun 11, 2024

paulej Jun 11, 2024

pabuhler Jun 11, 2024

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

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

Conversation

pabuhler commented Mar 25, 2024 • edited Loading

pabuhler commented Mar 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pabuhler commented Mar 25, 2024 •

edited

Loading