Skip to content

Commit 03d9be3

Browse files
jasnelladuh95
authored andcommitted
perf_hooks: harden histogram CBOR import validation
`importHistogram()` did not check the CBOR major type of keys and integer values, so other data items were decoded as integers. It also accepted duplicate keys, silently truncated integers cast to narrower types, allowed bucket counts that did not fit into an `int64_t`, and trusted the total count, min, and max, leaving imported histograms in an inconsistent state when those were absent or did not match the counts. Validate major types and value ranges, reject duplicate keys and non-increasing sparse count indexes, and derive the total count, min, and max from the counts when they are absent. A total count that is present must match the counts. Data produced by `histogram.export()` is unaffected. Assisted-by: OpenCode Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #66098 Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
1 parent 88aa3da commit 03d9be3

2 files changed

Lines changed: 249 additions & 73 deletions

File tree

‎src/histogram.cc‎

Lines changed: 108 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -1066,12 +1066,45 @@ static bool CborReadFloat64(const uint8_t*& p,
10661066
return true;
10671067
}
10681068

1069+
// Read the argument of a data item that must have the given major type
1070+
// (kCborUint, kCborArray, or kCborMap). CborReadUint() alone decodes the
1071+
// argument of any major type.
1072+
static bool CborReadArgument(const uint8_t*& p,
1073+
const uint8_t* end,
1074+
uint8_t major,
1075+
uint64_t* val) {
1076+
if (p >= end || (*p & 0xe0) != major) return false;
1077+
return CborReadUint(p, end, val);
1078+
}
1079+
1080+
// Read an unsigned integer that must fit into a non-negative int64_t.
1081+
static bool CborReadInt64(const uint8_t*& p, const uint8_t* end, int64_t* val) {
1082+
uint64_t v;
1083+
if (!CborReadArgument(p, end, kCborUint, &v) ||
1084+
v > static_cast<uint64_t>(std::numeric_limits<int64_t>::max())) {
1085+
return false;
1086+
}
1087+
*val = static_cast<int64_t>(v);
1088+
return true;
1089+
}
1090+
1091+
// Read an unsigned integer that must fit into a non-negative int32_t.
1092+
static bool CborReadInt32(const uint8_t*& p, const uint8_t* end, int32_t* val) {
1093+
uint64_t v;
1094+
if (!CborReadArgument(p, end, kCborUint, &v) ||
1095+
v > static_cast<uint64_t>(std::numeric_limits<int32_t>::max())) {
1096+
return false;
1097+
}
1098+
*val = static_cast<int32_t>(v);
1099+
return true;
1100+
}
1101+
10691102
// Read a value that may be either a uint or float64.
10701103
static bool CborReadNumber(const uint8_t*& p, const uint8_t* end, double* val) {
10711104
if (p >= end) return false;
10721105
if (*p == kCborFloat64) return CborReadFloat64(p, end, val);
10731106
uint64_t u;
1074-
if (!CborReadUint(p, end, &u)) return false;
1107+
if (!CborReadArgument(p, end, kCborUint, &u)) return false;
10751108
*val = static_cast<double>(u);
10761109
return true;
10771110
}
@@ -1502,13 +1535,12 @@ std::shared_ptr<Histogram> Histogram::Import(const uint8_t* data, size_t len) {
15021535
const uint8_t* end = data + len;
15031536

15041537
// Read top-level map header.
1505-
if (p >= end || (*p >> 5) != 5) return nullptr; // Must be a map.
15061538
uint64_t map_size;
1507-
if (!CborReadUint(p, end, &map_size)) return nullptr;
1539+
if (!CborReadArgument(p, end, kCborMap, &map_size)) return nullptr;
15081540

15091541
int64_t lowest = 1;
15101542
int64_t highest = std::numeric_limits<int64_t>::max();
1511-
int figures = 3;
1543+
int32_t figures = 3;
15121544
int64_t total_count = 0;
15131545
int64_t min_value = std::numeric_limits<int64_t>::max();
15141546
int64_t max_value = 0;
@@ -1517,6 +1549,20 @@ std::shared_ptr<Histogram> Histogram::Import(const uint8_t* data, size_t len) {
15171549
int32_t counts_len = 0;
15181550
uint64_t version = 0;
15191551

1552+
// Bitsets of the keys read so far, used to reject duplicate keys and to
1553+
// tell whether a field was present. All known keys are less than 64.
1554+
uint64_t seen_keys = 0;
1555+
uint64_t seen_ewma_keys = 0;
1556+
auto mark_seen = [](uint64_t* seen, uint64_t key) {
1557+
const uint64_t bit = uint64_t{1} << key;
1558+
if (*seen & bit) return false;
1559+
*seen |= bit;
1560+
return true;
1561+
};
1562+
auto has_key = [&seen_keys](uint64_t key) {
1563+
return (seen_keys & (uint64_t{1} << key)) != 0;
1564+
};
1565+
15201566
// Sparse counts storage.
15211567
std::vector<std::pair<int32_t, int64_t>> sparse_counts;
15221568

@@ -1530,99 +1576,80 @@ std::shared_ptr<Histogram> Histogram::Import(const uint8_t* data, size_t len) {
15301576
for (uint64_t i = 0; i < map_size; i++) {
15311577
// Read key (unsigned int).
15321578
uint64_t key;
1533-
if (!CborReadUint(p, end, &key)) return nullptr;
1579+
if (!CborReadArgument(p, end, kCborUint, &key)) return nullptr;
1580+
if (key > kKeyEwma) return nullptr; // Unknown key.
1581+
if (!mark_seen(&seen_keys, key)) return nullptr; // Duplicate key.
15341582

15351583
switch (key) {
15361584
case kKeyVersion:
1537-
if (!CborReadUint(p, end, &version)) return nullptr;
1585+
if (!CborReadArgument(p, end, kCborUint, &version)) return nullptr;
15381586
if (version != kExportVersion) return nullptr;
15391587
break;
1540-
case kKeyLowest: {
1541-
uint64_t v;
1542-
if (!CborReadUint(p, end, &v)) return nullptr;
1543-
lowest = static_cast<int64_t>(v);
1588+
case kKeyLowest:
1589+
if (!CborReadInt64(p, end, &lowest)) return nullptr;
15441590
break;
1545-
}
1546-
case kKeyHighest: {
1547-
uint64_t v;
1548-
if (!CborReadUint(p, end, &v)) return nullptr;
1549-
highest = static_cast<int64_t>(v);
1591+
case kKeyHighest:
1592+
if (!CborReadInt64(p, end, &highest)) return nullptr;
15501593
break;
1551-
}
1552-
case kKeyFigures: {
1553-
uint64_t v;
1554-
if (!CborReadUint(p, end, &v)) return nullptr;
1555-
figures = static_cast<int>(v);
1594+
case kKeyFigures:
1595+
if (!CborReadInt32(p, end, &figures)) return nullptr;
15561596
break;
1557-
}
1558-
case kKeyTotalCount: {
1559-
uint64_t v;
1560-
if (!CborReadUint(p, end, &v)) return nullptr;
1561-
total_count = static_cast<int64_t>(v);
1597+
case kKeyTotalCount:
1598+
if (!CborReadInt64(p, end, &total_count)) return nullptr;
15621599
break;
1563-
}
1564-
case kKeyMin: {
1565-
uint64_t v;
1566-
if (!CborReadUint(p, end, &v)) return nullptr;
1567-
min_value = static_cast<int64_t>(v);
1600+
case kKeyMin:
1601+
if (!CborReadInt64(p, end, &min_value)) return nullptr;
15681602
break;
1569-
}
1570-
case kKeyMax: {
1571-
uint64_t v;
1572-
if (!CborReadUint(p, end, &v)) return nullptr;
1573-
max_value = static_cast<int64_t>(v);
1603+
case kKeyMax:
1604+
if (!CborReadInt64(p, end, &max_value)) return nullptr;
15741605
break;
1575-
}
1576-
case kKeyNormOffset: {
1577-
uint64_t v;
1578-
if (!CborReadUint(p, end, &v)) return nullptr;
1579-
// Reject values that cannot be represented as int32_t; the
1580-
// static_cast below would wrap and produce an arbitrary offset.
1581-
if (v > static_cast<uint64_t>(std::numeric_limits<int32_t>::max()))
1582-
return nullptr;
1583-
norm_offset = static_cast<int32_t>(v);
1606+
case kKeyNormOffset:
1607+
// Reject values that cannot be represented as int32_t; casting them
1608+
// would wrap and produce an arbitrary offset.
1609+
if (!CborReadInt32(p, end, &norm_offset)) return nullptr;
15841610
break;
1585-
}
15861611
case kKeyConvRatio:
15871612
if (!CborReadNumber(p, end, &conv_ratio)) return nullptr;
15881613
break;
1589-
case kKeyCountsLen: {
1590-
uint64_t v;
1591-
if (!CborReadUint(p, end, &v)) return nullptr;
1592-
counts_len = static_cast<int32_t>(v);
1614+
case kKeyCountsLen:
1615+
if (!CborReadInt32(p, end, &counts_len)) return nullptr;
15931616
break;
1594-
}
15951617
case kKeyCounts: {
15961618
// Array of flat [delta, count, ...] pairs. Indices are
15971619
// delta-encoded: accumulate to recover absolute indices.
1598-
if (p >= end || (*p >> 5) != 4) return nullptr;
15991620
uint64_t arr_len;
1600-
if (!CborReadUint(p, end, &arr_len)) return nullptr;
1621+
if (!CborReadArgument(p, end, kCborArray, &arr_len)) return nullptr;
16011622
if (arr_len % 2 != 0) return nullptr;
16021623
// Each element needs at least 1 byte of CBOR encoding, so
16031624
// arr_len can't exceed the remaining buffer. Without this
16041625
// check, a crafted buffer claiming arr_len=2^60 would cause
16051626
// reserve() to OOM-crash before the loop catches the error.
16061627
if (arr_len > static_cast<uint64_t>(end - p)) return nullptr;
16071628
sparse_counts.reserve(static_cast<size_t>(arr_len / 2));
1608-
int32_t acc_idx = 0;
1629+
int64_t acc_idx = 0;
16091630
for (uint64_t j = 0; j < arr_len; j += 2) {
1610-
uint64_t delta, cnt;
1611-
if (!CborReadUint(p, end, &delta)) return nullptr;
1612-
if (!CborReadUint(p, end, &cnt)) return nullptr;
1613-
acc_idx += static_cast<int32_t>(delta);
1614-
sparse_counts.emplace_back(acc_idx, static_cast<int64_t>(cnt));
1631+
int32_t delta;
1632+
int64_t cnt;
1633+
if (!CborReadInt32(p, end, &delta)) return nullptr;
1634+
if (!CborReadInt64(p, end, &cnt)) return nullptr;
1635+
// Indices are strictly increasing, so only the first delta (the
1636+
// absolute index of the first non-empty bucket) may be zero.
1637+
if (j > 0 && delta == 0) return nullptr;
1638+
acc_idx += delta;
1639+
if (acc_idx > std::numeric_limits<int32_t>::max()) return nullptr;
1640+
sparse_counts.emplace_back(static_cast<int32_t>(acc_idx), cnt);
16151641
}
16161642
break;
16171643
}
16181644
case kKeyEwma: {
16191645
// Sub-map for EWMA state.
1620-
if (p >= end || (*p >> 5) != 5) return nullptr;
16211646
uint64_t sub_size;
1622-
if (!CborReadUint(p, end, &sub_size)) return nullptr;
1647+
if (!CborReadArgument(p, end, kCborMap, &sub_size)) return nullptr;
16231648
for (uint64_t j = 0; j < sub_size; j++) {
16241649
uint64_t sub_key;
1625-
if (!CborReadUint(p, end, &sub_key)) return nullptr;
1650+
if (!CborReadArgument(p, end, kCborUint, &sub_key)) return nullptr;
1651+
if (sub_key > kEwmaThreshold) return nullptr; // Unknown EWMA key.
1652+
if (!mark_seen(&seen_ewma_keys, sub_key)) return nullptr;
16261653
switch (sub_key) {
16271654
case kEwmaAlpha:
16281655
if (!CborReadNumber(p, end, &ewma_alpha)) return nullptr;
@@ -1636,20 +1663,13 @@ std::shared_ptr<Histogram> Histogram::Import(const uint8_t* data, size_t len) {
16361663
case kEwmaErrorRate:
16371664
if (!CborReadNumber(p, end, &ewma_error_rate)) return nullptr;
16381665
break;
1639-
case kEwmaThreshold: {
1640-
uint64_t v;
1641-
if (!CborReadUint(p, end, &v)) return nullptr;
1642-
threshold = static_cast<int64_t>(v);
1666+
case kEwmaThreshold:
1667+
if (!CborReadInt64(p, end, &threshold)) return nullptr;
16431668
break;
1644-
}
1645-
default:
1646-
return nullptr; // Unknown EWMA key.
16471669
}
16481670
}
16491671
break;
16501672
}
1651-
default:
1652-
return nullptr; // Unknown key.
16531673
}
16541674
}
16551675

@@ -1679,16 +1699,31 @@ std::shared_ptr<Histogram> Histogram::Import(const uint8_t* data, size_t len) {
16791699
if (norm_offset < 0 || norm_offset >= counts_len) return nullptr;
16801700

16811701
// Restore counts directly.
1702+
int64_t observed_total_count = 0;
16821703
for (const auto& [idx, cnt] : sparse_counts) {
16831704
if (idx < 0 || idx >= counts_len) return nullptr;
1705+
// The counts must add up without overflowing int64_t.
1706+
if (cnt > std::numeric_limits<int64_t>::max() - observed_total_count) {
1707+
return nullptr;
1708+
}
1709+
observed_total_count += cnt;
16841710
histogram->histogram_->counts[idx] = cnt;
16851711
}
1686-
histogram->histogram_->total_count = total_count;
1687-
histogram->histogram_->min_value = min_value;
1688-
histogram->histogram_->max_value = max_value;
16891712
histogram->histogram_->normalizing_index_offset = norm_offset;
16901713
histogram->histogram_->conversion_ratio = conv_ratio;
16911714

1715+
// Derive the total count, min, and max from the counts, as
1716+
// Histogram::Subtract() does. This keeps the histogram consistent when
1717+
// any of them are absent. A total count that is present must match the
1718+
// counts. Min and max values that are present are restored as recorded.
1719+
hdr_reset_internal_counters(histogram->histogram_.get());
1720+
if (has_key(kKeyTotalCount) &&
1721+
total_count != histogram->histogram_->total_count) {
1722+
return nullptr;
1723+
}
1724+
if (has_key(kKeyMin)) histogram->histogram_->min_value = min_value;
1725+
if (has_key(kKeyMax)) histogram->histogram_->max_value = max_value;
1726+
16921727
// Restore EWMA state.
16931728
if (ewma_alpha > 0) {
16941729
histogram->ewma_mean_ = ewma_mean;

0 commit comments

Comments
 (0)