[SERVER-69009] Narrowing conversion of timestamp components in mozjs valuewriter Created: 19/Aug/22  Updated: 29/Oct/23  Resolved: 14/Dec/22

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

Type: Bug Priority: Major - P3
Reporter: Dan Larkin-York Assignee: Justin Seyster
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Problem/Incident
causes SERVER-72446 jstests/core/validate_js_timestamp.js... Closed
Related
is related to SERVER-68309 Investigate for unsafe narrowing conv... Closed
Backwards Compatibility: Fully Compatible
Operating System: ALL
Sprint: QE 2022-09-19, QE 2022-10-03, QE 2022-10-17, QE 2022-10-31, QE 2022-11-14, QE 2022-11-28, QE 2022-12-12, QE 2022-12-26
Participants:
Linked BF Score: 59

 Description   

We could experience some unexpected, implementation-defined behavior if the components of the timestamp are doubles with values outside the allowed integer range.

src/mongo/scripting/mozjs/valuewriter.cpp:302:22: warning: narrowing conversion from 'double' to 'unsigned int' [bugprone-narrowing-conversions]
    return Timestamp(wrapper.getNumber("t"), wrapper.getNumber("i"));
                     ^
src/mongo/scripting/mozjs/valuewriter.cpp:302:46: warning: narrowing conversion from 'double' to 'unsigned int' [bugprone-narrowing-conversions]
    return Timestamp(wrapper.getNumber("t"), wrapper.getNumber("i"));
                                             ^
src/mongo/scripting/mozjs/valuewriter.cpp:453:30: warning: narrowing conversion from 'double' to 'unsigned int' [bugprone-narrowing-conversions]
                Timestamp ot(o.getNumber("t"), o.getNumber("i"));
                             ^
src/mongo/scripting/mozjs/valuewriter.cpp:453:48: warning: narrowing conversion from 'double' to 'unsigned int' [bugprone-narrowing-conversions]
                Timestamp ot(o.getNumber("t"), o.getNumber("i"));
  



 Comments   
Comment by Githook User [ 14/Dec/22 ]

Author:

{'name': 'Justin Seyster', 'email': 'justin.seyster@mongodb.com', 'username': 'jseyster'}

Message: SERVER-69009 Validate Timestamp objects returned from $function
Branch: master
https://github.com/mongodb/mongo/commit/2774a5a7a1ded801ec19c2ba6a89715b038fb850

Comment by Dan Larkin-York [ 08/Sep/22 ]

justin.seyster@mongodb.com Thanks for taking a look! I also agree the usassert approach sounds quite reasonable. If we backport the changes, there's some small chance this will cause errors in existing scripts/applications, but I think that would be preferable to silently doing something potentially wrong due to undefined behavior. And we can always just not backport it, since it seems a low-risk bug anyway. But I'll leave that decision up to your judgment.

Comment by Mohammad Dashti (Inactive) [ 08/Sep/22 ]

justin.seyster@mongodb.com, thanks for taking a look into it.

We've already discussed this on this Slack thread: https://mongodb.slack.com/archives/C03STQ6G8RL/p1660831758698009

I agree with justin.seyster@mongodb.com's suggestion, as a similar change is done for this kind of issue (e.g., here):

My suggestion is to add uassert}}s at the two locations referenced by this ticket to ensure that {{t and o are still in uint32_t range. Users will still be able to pass around invalid Timestamp objects, but any time they cross the barrier from JavaScript to mongod, the ranges will be revalidated.

Comment by Justin Seyster [ 07/Sep/22 ]

There is a hidden bug here, but it's minor.

These conversions should be correct, because the constructor for JavaScript Timestamp objects validates that both t and i values are within the bounds of a uint32_t.

However, it is possible to bypass validation by assigning to an already constructed Timestamp:

db.un.find({}, {
    computedField: {
        $function: {
            body: function(field) {
                let timestamp = Timestamp(1, 1);
                timestamp.t = field;  // <-- Bypass validation
 
                // This Timestamp object will be get passed to 'ValueWriter::toTimestamp()' with a
                // 't' value that has not been range checked.
                return timestamp;
            },
            args: ["$unvalidatedUserData"],
            lang: "js"
        }
    }
});

I don't think this will affect customers, though, and we probably don't want to spend a lot of time on it. My suggestion is to add uassert}}s at the two locations referenced by this ticket to ensure that {{t and o are still in uint32_t range. Users will still be able to pass around invalid Timestamp objects, but any time they cross the barrier from JavaScript to mongod, the ranges will be revalidated.

The bulletproof solution would be modifying the C++ wrapper that specifies the `Timestamp` object so that t and {i}} are getter/setter properties, giving us a hook we can use to validate new values. I don't know if it's possible to add a setter-style function to the kind of object `Timestamp` is, however, and JavaScript may still provide a way to make an unchecked modification to the underlying fields anyway.

Comment by Kyle Suarez [ 30/Aug/22 ]

arun.banala@mongodb.com / steve.tarzia@mongodb.com, does either Justin or Mohammad have bandwidth to take a look at this?

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