fix out-of-bounds read in FuzzySetMatch/FuzzyMatchParse octet scan#6206
Open
aizu-m wants to merge 1 commit into
Open
fix out-of-bounds read in FuzzySetMatch/FuzzyMatchParse octet scan#6206aizu-m wants to merge 1 commit into
aizu-m wants to merge 1 commit into
Conversation
AddressSanitizer, matching a pattern against a shorter address:
ERROR: AddressSanitizer: heap-buffer-overflow READ of size 1
#0 strlen
cfengine#1 sscanf
cfengine#2 FuzzySetMatch addr_lib.c
Noticed this reading the IP range matcher. FuzzySetMatch() splits an
address into octets with a fixed 4-iteration loop:
for (i = 0; i < 4; i++) {
sscanf(sp1, "%63[^.]", buffer1);
...
sscanf(sp2, "%63[^.]", buffer2);
sp2 += strlen(buffer2) + 1;
}
The +1 steps over the '.' separator. But on the last octet there is no
separator, so it steps over the terminating NUL instead. When s2 has
fewer octets than s1 (say pattern "1.2.3.4" against address "1"), sp2
walks past the end of the string and the next sscanf reads out of bounds.
buffer2 is not reset either, so a failed scan keeps stale content.
s2 reaches here from untrusted data through the isipinsubnet() and
iprange() policy functions, whose address argument is an arbitrary
string.
Only advance past a separator that is actually there, reset buffer2, and
stop with no match once the address runs out of octets. The IPv6 match
loop and the IPv4 range loop in FuzzyMatchParse() had the same walk-off,
fixed the same way. Added a regression test that trips ASan on the old
code.
Changelog: Title
Signed-off-by: Aizal Khan <aizumusheer2@gmail.com>
Contributor
|
@cf-bottom Jenkins please :) |
|
Sure, I triggered a build: Jenkins: https://ci.cfengine.com/job/pr-pipeline/14075/ Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-14075/ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
FuzzySetMatch splits an address into octets with sscanf("%63[^.]") in a fixed 4-iteration loop and does sp += strlen(octet) + 1 to step over the dot. The final octet has no dot, so the +1 steps over the NUL instead, and when the address has fewer octets than the pattern the pointer leaves the string.
Reset the scan buffer, stop with no match once the address runs out of octets, and only step over a separator that is actually present. Same change at all three sites. The regression test in addr_lib_test.c aborts under ASan on the current code and passes with the fix.