[SERVER-83137] Consider additional validation of size args to BufBuilder::appendBuf Created: 10/Nov/23  Updated: 17/Jan/24  Resolved: 17/Jan/24

Status: Closed
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: 7.3.0-rc0

Type: Task Priority: Major - P3
Reporter: George Wangensteen Assignee: Wei Hu
Resolution: Fixed Votes: 0
Labels: storex-shortlist
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Related
related to SERVER-85313 Create a benchmark for BufBuilder::grow Closed
is related to SERVER-82410 DocumentSourceListSearchIndexes shoul... Closed
Assigned Teams:
Storage Execution
Backwards Compatibility: Fully Compatible
Sprint: Execution Team 2023-12-25, Execution Team 2024-01-08, Execution Team 2024-01-22
Participants:

 Description   

BufBuilder::appendBuf (https://github.com/mongodb/mongo/blob/817bd0871e4d269c09c4a7ed009fba790549288e/src/mongo/bson/util/builder.h#L399C4-L402) accepts a size_t len argument describing the number of bytes to copy from the src buffer.

Before beginning the memcpy, it C-style casts len to `int`, and then compares that integer to the current buffer size to see if it needs to grow the current buffer.

When a large-enough size_t is passed to appendBuf, this can result in the C-style cast producing a negative number, and therefore the growth-needed-check receiving a negative number and concluding that the buffer doesn't need to grow. This will result in the memcpy writing to arbitrary addresses in the heap outside the range of the current buffer.

A motivating case of how this might happen is someone using BSONObjBuilder::append (see https://github.com/mongodb/mongo/blob/817bd0871e4d269c09c4a7ed009fba790549288e/src/mongo/bson/bsonobjbuilder.h#L180) to append a BSONObj. If the BSONObj we're appending is corrupt/has already been freed, it's objsize() member may return a negative integer, but because it will be converted to size_t in the call to appendBuf but back to a negative integer during the bounds-check, we won't catch the issue before we start copying the garbage memory into addresses outside the BufBuilder's ownership.

If it's possible without sacrificing performance (maybe only in debug builds?), we could add a check that the length arguments passed to appendBuf have positive `int` representations, and fail the appendBuf call if not. This would allow us to catch invalid calls to appendBuf before it further corrupts the heap.



 Comments   
Comment by Githook User [ 17/Jan/24 ]

Author:

{'name': 'Wei Hu', 'email': 'wei.hu@mongodb.com', 'username': 'wh5a'}

Message: SERVER-83137 validation of the by arg to BufBuilder::grow (#17697)

GitOrigin-RevId: 789d3556c190f70858ebcfba7b969a61efd76fad
Branch: master
https://github.com/mongodb/mongo/commit/a72561dde069d37e512d963f0060bf1a0bd4eb0d

Generated at Thu Feb 08 06:51:21 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.