make TsigVerify check time after signature per rfc2845bis #1135
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1135 +/- ##
==========================================
+ Coverage 55.32% 55.43% +0.10%
==========================================
Files 41 41
Lines 10109 10110 +1
==========================================
+ Hits 5593 5604 +11
+ Misses 3488 3482 -6
+ Partials 1028 1024 -4
Continue to review full report at Codecov.
|
tsig_test.go
Outdated
| // tweak the algorithm name in the wire data, resulting in the "unknown algorithm" error. | ||
| msgData := buildMsgData(timeSigned) | ||
| garbage := []byte("bogus") | ||
| copy(msgData[67:67+len(garbage)], garbage) |
There was a problem hiding this comment.
This can just be: copy(msgData[67:], "bogus").
There was a problem hiding this comment.
thanks, updated it as suggested.
tsig_test.go
Outdated
| return msgData | ||
| } | ||
|
|
||
| checkError := func(expected, actual error) { |
There was a problem hiding this comment.
Ditch this because it doesn’t match the rest of the tests and just include only the if actual != expected check directly.
There was a problem hiding this comment.
do you mean instead of
checkError(ErrTime, tsigVerify(buildMsgData(timeSigned), testSecret, "", false, timeSigned+301))
do something like this?
if err := tsigVerify(buildMsgData(timeSigned), testSecret, "", false, timeSigned+301); err != ErrTime {
t.Fatalf("expected an error '%v' but got '%v'", ErrTime, err)
}
There was a problem hiding this comment.
@tmthrgd okay, updated as such.
|
/merge
…On Sat, 18 Jul 2020, 06:13 Tom Thorogood, ***@***.***> wrote:
***@***.**** approved this pull request.
LGTM.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#1135 (review)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACWIWZZNVD4ERRPH73U64LR4EONHANCNFSM4O5COLAA>
.
|
Automatically submitted.
The current implementation of
TsigVerifyis compliant to RFC2845 in that it first checks 'time signed' and then verifies the signature. But this logic is known to have a security vulnerability as reported in CVE-2017-3142/3143.The protocol is being revised as draft-ietf-dnsop-rfc2845bis, reversing the order of these checks. This PR implements the new behavior in a straightforward way. While the new protocol spec is still an internet draft, it's already in the RFC editor queue, so I believe it's mature enough.
(Added test covers this specific case with some other error conditions from
TsigVerify)