fix: prevent integer overflow in query parameter parsing#6612
fix: prevent integer overflow in query parameter parsing#6612mdisibio merged 4 commits intografana:mainfrom
Conversation
Replace strconv.Atoi and strconv.ParseInt with strconv.ParseUint for
query parameters that are stored as unsigned integers (uint32/uint64).
Previously, passing negative values or values exceeding the target
type's range would silently overflow/wrap. For example:
- start=-1 would wrap to 4294967295 (uint32 max)
- limit=4294967298 would wrap to 2
Now, out-of-range and negative values are properly rejected with
descriptive parse errors.
Affected parameters in ParseSearchRequestWithDefault:
- start, end: ParseInt(s, 10, 32) -> ParseUint(s, 10, 32)
- limit, spss: Atoi -> ParseUint(s, 10, 32)
Affected parameters in ParseSpanMetricsRequest:
- start, end: ParseInt(s, 10, 32) -> ParseUint(s, 10, 32)
- limit: Atoi -> ParseUint(l, 10, 64)
Affected parameters in ParseQueryRangeRequest:
- startPage, pagesToSearch, footerSize, exemplars, maxSeries:
Atoi -> ParseUint(s, 10, 32)
- size: Atoi -> ParseUint(s, 10, 64)
Fixes: grafana#6503
Signed-off-by: bejaratommy <tommy@bejara.net>
ricardbejarano
requested review from
carles-grafana,
electron0zero,
ie-pham,
javiermolinar,
mapno,
mattdurham,
mdisibio,
oleg-kozlyuk-grafana,
ruslan-mikhailov,
stoewer,
yvrhdn and
zalegrala
as code owners
mdisibio
left a comment
mdisibio
left a comment
There was a problem hiding this comment.
Thanks for the fixes. Honestly not sure why we did signed parsing, and amusingly we checked for negatives for spansPerSpanSet. LGTM and will merge after CI.
|
There are a couple of CI issues. (1) It looks like we need to update one of the tests for the new error message: (2) Not sure why but this is triggering lint errors on unrelated (existing) code in modules/generator and modules/querier. Haven't run into this before, I don't seem them changed in any commit in this branch or any force pushes. I'm thinking there are a couple options: (a) Change these in this PR or (b) I can fix this separately and then you would need to merge latest main into this branch to get past them. Leaning towards (b) unless you are interested in (a). I'll keep looking to see what the explanation is. |
|
Hi @mdisibio, i'll look at the tests. I'd prefer you fix the linting issues elsewhere and then i can rebase (option B), yeah. Thanks! |
|
|
Seems like the PR is in healthy state. Just FYI, I'm working on to fix the historical lint errors #6529. |
Thanks, missed that. In that case we are good, and it is merged. @ricardbejarano Thanks for the fix 👍 |
4 participants
What this PR does:
Replace
strconv.Atoiandstrconv.ParseIntwithstrconv.ParseUintfor query parameters stored as unsigned integers (uint32/uint64) inpkg/api/http.go.Passing negative values or values exceeding the target type's range silently overflow/wrap instead of returning an error. For example:
start=-1wraps to4294967295(uint32 max)limit=4294967298wraps to2This is because
strconv.Atoiparses into a platform-dependentint(64-bit on 64-bit systems) andstrconv.ParseInt(s, 10, 32)accepts negative values, both of which silently truncate or wrap when cast touint32.Replaced all 13 affected call sites across
ParseSearchRequestWithDefault,ParseSpanMetricsRequest, andParseQueryRangeRequest. Also removed now-redundant sign checks sinceParseUintrejects negative values at parse time.Which issue(s) this PR fixes:
Fixes #6503
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]