Skip to content

Commit 2bc9f11

Browse files
arpitjain099arpitjain099
authored
Fix pprof validation: nil period_type and off-by-one string bounds (#6376)
ValidatePprofProfile runs on the WriteRaw ingest path, so it sees attacker-influenced pprof bytes. Two crashes are reachable there. First, period_type is an optional field and comes across as a nil *ValueType when omitted. Every other pointer in the validator is nil-checked, but the period_type branch dereferences p.PeriodType directly, so a profile that simply leaves it out panics inside the validator. Guard it like the rest. Second, the string-table index checks use > len(StringTable). String indices are 0-based, so the only valid values are 0..len-1 and an index equal to len passes the check but is one past the end. Downstream code (MetaFromPprof, LabelsFromSample) then indexes it and panics. Switch the eleven 0-based string-index checks to >=. The 1-based id checks (mapping/function/location) are left as > since len is a valid id there. Adds a table-driven test covering an omitted period_type and an index equal to the string-table length for both the sample-type and period-type paths. Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
1 parent 7d2c7ed commit 2bc9f11

2 files changed

Lines changed: 102 additions & 14 deletions

File tree

‎pkg/normalizer/validate.go‎

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -37,10 +37,10 @@ func ValidatePprofProfile(p *pprofpb.Profile, ei []*profilestorepb.ExecutableInf
3737
if m.Id != uint64(i+1) {
3838
return fmt.Errorf("mapping id is not sequential")
3939
}
40-
if m.Filename != 0 && m.Filename > stringTableLen {
40+
if m.Filename != 0 && m.Filename >= stringTableLen {
4141
return fmt.Errorf("mapping (id: %d) has invalid filename index %d", m.Id, m.Filename)
4242
}
43-
if m.BuildId != 0 && m.BuildId > stringTableLen {
43+
if m.BuildId != 0 && m.BuildId >= stringTableLen {
4444
return fmt.Errorf("mapping (id: %d) has invalid buildid index %d", m.Id, m.Filename)
4545
}
4646
}
@@ -57,13 +57,13 @@ func ValidatePprofProfile(p *pprofpb.Profile, ei []*profilestorepb.ExecutableInf
5757
if f.Id != uint64(i+1) {
5858
return fmt.Errorf("function id is not sequential")
5959
}
60-
if f.Name != 0 && f.Name > stringTableLen {
60+
if f.Name != 0 && f.Name >= stringTableLen {
6161
return fmt.Errorf("function (id: %d) has invalid name index %d", f.Id, f.Name)
6262
}
63-
if f.SystemName != 0 && f.SystemName > stringTableLen {
63+
if f.SystemName != 0 && f.SystemName >= stringTableLen {
6464
return fmt.Errorf("function (id: %d) has invalid systemname index %d", f.Id, f.SystemName)
6565
}
66-
if f.Filename != 0 && f.Filename > stringTableLen {
66+
if f.Filename != 0 && f.Filename >= stringTableLen {
6767
return fmt.Errorf("function (id: %d) has invalid filename index %d", f.Id, f.Filename)
6868
}
6969
}
@@ -97,21 +97,23 @@ func ValidatePprofProfile(p *pprofpb.Profile, ei []*profilestorepb.ExecutableInf
9797
return fmt.Errorf("profile has nil sample type")
9898
}
9999

100-
if st.Type != 0 && st.Type > stringTableLen {
100+
if st.Type != 0 && st.Type >= stringTableLen {
101101
return fmt.Errorf("sample type %d has invalid type index %d", i, st.Type)
102102
}
103103

104-
if st.Unit != 0 && st.Unit > stringTableLen {
104+
if st.Unit != 0 && st.Unit >= stringTableLen {
105105
return fmt.Errorf("sample type %d has invalid unit index %d", i, st.Unit)
106106
}
107107
}
108108

109-
if p.PeriodType.Type != 0 && p.PeriodType.Type > stringTableLen {
110-
return fmt.Errorf("period type has invalid type index %d", p.PeriodType.Type)
111-
}
109+
if p.PeriodType != nil {
110+
if p.PeriodType.Type != 0 && p.PeriodType.Type >= stringTableLen {
111+
return fmt.Errorf("period type has invalid type index %d", p.PeriodType.Type)
112+
}
112113

113-
if p.PeriodType.Unit != 0 && p.PeriodType.Unit > stringTableLen {
114-
return fmt.Errorf("period type has invalid unit index %d", p.PeriodType.Unit)
114+
if p.PeriodType.Unit != 0 && p.PeriodType.Unit >= stringTableLen {
115+
return fmt.Errorf("period type has invalid unit index %d", p.PeriodType.Unit)
116+
}
115117
}
116118

117119
for i, s := range p.Sample {
@@ -133,10 +135,10 @@ func ValidatePprofProfile(p *pprofpb.Profile, ei []*profilestorepb.ExecutableInf
133135
if label.Key == 0 {
134136
return fmt.Errorf("sample %d label %d has no key", i, j)
135137
}
136-
if label.Key != 0 && label.Key > stringTableLen {
138+
if label.Key != 0 && label.Key >= stringTableLen {
137139
return fmt.Errorf("sample %d label %d has invalid key index %d", i, j, label.Key)
138140
}
139-
if label.Str != 0 && label.Str > stringTableLen {
141+
if label.Str != 0 && label.Str >= stringTableLen {
140142
return fmt.Errorf("sample %d label %d has invalid str index %d", i, j, label.Str)
141143
}
142144
}

‎pkg/normalizer/validate_test.go‎

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
// Copyright 2026 The Parca Authors
2+
// Licensed under the Apache License, Version 2.0 (the "License");
3+
// you may not use this file except in compliance with the License.
4+
// You may obtain a copy of the License at
5+
//
6+
// http://www.apache.org/licenses/LICENSE-2.0
7+
//
8+
// Unless required by applicable law or agreed to in writing, software
9+
// distributed under the License is distributed on an "AS IS" BASIS,
10+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
11+
// See the License for the specific language governing permissions and
12+
// limitations under the License.
13+
14+
package normalizer
15+
16+
import (
17+
"testing"
18+
19+
"github.com/stretchr/testify/require"
20+
21+
pprofpb "github.com/parca-dev/parca/gen/proto/go/google/pprof"
22+
)
23+
24+
func TestValidatePprofProfile_StringTableBounds(t *testing.T) {
25+
// String table has 3 entries, so the only valid indices are 0, 1 and 2.
26+
// An index equal to len(StringTable) is one past the end.
27+
stringTable := []string{"", "cpu", "nanoseconds"}
28+
29+
for _, tc := range []struct {
30+
name string
31+
profile *pprofpb.Profile
32+
wantErr bool
33+
}{
34+
{
35+
name: "valid",
36+
profile: &pprofpb.Profile{
37+
StringTable: stringTable,
38+
SampleType: []*pprofpb.ValueType{{Type: 1, Unit: 2}},
39+
PeriodType: &pprofpb.ValueType{Type: 1, Unit: 2},
40+
},
41+
wantErr: false,
42+
},
43+
{
44+
// period_type is optional; omitting it must validate cleanly,
45+
// not panic inside the validator.
46+
name: "nil period type",
47+
profile: &pprofpb.Profile{
48+
StringTable: stringTable,
49+
SampleType: []*pprofpb.ValueType{{Type: 1, Unit: 2}},
50+
PeriodType: nil,
51+
},
52+
wantErr: false,
53+
},
54+
{
55+
name: "sample type index equal to string table length",
56+
profile: &pprofpb.Profile{
57+
StringTable: stringTable,
58+
SampleType: []*pprofpb.ValueType{{Type: int64(len(stringTable))}},
59+
PeriodType: &pprofpb.ValueType{Type: 1, Unit: 2},
60+
},
61+
wantErr: true,
62+
},
63+
{
64+
name: "period type index equal to string table length",
65+
profile: &pprofpb.Profile{
66+
StringTable: stringTable,
67+
SampleType: []*pprofpb.ValueType{{Type: 1, Unit: 2}},
68+
PeriodType: &pprofpb.ValueType{Type: int64(len(stringTable))},
69+
},
70+
wantErr: true,
71+
},
72+
} {
73+
t.Run(tc.name, func(t *testing.T) {
74+
// require.NotPanics keeps a regression from crashing the whole
75+
// test binary and reports it as a normal failure instead.
76+
require.NotPanics(t, func() {
77+
err := ValidatePprofProfile(tc.profile, nil)
78+
if tc.wantErr {
79+
require.Error(t, err)
80+
} else {
81+
require.NoError(t, err)
82+
}
83+
})
84+
})
85+
}
86+
}

0 commit comments

Comments
 (0)