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.

Sign up for GitHub

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

Jump to bottom

Invalid NSEC/3 bitmap on non-zero buffer #1338

Merged
merged 4 commits into from Apr 1, 2022
Merged

Invalid NSEC/3 bitmap on non-zero buffer #1338

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
67c37c4
Invalid NSEC/3 bitmap on non-zero buffer
rs Feb 6, 2022
09a2596
Add some comments + rename some vars to make algo clearer
rs Feb 7, 2022
e6e7d50
Revert to previous algo with window length compute+0 on new window
rs Feb 11, 2022
afe53a6
Use typeBitMapLen to compute the bitmap length to zero
rs Mar 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions msg_helpers.go
View file
Open in desktop
Expand Up @@ -558,6 +558,16 @@ func packDataNsec(bitmap []uint16, msg []byte, off int) (int, error) {
if len(bitmap) == 0 {
return off, nil
}
if off > len(msg) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it's possible for us to ever be called with off > len(msg), so I think this check is unneeded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Famous last word :)

The first unit test was testing this and would panic without this test. The cost is marginal so I would tend leave it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is too. I’m certain we can never hit this condition, but I’ll defer to the unit test.

return off, &Error{err: "overflow packing nsec"}
}
toZero := msg[off:]
if maxLen := typeBitMapLen(bitmap); maxLen < len(toZero) {
toZero = toZero[:maxLen]
}
for i := range toZero {
toZero[i] = 0
}
var lastwindow, lastlength uint16
for _, t := range bitmap {
window := t / 256
Expand Down
77 changes: 67 additions & 10 deletions msg_helpers_test.go
View file
Open in desktop
Expand Up @@ -19,7 +19,8 @@ func TestPackDataNsec(t *testing.T) {
tests := []struct {
name string
args args
want int
wantOff int
wantBytes []byte
wantErr bool
wantErrMsg string
}{
Expand All @@ -44,14 +45,14 @@ func TestPackDataNsec(t *testing.T) {
},
wantErr: true,
wantErrMsg: "dns: overflow packing nsec",
want: 31,
wantOff: 48,
},
{
name: "disordered nsec bits",
args: args{
bitmap: []uint16{
8962,
0,
1,
},
msg: []byte{
48, 48, 48, 48, 0, 0, 0, 1, 0, 0, 0, 0,
Expand All @@ -72,13 +73,13 @@ func TestPackDataNsec(t *testing.T) {
},
wantErr: true,
wantErrMsg: "dns: nsec bits out of order",
want: 155,
wantOff: 155,
},
{
name: "simple message with only one window",
args: args{
bitmap: []uint16{
0,
1,
},
msg: []byte{
48, 48, 48, 48, 0, 0,
Expand All @@ -89,13 +90,33 @@ func TestPackDataNsec(t *testing.T) {
},
off: 0,
},
wantErr: false,
want: 3,
wantErr: false,
wantOff: 3,
wantBytes: []byte{0, 1, 64},
},
{
name: "multiple types",
args: args{
bitmap: []uint16{
TypeNS, TypeSOA, TypeRRSIG, TypeDNSKEY, TypeNSEC3PARAM,
},
msg: []byte{
48, 48, 48, 48, 0, 0,
0, 1, 0, 0, 0, 0,
0, 0, 50, 48, 48, 48,
48, 48, 48, 0, 54, 48,
48, 48, 48, 0, 19, 48, 48,
},
off: 0,
},
wantErr: false,
wantOff: 9,
wantBytes: []byte{0, 7, 34, 0, 0, 0, 0, 2, 144},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := packDataNsec(tt.args.bitmap, tt.args.msg, tt.args.off)
gotOff, err := packDataNsec(tt.args.bitmap, tt.args.msg, tt.args.off)
if (err != nil) != tt.wantErr {
t.Errorf("packDataNsec() error = %v, wantErr %v", err, tt.wantErr)
return
Expand All @@ -104,13 +125,49 @@ func TestPackDataNsec(t *testing.T) {
t.Errorf("packDataNsec() error msg = %v, wantErrMsg %v", err.Error(), tt.wantErrMsg)
return
}
if got != tt.want {
t.Errorf("packDataNsec() = %v, want %v", got, tt.want)
if gotOff != tt.wantOff {
t.Errorf("packDataNsec() = %v, want off %v", gotOff, tt.wantOff)
}
if err == nil && tt.args.off < len(tt.args.msg) && gotOff < len(tt.args.msg) {
if want, got := tt.wantBytes, tt.args.msg[tt.args.off:gotOff]; !bytes.Equal(got, want) {
t.Errorf("packDataNsec() = %v, want bytes %v", got, want)
}
}
})
}
}

func TestPackDataNsecDirtyBuffer(t *testing.T) {
zeroBuf := []byte{0, 0, 0, 0, 0, 0, 0, 0, 0}
dirtyBuf := []byte{1, 2, 3, 4, 5, 6, 7, 8, 9}
off1, _ := packDataNsec([]uint16{TypeNS, TypeSOA, TypeRRSIG}, zeroBuf, 0)
off2, _ := packDataNsec([]uint16{TypeNS, TypeSOA, TypeRRSIG}, dirtyBuf, 0)
if off1 != off2 {
t.Errorf("off1 %v != off2 %v", off1, off2)
}
if !bytes.Equal(zeroBuf[:off1], dirtyBuf[:off2]) {
t.Errorf("dirty buffer differs from zero buffer: %v, %v", zeroBuf[:off1], dirtyBuf[:off2])
}
}

func BenchmarkPackDataNsec(b *testing.B) {
benches := []struct {
name string
types []uint16
}{
{"empty", nil},
{"typical", []uint16{TypeNS, TypeSOA, TypeRRSIG, TypeDNSKEY, TypeNSEC3PARAM}},
{"multiple_windows", []uint16{1, 300, 350, 10000, 20000}},
}
for _, bb := range benches {
b.Run(bb.name, func(b *testing.B) {
buf := make([]byte, 100)
for n := 0; n < b.N; n++ {
packDataNsec(bb.types, buf, 0)
}
})
}
}
func TestUnpackString(t *testing.T) {
msg := []byte("\x00abcdef\x0f\\\"ghi\x04mmm\x7f")
msg[0] = byte(len(msg) - 1)
Expand Down