Fix three high-impact Coverity defects#13030
Open
bryancall wants to merge 2 commits intoapache:masterfrom
Open
Fix three high-impact Coverity defects#13030bryancall wants to merge 2 commits intoapache:masterfrom
bryancall wants to merge 2 commits intoapache:masterfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Addresses three high-impact Coverity-reported memory-safety defects in ATS core ACL parsing and the slice plugin.
Changes:
- Prevent out-of-bounds writes in ACL subject parsing when more than
MAX_SUBJECTSare configured. - Fix a use-after-free in
sliceby ensuring the effective URL buffer is freed only after its final use. - Ensure ETag / Last-Modified buffers in
sliceare bounded and explicitly NUL-terminated.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/proxy/IPAllow.cc |
Stops parsing when too many ACL subjects are provided to avoid writing past subjects[]. |
plugins/slice/slice.cc |
Moves TSfree(urlstr) to after the final logging/use to avoid UAF. |
plugins/slice/server.cc |
Replaces strncpy() with bounded copy + explicit terminator for identifier headers to ensure safe strings. |
bryancall
force-pushed
the
coverity-high-impact-fixes
branch
from
7fbe242 to
41b2ae5
Compare
CID 1644226: Use-after-free in slice plugin. should_skip_this_obj() used urlstr in DEBUG_LOG after calling TSfree(). Move TSfree() to after the last use of urlstr. CID 1644298: Out-of-bounds write in IpAllow constructor. When more than MAX_SUBJECTS ACL subjects are configured, the loop continued writing to subjects[] past the array bounds. Add break after the error message. CID 1644219: Buffer not null-terminated in slice plugin. handleNextServerHeader() used strncpy() to copy etag and last-modified values without ensuring null termination. Replace with memcpy() + explicit null terminator, and clamp lengths to buffer size.
bryancall
force-pushed
the
coverity-high-impact-fixes
branch
2 times, most recently
from
f491e3e to
00955c7
Compare
Add ip_allow_subjects.test.py: Verifies that configuring more than MAX_SUBJECTS (3) ACL subjects logs an error and does not crash, and that exactly MAX_SUBJECTS works without error. Add slice_long_etag.test.py: Verifies the slice plugin handles long ETag values (4000 chars) without crashing when copying between internal buffers during multi-block range requests.
bryancall
force-pushed
the
coverity-high-impact-fixes
branch
from
00955c7 to
59d7b0d
Compare
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
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.
Summary
Fix three high-impact Coverity Scan defects (memory safety issues):
CID 1644226 (Use-after-free, High):
plugins/slice/slice.cc-should_skip_this_obj()usedurlstrinDEBUG_LOGafter callingTSfree(). MovedTSfree()to after the last use ofurlstr.CID 1644298 (Out-of-bounds write, High):
src/proxy/IPAllow.cc- When more thanMAX_SUBJECTS(3) ACL subjects are configured, the parsing loop continued writing past thesubjects[]array bounds. Addedbreakafter the error message.CID 1644219 (Buffer not null-terminated, High):
plugins/slice/server.cc-handleNextServerHeader()usedstrncpy()to copy etag and last-modified values without ensuring null termination. Replaced withmemcpy()+ explicit null terminator, and clamped lengths to buffer size to prevent overflow.Test plan