Fix RolloverCount calculation error #146

cszdlt · 2021-06-15T07:02:02Z

Description

Fix unorder interval exceeds maxROCDisorder causing RolloverCount error.

When there is a serious disorder in the network (the disorder segment is greater than maxROCDisorder), the nextRolloverCount will be calculated incorrectly.Will cause srtp decryption to fail.
This PR fixed this problem.

Imitate libsrtp/crypto/replay/rdbx.c: srtp_index_guess

Sean-Der · 2021-07-05T14:32:46Z

Thank you so much @cszdlt

@at-wat mind giving this a look over as well? It looks ok to me, but it is a sensitive area would love your input.

Sean-Der · 2021-07-05T14:33:19Z

@cszdlt This is a really great contribution, would you be interested in joining the Pion organization? Would love to know what we could improve, and would love your help.

cszdlt · 2021-07-05T15:11:17Z

I am a beginner with limited ability😅, but I am very happy to be able to help.

at-wat · 2021-07-05T15:37:20Z

I'll take a look tomorrow.

codecov · 2021-07-06T01:11:30Z

Codecov Report

Merging #146 (057a9d8) into master (d6718e5) will increase coverage by 0.50%.
The diff coverage is 100.00%.

❗ Current head 057a9d8 differs from pull request most recent head 8ec90ad. Consider uploading reports for the commit 8ec90ad to get more accurate results

@@            Coverage Diff             @@
##           master     #146      +/-   ##
==========================================
+ Coverage   75.56%   76.06%   +0.50%     
==========================================
  Files          16       16              
  Lines         757      773      +16     
==========================================
+ Hits          572      588      +16     
  Misses        103      103              
  Partials       82       82

