Issue #256 Seq number incorrectly masked for IV. #259
Conversation
During the IV calculation, the sequence number has it's endianess reversed before clearing the most significant bit, bit 31. On little endian platforms, this results in the incorrect clearing of bit 7. The fix is to clear the most significant bit prior to swapping the byte order. Please note that this change results in incompatibility with libsrtp releases without this change.
srtp/srtp.c
Outdated
| /* The SRTCP index (seq_num) spans bits 0 through 30 inclusive. | ||
| * Bit 31 must be cleared to zero. | ||
| */ | ||
| in.v32[2] = htonl((seq_num & 0x7FFFFFFF)); |
There was a problem hiding this comment.
Minor nit, but the extra parentheses here doesn't really have any effect.
|
Paul,
I agree.
Best Regards,
Mike Loutris
… On February 23, 2017 at 12:25 AM "Paul E. Jones" ***@***.***> wrote:
@paulej approved this pull request.
---------------------------------------------
In srtp/srtp.c #259 (comment) :
> @@ -3202,7 +3202,10 @@ static void srtp_calc_aead_iv_srtcp(srtp_session_keys_t *session_keys, v128_t *i
in.v16[0] = 0;
memcpy(&in.v16[1], &hdr->ssrc, 4); /* still in network order! */
in.v16[3] = 0;
- in.v32[2] = 0x7FFFFFFF & htonl(seq_num); /* bit 32 is suppose to be zero */
+ /* The SRTCP index (seq_num) spans bits 0 through 30 inclusive.
+ * Bit 31 must be cleared to zero.
+ */
+ in.v32[2] = htonl((seq_num & 0x7FFFFFFF));
Minor nit, but the extra parentheses here doesn't really have any effect.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub #259 (review) , or mute the thread https://github.com/notifications/unsubscribe-auth/AYveu0z1UxhgAO9DgdeDakkZyM_I_8K8ks5rfSZrgaJpZM4MJiQA .
|
|
After the fix, isn't this a no-op?
|
|
The new code might be a no-op, though the old code is broken. That said, I don't mind this given that it enforces the constraint. Nonetheless, you're right that the sequence number (as the code is current written) will never be greater than 0x7FFFFFFF. |
|
Let me know if you want the masking removed.
…-Mike
On February 24, 2017 at 1:06 PM "Paul E. Jones" ***@***.***> wrote:
The new code might be a no-op, though the old code is broken. That said, I don't mind this given that it enforces the constraint. Nonetheless, you're right that the sequence number (as the code is current written) will never be greater than 0x7FFFFFFF.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub #259 (comment) , or mute the thread https://github.com/notifications/unsubscribe-auth/AYveu7is4_7yfJfl62RVA23vKLa5HcHsks5rfyoYgaJpZM4MJiQA .
|
|
I prefer keeping as you have it, but no strong preference either way. My reasoning is simply that it make it more explicit and you can easily match this with the RFC. But, I have no strong preference. We could just put a comment out to the right indicating that this value is < 0x8000000 and, thus, the MSB will be 0 as required by the RFC. @JonathanLennox do you have a preference? |
|
Do we use asserts anywhere in libsrtp? This value being greater than 0x7FFFFFFF is a coding error, and if you hit this scenario and just mask it out you'd have a two-time pad problem. |
|
No asserts used (except for some test code: test/rtp_decoder.c). |
|
Should I be concerned that there are no updates to tests? And if I can weigh in on the "should we have a line of code that does nothing, no line of code"? I'd prefer no line of code, since it's clear from this example that it's easy to get a line of code wrong and cause problems. If the only reason for the line of code would be to handle an invalid seq_num passed in, why not have srtp_calc_aead_iv_srtcp return a srtp_err_status_t and then check that status from two places it is called? |
|
@pthatcher More tests would not hurt. However, it would take at least 128 packets before the problem would be detected. Then the problem would disappear after a while and re-appear. Not sure anyone would think to construct a test that could reproduce exactly this issue. There are tests for GCM, but they clearly don't test this condition. The line of code is needed. The only thing not strictly needed (since the value is supposed to be in proper form) is the logical AND. That's all that's in question. |
|
I like Peter's suggestion that rather than have the logical AND, we change srtp_calc_aead_iv_srtcp to return srtp_status_t. If its seq_num parameter is greater than 0x7FFFFFFF, return an error (probably srtp_err_status_key_expired). |
|
I added PR #262 in response to the suggestion made by @JonathanLennox and @pthatcher |
|
I like PR #262. Is the idea to merge it, and then merge this one in the form where we remove the logical AND? That sounds the best option to me. (When I said "no line of code", I should have said "no logical AND"). As for unit tests, could we at least add one that tests that if a seq_num >= 0x80000000 is passed in that we return srtp_err_status_bad_param? |
|
@pthatcher Yeah, that was my thinking. We can merge in either order, though. As for unit tests, we really don't have a framework for that. We could throw something into srtp_validate_gcm() in test/srtp_driver.c, but I'm not very favorable to doing that. I'd prefer if somebody could find time to set up a real unit test framework for all of libsrtp. |
|
Unfortunately, the only way to test this is using the external APIs to actually try to encrypt more than 2^31 RTCP packets and then make sure we get the "key exhausted" error. This would be a useful test, but not a quick one. (The RTCP sequence number isn't an external parameter for encryption; for decryption, it's in a field which is only 31 bits wide.) |
|
@paulej - I was actually discussing that briefly with @pabuhler today. I would be happy to do this, but unsure of what framework it should be. I'd love something that is single header and provides some type of coverage report, but I am fairly unfamiliar with C unit testing frameworks. Should I perhaps make an issue for discussing this? @JonathanLennox - Would implementing a unit test framework and exposing functions in |
|
@thisisG I've heard good things said of Google's test framework. I think it's this one: https://github.com/google/googletest. There is also CTest (https://cmake.org/Wiki/CMake/Testing_With_CTest). I don't know which might be better. I suspect either would allow us to construct unit tests -- even at getting at this function by testing specific values -- without a ton of work. Of course, unit tests are always a bit of work, but I suspect either of these will make things less painful. |
|
@paulej - Please see #265 and see if that kind of simple framework would suffice. @pthatcher - Added a test in the PR mentioned above to check that if seq num exceeds |
|
ACK. Will look at #265. |
…ked_for_little_endian
Now that there is a check that the 31 bit is 0, the masking becomes a noop and can be removed.
|
Hi, This change should now reflect what has been discussed, I have removed the previous "approval" and would appreciate if those involved in the discussion to review the change add approval. Once approved it will be merged. |
|
Note that the |
During the IV calculation, the sequence number has it's endianess reversed before clearing the most significant bit, bit 31. On little endian platforms, this results in the incorrect clearing of bit 7. The fix is to clear the most significant bit prior to swapping the byte order. Please note that this change results in incompatibility with libsrtp releases without this change. This is a backport of cisco/libsrtp#259 and cisco/libsrtp#262 BUG=webrtc:7288 TESTED=Ran all tests using: gn gen out/libsrtp --args=build_libsrtp_tests=true ninja -C out/libsrtp srtp_tests out/libsrtp/srtp_tests/run_all_tests.sh R=kjellander@chromium.org, pthatcher@chromium.org Review-Url: https://codereview.chromium.org/2723863003 .
During the IV calculation, the sequence number has it's endianess
reversed before clearing the most significant bit, bit 31.
On little endian platforms, this results in the incorrect clearing
of bit 7. The fix is to clear the most significant bit prior to
swapping the byte order.
Updated the comment to reflect the high order bit is 31, not 30 and to give a rationale for why the high order bit should be cleared.
Please note that this change results in incompatibility with libsrtp
releases without this change.