Uploaded image for project: 'Core Server'
  1. Core Server
  2. SERVER-51720

BSONElement::_binDataVector incorrect bindata-type-2 bounds

    XMLWordPrintableJSON

Details

    • Icon: Bug Bug
    • Resolution: Fixed
    • Icon: Major - P3 Major - P3
    • 4.9.0
    • None
    • None
    • None
    • Fully Compatible
    • ALL
    • Service arch 2020-11-02

    Description

      I could be wrong, but I think BSONElement::_binDataVector uses incorrect bounds for the std::vector<uint8_t> result constructor when retrieving the data for a bindata-type-2 (the ByteArrayDeprecated bindata subtype).

      https://github.com/mongodb/mongo/blob/8751e79bc03b5c4c679040440ac76481fd3db356/src/mongo/bson/bsonelement.h#L565-L567

          std::vector<uint8_t> _binDataVector() const {
              if (binDataType() != ByteArrayDeprecated) {
                  return std::vector<uint8_t>(reinterpret_cast<const uint8_t*>(value()) + 5,
                                              reinterpret_cast<const uint8_t*>(value()) + 5 +
                                                  valuestrsize());
              } else {
                  // Skip the extra int32 size
                  return std::vector<uint8_t>(reinterpret_cast<const uint8_t*>(value()) + 4,
                                              reinterpret_cast<const uint8_t*>(value()) + 4 +
                                                  valuestrsize() - 4);
      

      I believe the +4 offsets in the else clause are incorrect, and it needs to be:

                  return std::vector<uint8_t>(reinterpret_cast<const uint8_t*>(value()) + 9,
                                              reinterpret_cast<const uint8_t*>(value()) + 9 +
                                                  valuestrsize() - 4);
      

      The difference between ByteArrayDeprecated (binDataType==2) and other binData payloads is that it has a wasted redundant 4-byte length field. So we need to skip over that when returning the binData payload. I think the intend of the +4 was to skip over this length, but it's incorrectly specified here. It should be that we skip 4 beyond what we'd do for other binData types, and they already skip 5.

      I'll note also that valuestrsize() might be smaller than 4 if the BSON is malformed, so we should skip 4 bytes, but no more than valuestrsize(), to ensure that the vector arguments are a valid range.

      In other words,

          std::vector<uint8_t> _binDataVector() const {
              auto first = reinterpret_cast<const uint8_t*>(value()) + 5;
              auto last = first + valuestrsize();
              if (binDataType() == ByteArrayDeprecated)
                  first += std::min<size_t>(4, last - first);  // skip extra int32 size.
              return {first, last};
      }
      

      Attachments

        Activity

          People

            billy.donahue@mongodb.com Billy Donahue
            billy.donahue@mongodb.com Billy Donahue
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: