[SERVER-54418] mongos collMod command does not validate its reply Created: 09/Feb/21  Updated: 29/Oct/23  Resolved: 10/Feb/21

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

Type: Bug Priority: Major - P3
Reporter: A. Jesse Jiryu Davis Assignee: Moustafa Maher
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Backwards Compatibility: Minor Change
Operating System: ALL
Sprint: Repl 2021-02-22
Participants:

 Description   

I think there's a bug in cluster_collection_mod_cmd.cpp:

    void validateResult(const BSONObj& resultObj) final {
        auto ctx = IDLParserErrorContext("CollModReply");
        StringDataSet ignorableFields(
            {kWriteConcernErrorFieldName, ErrorReply::kOkFieldName, kTopologyVersionFieldName});
        if (!checkIsErrorStatus(resultObj, ctx)) {
            if (resultObj.hasField(kRawFieldName)) {
                const auto& rawData = resultObj[kRawFieldName];
                if (ctx.checkAndAssertType(rawData, Object)) {
                    for (const auto& element : rawData.Obj()) {
                        const auto& shardReply = element.Obj();
                        if (!checkIsErrorStatus(shardReply, ctx)) {
                            auto reply =
                                Reply::parse(ctx, shardReply.removeFields(ignorableFields));
                            coll_mod_reply_validation::validateReply(reply);
                        }
                    }
                }
            } else {
                auto reply = Reply::parse(ctx, resultObj.removeFields(ignorableFields));
                coll_mod_reply_validation::validateReply(reply);
            }
        }
    }

That will not call Reply::parse if the reply includes a "raw" field. Thus I think Reply::parse is never called?



 Comments   
Comment by Githook User [ 10/Feb/21 ]

Author:

{'name': 'Moustafa Maher', 'email': 'm.maher@10gen.com', 'username': 'moustafamaher'}

Message: SERVER-54418 mongos collMod command does not validate its reply
Branch: master
https://github.com/mongodb/mongo/commit/86e443952115d1b1d61eb7f372505191d31e11ab

Comment by A. Jesse Jiryu Davis [ 09/Feb/21 ]

Sounds good to me.

Comment by Moustafa Maher [ 09/Feb/21 ]

Yes you are right, there is no harm to move the else block, as the collModReply doesn't have any non-optional fields that would have failed parsing, although it will add more value instead.

I don't think we should add "raw" field to the idl file, as it is a custom field that we add from mongos only, and shouldn't be defined in the stable API, and we should add it to "ignorableFields", what do you think ?

Comment by A. Jesse Jiryu Davis [ 09/Feb/21 ]

Let's validate the whole reply, and ensure it conforms to the CollModReply IDL definition:

    CollModReply:
        description: "The collMod command's reply."
        strict: true
        fields:
            expireAfterSeconds_old:
                optional: true
                type: safeInt
            expireAfterSeconds_new:
                optional: true
                type: safeInt
            hidden_old:
                optional: true
                type: safeBool
            hidden_new:
                optional: true
                type: safeBool

For example, if mongos always adds the "raw" field, and we changed mongos in the future to add a "hidden_new" field of type "int", that would violate this IDL definition but the current code won't catch this violation because it doesn't call Reply::parse if "raw" is present. I propose:

    CollModReply:
        description: "The collMod command's reply."
        strict: true
        fields:
            expireAfterSeconds_old:
                optional: true
                type: safeInt
            expireAfterSeconds_new:
                optional: true
                type: safeInt
            hidden_old:
                optional: true
                type: safeBool
            hidden_new:
                optional: true
                type: safeBool
            # ADDED:
            raw:
                optional: true
                type: object

    void validateResult(const BSONObj& resultObj) final {
        auto ctx = IDLParserErrorContext("CollModReply");
        StringDataSet ignorableFields(
            {kWriteConcernErrorFieldName, ErrorReply::kOkFieldName, kTopologyVersionFieldName});
        if (!checkIsErrorStatus(resultObj, ctx)) {
            // MOVED FROM "ELSE" BLOCK:
            auto reply = Reply::parse(ctx, resultObj.removeFields(ignorableFields));
            coll_mod_reply_validation::validateReply(reply);
 
            if (resultObj.hasField(kRawFieldName)) {
                const auto& rawData = resultObj[kRawFieldName];
                if (ctx.checkAndAssertType(rawData, Object)) {
                    for (const auto& element : rawData.Obj()) {
                        const auto& shardReply = element.Obj();
                        if (!checkIsErrorStatus(shardReply, ctx)) {
                            auto reply =
                                Reply::parse(ctx, shardReply.removeFields(ignorableFields));
                            coll_mod_reply_validation::validateReply(reply);
                        }
                    }
                }
            }
        }
    }

Comment by Moustafa Maher [ 09/Feb/21 ]

But we can always call this:
auto reply = Reply::parse(ctx, resultObj.removeFields(ignorableFields));
whether the "raw" field exist or not, as the collModReply doesn't have any required field, so it will catch any extra fields.

Comment by Moustafa Maher [ 09/Feb/21 ]

top-level result-obj doesn't have anything else than "raw" if there are no errors:
look at output parameter in this method

Comment by A. Jesse Jiryu Davis [ 09/Feb/21 ]

Do we need to verify the top-level resultObj or not?

Comment by Moustafa Maher [ 09/Feb/21 ]

if the reply includes a "raw" field, we will go through each shard reply and call Reply::parse on it.
auto reply = Reply::parse(ctx, shardReply.removeFields(ignorableFields));

Generated at Thu Feb 08 05:33:29 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.