[SERVER-27209] BSONObj::getStringField() does not handle embedded null bytes correctly Created: 29/Nov/16  Updated: 05/Jun/22  Resolved: 13/Jan/22

Status: Closed
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: 5.3.0

Type: Bug Priority: Major - P3
Reporter: Marko Vojvodic Assignee: Matt Kneiser
Resolution: Done Votes: 0
Labels: bson, neweng, techdebt
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Problem/Incident
causes SERVER-67028 Fix 12% perf loss in validateBSON due... Closed
Backwards Compatibility: Minor Change
Operating System: ALL
Sprint: Execution Team 2021-11-29, Execution Team 2021-12-13, Execution Team 2021-12-27, Execution Team 2022-01-10, Execution Team 2022-01-24
Participants:
Linked BF Score: 129

 Description   

A BSONElement of type String has a pointer + length implementation and therefore may contain an embedded null byte. BSONObj::getStringField uses valuestr in its implementation, which can lead us to incorrectly interpret the string as null terminated.



 Comments   
Comment by Githook User [ 13/Jan/22 ]

Author:

{'name': 'Matt Kneiser', 'email': 'matt.kneiser@mongodb.com', 'username': 'themattman'}

Message: SERVER-27209 Eliminate dangerous BSONElement string extraction methods

  • Fix: Change return type of BSONObj::getStringField to include size
    (StringData vs. char*). A char* only contains the data with an ending NULL
    termination. Whereas a StringData contains data + size so caller knows how
    to interpret data if there are embedded NULLs.
  • Cleanup: Remove old tag - CachedSizeTag - that disambiguated BSONElement ctors.
    A dangling reference to 'maxLen' in a comment led me to this historical issue.
    $ git log -S'maxLen' – src/mongo/bson/bsonelement.h
    commit 0d38ef5
    Author: Mathias Stearn mathias@10gen.com
    Date: Tue Dec 19 14:23:08 2017 -0500

SERVER-32302 Compute BSONElement sizes eagerly

  • Test: Add tests for NULL bytes being returned by getStringField
  • $ ninja -j400 +bson_obj_test
  • Cleanup: Move BSONElement::valuestr() from public to private
  • Cleanup: Remove BSONElement::valuestrsafe()
  • Cleanup: Remove all external callers of valuestr/valuestrsafe and cleanup
    their callsites with better alternatives.
  • Cleanup: Make multi-line BSONElement & BSONObj public API comments
    conform to style guidelines
Comment by Githook User [ 13/Jan/22 ]

Author:

{'name': 'Matt Kneiser', 'email': 'matt.kneiser@mongodb.com', 'username': 'themattman'}

Message: SERVER-27209 Propagate removal of BSONElement::valuestr()

  • Fix: Remove `valuestr()` to remove potentially dangerous handling of embedded NULLs in string fields
  • Fix: Change signedness of size parameter to unsigned
  • Fix: Change callsites of `BSONObj::getStringField()` to account for changed return type `char*` -> `StringData`

Related PR in server repo: [SERVER-27209 Eliminate dangerous BSONElement string extraction methods](https://github.com/10gen/mongo/pull/2579)
Branch: master
https://github.com/10gen/mongo-enterprise-modules/commit/65f2a1b1ba3bfacac7d349aae6391563ab3b4702

Comment by David Storch [ 29/Nov/16 ]

BSONElement::valuestr() should probably go away entirely, although a quick grep suggests that there are currently about 180 callers.

Generated at Thu Feb 08 04:14:28 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.