CBG-3238 perform a length check on raw bytes #6438
Merged
Conversation
This file contains 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
- refactor attachment compaction tests so we can make sure that it doesn't terminate
adamcfraser
requested changes
Sep 20, 2023
db/document.go
Outdated
| @@ -515,6 +515,9 @@ func parseXattrStreamData(xattrName string, userXattrName string, data []byte) ( | |||
| } | |||
|
|
|||
| xattrsLen := binary.BigEndian.Uint32(data[0:4]) | |||
| if int(xattrsLen) > len(data) { | |||
There was a problem hiding this comment.
This looks like it should check if int(xattrsLen+4) > len(data) to avoid all possible panics on line 518.
| @@ -515,6 +515,9 @@ func parseXattrStreamData(xattrName string, userXattrName string, data []byte) ( | |||
| } | |||
|
|
|||
| xattrsLen := binary.BigEndian.Uint32(data[0:4]) | |||
| if int(xattrsLen) > len(data) { | |||
| return nil, nil, nil, fmt.Errorf("%w (%d) from bytes %s", base.ErrXattrInvalidLen, xattrsLen, data) | |||
| } | |||
There was a problem hiding this comment.
Are we able to write a test for what happens if data[0:4] doesn't represent xattrsLen (i.e. document doesn't have xattrs, it's just the body), but the Uint32 value of data[0:4] is less than len(data)? I'm assuming we'll return an error while trying to parse the xattr key/value pairs below, but would be good to verify there aren't any subsequent panics in that case.
There was a problem hiding this comment.
Added a test for this.
db/attachment_compaction.go
Outdated
| base.WarnfCtx(ctx, format, args...) | ||
| return false | ||
| // processAttachmentCompactSweepCallback(ctx context.Context, dataStore base.DataStore, db *Database, compactionID, compactionLoggingID string, dryRun bool, purgedAttachmentCount *base.AtomicInt, event sgbucket.FeedEvent) { | ||
| func processAttachmentCompactMarkCallback(ctx context.Context, dataStore base.DataStore, compactionID, compactionLoggingID string, markedAttachmentCount *base.AtomicInt, event sgbucket.FeedEvent) error { |
There was a problem hiding this comment.
Can you provide some more context on the need for these attachment compaction changes? I don't see how they are strictly related to the length check enhancement.
There was a problem hiding this comment.
As discussed offline, I removed these changes because they were bigger than was appropriate. I left a single change to treat a document with an invalid xattr as a document with xattrs.
- remove attachment compaction changes and leave only a change to avoid failure to mark changes if xattr length is invalid. Treat this as a document without xattrs in order to not fail attachment compaction test. - add code to prevent overflow if xattr len is a number between 1-4 which would cause a panic.
adamcfraser
approved these changes
Sep 21, 2023
torcolvin
added a commit
that referenced
this pull request
Sep 21, 2023
* CBG-3238 perform a length check on raw bytes - refactor attachment compaction tests so we can make sure that it doesn't terminate * Add comment * remove redundant returns * Move error handling into getAttachmentSyncData * PR fixup - remove attachment compaction changes and leave only a change to avoid failure to mark changes if xattr length is invalid. Treat this as a document without xattrs in order to not fail attachment compaction test. - add code to prevent overflow if xattr len is a number between 1-4 which would cause a panic. * Update tests * remove test tested in separate function
torcolvin
added a commit
that referenced
this pull request
Sep 21, 2023
* CBG-3238 perform a length check on raw bytes - refactor attachment compaction tests so we can make sure that it doesn't terminate * Add comment * remove redundant returns * Move error handling into getAttachmentSyncData * PR fixup - remove attachment compaction changes and leave only a change to avoid failure to mark changes if xattr length is invalid. Treat this as a document without xattrs in order to not fail attachment compaction test. - add code to prevent overflow if xattr len is a number between 1-4 which would cause a panic. * Update tests * remove test tested in separate function
1 task
torcolvin
added a commit
that referenced
this pull request
Sep 22, 2023
bbrks
pushed a commit
that referenced
this pull request
Mar 28, 2024
* CBG-3238 perform a length check on raw bytes - refactor attachment compaction tests so we can make sure that it doesn't terminate * Add comment * remove redundant returns * Move error handling into getAttachmentSyncData * PR fixup - remove attachment compaction changes and leave only a change to avoid failure to mark changes if xattr length is invalid. Treat this as a document without xattrs in order to not fail attachment compaction test. - add code to prevent overflow if xattr len is a number between 1-4 which would cause a panic. * Update tests * remove test tested in separate function
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.
Pre-review checklist
fmt.Print,log.Print, ...)base.UD(docID),base.MD(dbName))docs/apiIntegration Tests
GSI=true,xattrs=truehttps://jenkins.sgwdev.com/job/SyncGateway-Integration/2025/