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.

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

Add replay attack protection #211

Merged
merged 1 commit into from
Mar 8, 2020
Merged

Add replay attack protection #211

merged 1 commit into from
Mar 8, 2020

Conversation

at-wat
Copy link
Member

@at-wat at-wat commented Mar 6, 2020

Implements RFC 6347 Section 4.1.2.6.
Set config.ReplayProtectionWindow to change the size of the protection window.
Default is 64.

Reference issue

Fixes #146

@at-wat
Copy link
Member Author

at-wat commented Mar 6, 2020

CI failure on WASM will be fixed by #210.

@codecov
Copy link

codecov bot commented Mar 6, 2020

Codecov Report

Merging #211 into master will decrease coverage by 0.30%.
The diff coverage is 91.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #211      +/-   ##
==========================================
- Coverage   77.17%   76.86%   -0.31%     
==========================================
  Files          73       75       +2     
  Lines        3965     4055      +90     
==========================================
+ Hits         3060     3117      +57     
- Misses        615      633      +18     
- Partials      290      305      +15     
Flag Coverage Δ
#go 76.86% <91.75%> (-0.31%) ⬇️
#wasm 71.76% <91.75%> (+0.16%) ⬆️
Impacted Files Coverage Δ
handshake_header.go 88.23% <0.00%> (-11.77%) ⬇️
internal/net/connctx/connctx.go 82.71% <0.00%> (-6.18%) ⬇️
internal/net/udp/conn.go 78.90% <0.00%> (-4.69%) ⬇️
fragment_buffer.go 86.04% <0.00%> (-4.66%) ⬇️
crypto_gcm.go 65.38% <0.00%> (-3.85%) ⬇️
conn.go 83.97% <0.00%> (-1.10%) ⬇️
internal/replaydetector/fixedbig.go 95.00% <0.00%> (ø)
internal/replaydetector/replaydetector.go 83.33% <0.00%> (ø)

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 fe75c17...0852438. Read the comment docs.

conn.go Outdated

mtu := config.MTU
if mtu <= 0 {
mtu = defaultMTU
}

replayProtectionWindow := config.ReplayProtectionWindow
if replayProtectionWindow <= 0 {
replayProtectionWindow = 64
Copy link
Member

Choose a reason for hiding this comment

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

Mind making the 64 a const? Just so people can easily find.

conn.go Outdated
if isClient {
loggerName = "dtls client"
}
logger := loggerFactory.NewLogger(loggerName)
Copy link
Member

@Sean-Der Sean-Der Mar 7, 2020

Choose a reason for hiding this comment

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

I don't have a problem letting people pass this in, but probably shouldn't make this change now! People collect logs off of dtls right now, so would break them.

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'll just revert log related changes. I added this to check trace on CI as it is very hard to see the trace log, but it is not needed in usual applications.

conn.go Outdated
replaydetector.New(c.replayProtectionWindow, maxSequenceNumber),
)
}
accept, ok := c.state.replayDetector[int(h.epoch)].Check(h.sequenceNumber)
Copy link
Member

@Sean-Der Sean-Der Mar 7, 2020

Choose a reason for hiding this comment

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

mind renaming accept so it makes it more clear it is accepting it for the replayDetector? All the calls to accept below might confuse people who aren't familiar.

input []uint64
expected []uint64
}{
"Continuous": {16, 0x0000FFFFFFFFFFFF,
Copy link
Member

@Sean-Der Sean-Der Mar 7, 2020

Choose a reason for hiding this comment

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

Is it worth adding a test for the window being moved forward by a large jump (and then old stuff should be ignored)?

   If the received record falls within the window and is new, or if the
   packet is to the right of the window, then the receiver proceeds to
   MAC verification.  If the MAC validation fails, the receiver MUST
   discard the received record as invalid.  The receive window is
   updated only if the MAC verification succeeds.

return func() {}, false
}
if seq > d.latestSeq {
// Update the head of the window.
Copy link
Member Author

Choose a reason for hiding this comment

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

Window shift must be done after validation.

shift := seq - d.latestSeq
for i := len(d.mask) - 1; i > 0; i-- {
d.mask[i] <<= shift
d.mask[i] |= d.mask[i-1] >> (64 - shift)
Copy link
Member Author

Choose a reason for hiding this comment

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

This loose some bits if window size is larger than 128 and window shift is larger than 64.

@at-wat at-wat force-pushed the add-replay-protection branch from 7432d82 to 7046799 Compare March 7, 2020 15:20
@at-wat at-wat requested a review from Sean-Der March 7, 2020 15:57
@at-wat
Copy link
Member Author

at-wat commented Mar 7, 2020

Addressed review feedbacks and rebased.

Copy link
Member

@Sean-Der Sean-Der left a comment

Choose a reason for hiding this comment

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

Really amazing work, this is a massive improvement!

Implements RFC 6347 Section 4.1.2.6.
Set config.ReplayProtectionWindow to change the size of the
protection window. Default is 64.
@at-wat at-wat force-pushed the add-replay-protection branch from 7046799 to 0852438 Compare March 7, 2020 23:59
@at-wat
Copy link
Member Author

at-wat commented Mar 7, 2020

rebased

@at-wat at-wat merged commit 1a60060 into master Mar 8, 2020
@at-wat at-wat deleted the add-replay-protection branch March 8, 2020 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add sliding window replay detection for ApplicationData
2 participants