Flag	Coverage Δ
go	`76.06% <100.00%> (+0.50%)`	⬆️
wasm	`75.54% <100.00%> (+0.51%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
context.go	`88.76% <100.00%> (+2.46%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d6718e5...8ec90ad. Read the comment docs.

at-wat · 2021-07-06T01:15:16Z

https://github.com/cisco/libsrtp/blob/master/crypto/replay/rdbx.c

at-wat · 2021-07-06T01:25:04Z

context.go

-			roc++
+	seq := int32(sequenceNumber)
+	localRoc := uint32(s.index >> 16)
+	localSeq := int32(s.index & 0xffff)


Suggested change

localSeq := int32(s.index & 0xffff)

localSeq := int32(s.index & (seqNumMax - 1))

just to make it consistent with https://github.com/pion/srtp/pull/146/files#diff-552f47512a00afe5fc6850cc9ddc830a6daeca162750e50aab4ed549685e0253R192
(seqNumMax - 1 is evaluated during compilation)

context.go

at-wat · 2021-07-06T01:39:12Z

context.go

+	guessRoc := localRoc
+	var difference int32 = 0
+
+	if s.rolloverHasProcessed {


https://github.com/cisco/libsrtp/blob/bd0f27ec0e299ad101a396dde3f7c90d48efc8fc/crypto/replay/rdbx.c#L309-L325
I think above condition (s.index > seqNumMedian) should be checked here to avoid wrong guess after initialization and ROC overflow.

Yes, this judgment needs to be added.

@at-wat Sorry, I forgot to run the check script. This is my first PR... T_T

context.go

s.index > seqNumMedian seems have to be checked

at-wat · 2021-07-06T14:05:41Z

@Sean-Der generate-authors workflow doesn't work for forked PRs since secrets can't be expanded on the forked PRs for security.

Also, passing personal access token of the bot to the third-party action is not good at security unless action version is specified by commit hash.
(actions/checkout persists given token to the disk and stefanzweifel/git-auto-commit-action is using it to push to the repository.)
It is what I wrote in pion/.goassets#63 (comment).

at-wat

LGTM, sorry for taking too long.

Mind adding you to AUTHORS.txt by manually running .github/generate-authors.sh script?

cszdlt · 2021-07-22T02:51:41Z

LGTM, sorry for taking too long.

Mind adding you to AUTHORS.txt by manually running .github/generate-authors.sh script?

it's okay no problem.
What should I do? @at-wat

at-wat · 2021-07-22T03:02:18Z

Running

GITHUB_WORKSPACE=$(pwd) .github/generate-authors.sh

will update AUTHORS.txt based on the commit history.
Please commit AUTHORS.txt and push.

cszdlt · 2021-07-22T03:18:23Z

Running
GITHUB_WORKSPACE=$(pwd) .github/generate-authors.sh 
will update AUTHORS.txt based on the commit history.
Please commit AUTHORS.txt and push.

The email I set on github is different from my local one, and two entries will be added after running the script... like this:

 backkem <mail@backkem.me>
 chenkaiC4 <chenkaic4@gmail.com>
 Chris Hiszpanski <chris@hiszpanski.name>
+cszdlt <1106543196@qq.com>
+cszdlt <cszdlt@qq.com>
 Hugo Arregui <hugo.arregui@gmail.com>
 Jerko Steiner <jerko.steiner@gmail.com>
 Juliusz Chroboczek <jch@irif.fr>

what should I do?

at-wat · 2021-07-22T03:36:08Z

git rebase -i master

will open an editor to select action for each commit. Change pick to edit of the commits which you want to fix the commit author.
Then,

git commit --amend --reset-author
git rebase --continue

until processing all selected commits. It will update the commit author info to your current git config.

at-wat · 2021-07-22T03:37:38Z

Or it may be easier to squash all commits in this PR.

Fix Unorder interval exceeds maxROCDisorder causing RolloverCount error.

cszdlt · 2021-07-22T04:45:24Z

Or it may be easier to squash all commits in this PR.

Is it like this? :)

at-wat · 2021-07-22T04:48:21Z

Perfect! Thank you so much!

Sean-Der requested review from at-wat and Sean-Der July 5, 2021 14:33

at-wat previously approved these changes Jul 6, 2021

View reviewed changes

cszdlt force-pushed the fix_rolloverr_count branch from 20ce78c to 55d0a2a Compare July 6, 2021 09:45

at-wat previously approved these changes Jul 22, 2021

View reviewed changes

cszdlt dismissed at-wat’s stale review via f0dcd1c July 22, 2021 04:30

cszdlt force-pushed the fix_rolloverr_count branch from 057a9d8 to f0dcd1c Compare July 22, 2021 04:30

Fix RolloverCount calculation error

8ec90ad

Fix Unorder interval exceeds maxROCDisorder causing RolloverCount error.

cszdlt force-pushed the fix_rolloverr_count branch from cce0529 to 8ec90ad Compare July 22, 2021 04:40

at-wat approved these changes Jul 22, 2021

View reviewed changes

at-wat merged commit ffddef1 into pion:master Jul 22, 2021

at-wat mentioned this pull request Jul 22, 2021

Post request to update AUTHORS.txt on forked repo pion/.goassets#72

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix RolloverCount calculation error #146

Fix RolloverCount calculation error #146

cszdlt commented Jun 15, 2021 •

edited

Loading

Sean-Der commented Jul 5, 2021

Sean-Der commented Jul 5, 2021

cszdlt commented Jul 5, 2021

at-wat commented Jul 5, 2021

codecov bot commented Jul 6, 2021 •

edited

Loading

at-wat commented Jul 6, 2021

at-wat Jul 6, 2021

at-wat Jul 6, 2021 •

edited

Loading

cszdlt Jul 6, 2021

cszdlt Jul 6, 2021

at-wat commented Jul 6, 2021

at-wat left a comment

cszdlt commented Jul 22, 2021

at-wat commented Jul 22, 2021

cszdlt commented Jul 22, 2021

at-wat commented Jul 22, 2021

at-wat commented Jul 22, 2021

cszdlt commented Jul 22, 2021

at-wat commented Jul 22, 2021

	localSeq := int32(s.index & 0xffff)
	localSeq := int32(s.index & (seqNumMax - 1))

Fix RolloverCount calculation error #146

Fix RolloverCount calculation error #146

Conversation

cszdlt commented Jun 15, 2021 • edited Loading

Description

Sean-Der commented Jul 5, 2021

Sean-Der commented Jul 5, 2021

cszdlt commented Jul 5, 2021

at-wat commented Jul 5, 2021

codecov bot commented Jul 6, 2021 • edited Loading

Codecov Report

at-wat commented Jul 6, 2021

at-wat Jul 6, 2021

Choose a reason for hiding this comment

at-wat Jul 6, 2021 • edited Loading

Choose a reason for hiding this comment

cszdlt Jul 6, 2021

Choose a reason for hiding this comment

cszdlt Jul 6, 2021

Choose a reason for hiding this comment

at-wat commented Jul 6, 2021

at-wat left a comment

Choose a reason for hiding this comment

cszdlt commented Jul 22, 2021

at-wat commented Jul 22, 2021

cszdlt commented Jul 22, 2021

at-wat commented Jul 22, 2021

at-wat commented Jul 22, 2021

cszdlt commented Jul 22, 2021

at-wat commented Jul 22, 2021

cszdlt commented Jun 15, 2021 •

edited

Loading

codecov bot commented Jul 6, 2021 •

edited

Loading

at-wat Jul 6, 2021 •

edited

Loading