Add testcases for all documented safeguards

[ChALkeR]

To clarify: I am not blaming anyone. I view this as a problem of the process that we used, the aim of this issue is to seek how we can actually improve things. I myself LGTMd the change that introduced the initial safeguard without a testcase.

This is a post-mortem of https://github.com/nodejs-private/security/issues/202 (see 40a7bee).

That itself was a minor issue which is very unlikely to be exploitable (more unlikely than the second one fixed in the same release), but it revealed what I think is a process problem.

That issue was introduced in #18790 which removed the corresponding safeguard from the code while optimizing performance. The safeguard was initially introduced in #4682, and was documented in the code — that didn't save it from being removed:

This prevents accidentally sending in a number that would be interpretted as a start offset.

My opinion is that a testcase should have been introduced at the same time when the comment was. Or, perhaps, even instead of the comment — the comment turned out to be not effective.

So, the proposal is to:

1. Collect a list of similar comments in the codebase that warn against changing some code or explain why is it needed — that hints that the common usecase might work with that code removed, but doing so is expected to break some rare usecase or cause a potential security issue.
2. Try removing/modifying the safegaurds and see if any tests break, make a list of safeguards that do not break tests when are tampered with.
3. Add missing tests.
4. Update to try spotting such things (i.e. untested expectations) in the future while reviewing PRs. Should be fairly easy to spot things that were worth a comment in the code and to question if those are tested?

There might be none — but I would prefer if we re-checked that.

Side note: code coverage is not an effective measure to collect the data here or prevent the issue.

/cc @nodejs/security-wg

[lirantal]

I can assume a bolder statement made in comments about the importance of the code would make a person think twice before they are making a change, and that may still be there to explain the rational, however having tests for these cases sounds really good to me as it will also break CI and anyone from making the change. The only problem with that is somebody refactoring the code might also refactor the tests out and could replace them completely, ending up with another safeguards disappearing.

[BridgeAR]

If I recall correct I concentrated on the code and while playing around I moved bits and peaces. I did read the comment but I refactoring _fill quite a lot and at some point the comment did not seem important anymore. Once gone, it did not make it's way back in... It's too bad I missed it but in the end I also think we should just take our lesson from it to use tests instead of comments for things like these.
I also agree with @lirantal that having a stronger comment would be good. I suggest to use all caps for security related comments. That should make it very clear.

[mcollina]

I totally agree with the approach @ChALkeR.

[lirantal]

Question is how do we enforce the process so it isn't overlooked again?
What about:

1. Security related code comments can use a @SECURITY code comment tag with a clear comment
2. Updating the github PR template with another check item to confirm that no secure code denoted by the SECURITY token was tampered with

And obviously we need to add appropriate tests but that's an on-going effort. Would it make sense to put the security related tests in their own dedicated test suite so that if someone will need to refactor related code in that area and their tests it will raise red flags that if code there changes it should be for good reasons and properly reviewed?

[ChALkeR]

Preliminary grep for (accident|prevents|ensure| sure ) in lib and src:

lib/internal/process/main_thread_only.js:166:    // Make sure it's not accidentally inherited by child processes.
lib/internal/bootstrap/node.js:272:        // Make sure it's not accidentally inherited by child processes.
src/node_http2.cc:1505:      // Note that nghttp2 currently prevents this from happening for SETTINGS
src/node.cc:2998:  // handles after beforeExit terminates. This prevents unrefed timers
src/node_url.cc:1804:            // the condition port <= 0xffff prevents integer overflow
src/tracing/node_trace_writer.h:47:  // Prevents concurrent R/W on state related to serialized trace data
src/tracing/node_trace_writer.h:50:  // Prevents concurrent R/W on state related to write requests.
lib/internal/repl/recoverable.js:17:  //       This prevents us from declaring partial tokens (like '2e') as
lib/internal/http2/core.js:2004:        // before the stream is officially closed. This prevents a bug
lib/tty.js:93:  // Prevents interleaved or dropped stdout/stderr output for terminals.
src/node_file.cc:1821:  //    need to call StringBytes::Write() to ensure proper byte swapping.
src/node_messaging.cc:639:  // want to serialize the message to ensure spec-compliant behavior w.r.t.
src/node.h:638: * This object should be stack-allocated to ensure that it is contained in a
src/node_http2.cc:108:  // code. This ensures that the flow of data over the connection
src/node_http2.cc:807:// Used as one of the Padding Strategy functions. Will attempt to ensure
src/node_http2.h:323:  // Attempts to ensure that the frame is 8-byte aligned
src/node_http2.h:325:  // Padding will ensure all data frames are maxFrameSize
src/node.cc:193:// Ensures that __metadata trace events are only emitted
src/node_crypto.cc:276:// Ensure that OpenSSL has enough entropy (at least 256 bits) for its PRNG.
src/node_crypto.cc:311:  // Ensure that OpenSSL's PRNG is properly seeded.
src/node_crypto.cc:649:  // Just to ensure that `ERR_peek_last_error` below will return only errors
src/node_crypto.cc:4639:    CheckEntropy();  // Ensure that OpenSSL's PRNG is properly seeded.
src/tls_wrap.cc:321:  // Ensure that the progress will be made and `InvokeQueued` will be called.
lib/internal/wrap_js_stream.js:121:      // Ensure that write is dispatched asynchronously.
lib/internal/wrap_js_stream.js:160:      // Ensure that this is called once in case of error
lib/internal/wrap_js_stream.js:168:      // Ensure that write was dispatched
lib/internal/fs/promises.js:172:// All of the functions are defined as async in order to ensure that errors
lib/internal/process/per_thread.js:40:    // If a previous value was passed in, ensure it has the correct shape.
lib/internal/process/per_thread.js:85:  // Ensure that a previously passed in value is valid. Currently, the native
lib/internal/util/comparisons.js:223:  // Ensure reflexivity of deepEqual with `arguments` objects.
lib/internal/util.js:73:    // Setting this (rather than using Object.setPrototype, as above) ensures
lib/internal/timers.js:146:  // Ensure that msecs fits into signed int32
lib/internal/http2/core.js:460:      // We have to ensure that it is a properly serialized
lib/internal/http2/core.js:819:  // ensures that those at l`east get cleared out.
lib/internal/http2/core.js:1363:      // ensure they are doing the right thing or the payload data will
lib/internal/http2/core.js:1604:  // ensure that the RST_STREAM frame is sent after the stream ID has
lib/internal/http2/core.js:1624:    // but ensures that those are buffered until the handle has
lib/internal/http2/core.js:2444:  // fs.fstat(). Assuming fstat is successful, a check is made to ensure
lib/buffer.js:125:  // Ensure aligned slices
lib/console.js:301:  // Ensures that label is a string, and only things that can be
lib/fs.js:139:// Ensure that callbacks run in the global context. Only use this function
lib/_http_client.js:213:      // For the Host header, ensure that IPv6 addresses are enclosed
lib/_http_client.js:411:  // Ensure that no further data will come out of the socket
lib/_tls_wrap.js:579:  // Ensure that we'll cycle through internal openssl's state
lib/net.js:1322:  // ensure handle hasn't closed
lib/_http_server.js:329:  // Ensure that the server property of the socket is correctly set.
lib/child_process.js:583:  // We may want to pass data in on any given fd, ensure it is a valid buffer
lib/_stream_readable.js:797:// Ensure readable listeners eventually get something
lib/_stream_writable.js:374:  // we must ensure that previous needDrain will not be reset to false.
lib/url.js:33:// This ensures setURLConstructor() is called before the native
lib/url.js:534:  // ensure it's an object, and not a string url.
src/string_bytes.cc:598:  // We know how much we'll write, just make sure that there's space.
src/node_file.cc:346:    // a read is in progress. Moving it into a local variable makes sure that
src/callback_scope.cc:102:  // Make sure the stack unwound properly. If there are nested MakeCallback's
src/cares_wrap.cc:598:    // Make sure the channel object stays alive during the query lifetime.
src/node_http2.cc:101:  // Make sure closed connections aren't kept around, taking up memory.
src/node_http2.cc:381:  // Make sure the start address is aligned appropriately for an nghttp2_nv*.
src/node_http2.cc:437:  // Make sure the start address is aligned appropriately for an nghttp2_nv*.
src/node_http2.cc:1798:    // Makre sure that there was no read previously active.
src/env.h:99:// Make sure that any macro V defined for use with the PER_ISOLATE_* macros is
src/env.cc:144:  // We'll be creating new objects so make sure we've entered the context.
src/env.cc:201:  // Make sure there are no re-used libuv wrapper objects.
src/util.h:308:  // Make sure enough space for `storage` entries is available.
src/node.cc:2342:  // Make sure file descriptors 0-2 are valid before we start logging anything.
src/stream_pipe.cc:31:  // In particular, this makes sure that they are garbage collected as a group,
src/base64.h:132:  // We know how much we'll write, just make sure that there's space.
src/env-inl.h:828:  // Make sure there was no existing element with these values.
src/node_process.cc:72:/* MAX_PATH is in characters, not bytes. Make sure we have enough headroom. */
src/node_process.cc:645:          "process.env property is deprecated. Please make sure to convert the "
src/node_options-inl.h:276:  // The first entry is the process name. Make sure it ends up in the V8 argv,
src/node_crypto.cc:4997:    // Make sure that the CSPRNG is properly seeded so the results are secure.
src/node_http_parser.cc:316:      // Make sure Buffer will be in parent HandleScope
src/tls_wrap.cc:70:  // sc comes from an Unwrap. Make sure it was assigned.
lib/internal/wrap_js_stream.js:53:        // Make sure that no further `data` events will happen.
lib/internal/modules/cjs/loader.js:326:    // path.resolve will make sure from.length >=3 in Windows.
lib/internal/modules/cjs/loader.js:484:  // make sure require('./path') and require('path') get distinct ids, even
lib/internal/process/main_thread_only.js:166:    // Make sure it's not accidentally inherited by child processes.
lib/internal/process/stdio.js:85:        // Make sure the stdin can't be `.end()`-ed
lib/internal/util/comparisons.js:367:      // Remove the matching element to make sure we do not check that again.
lib/internal/util/types.js:21:// Cached to make sure no userland code can tamper with it.
lib/internal/util/inspect.js:783:  // This limit also makes sure that huge objects don't block the event loop
lib/internal/util/inspect.js:879:      // Use the existing functionality. This makes sure the indentation and
lib/internal/util.js:101:// Move the "slow cases" to a separate function to make sure this function gets
lib/internal/bootstrap/node.js:272:        // Make sure it's not accidentally inherited by child processes.
lib/internal/bootstrap/node.js:638:      // If we handled an error, then make sure any ticks get processed
lib/internal/timers.js:86:// Make sure the linked list only shows the minimal necessary information.
lib/internal/errors.js:265:  // to make sure it is the same as the one created in C++
lib/internal/http2/core.js:2381:  // default. In respondWithFile, the file is checked to make sure it is a
lib/internal/assert.js:43:  // The util.inspect default values could be changed. This makes sure the
lib/internal/assert.js:305:        // Reset on each call to make sure we handle dynamically set environment
lib/internal/async_hooks.js:268:// the user to safeguard this call and make sure it's zero'd out when the
lib/internal/streams/buffer_list.js:161:  // Make sure the linked list only shows the minimal necessary information.
lib/_http_outgoing.js:360:      // Make sure we don't end the 0\r\n\r\n at the end of the message.
lib/_http_client.js:397:    // and we need to make sure we don't double-fire the error event.
lib/_tls_wrap.js:334:  // Make sure to setup all required properties like: `connecting` before
lib/_tls_wrap.js:450:  // Make sure we are not doing it on OpenSSL's stack
lib/net.js:1594:    // Increment connections to be sure that, even if all sockets will be closed
lib/timers.js:241:// Make sure the linked list only shows the minimal necessary information.
lib/timers.js:321:  // to `list` was created. Make sure they're the same instance of the list
lib/timers.js:383:  // if active is called later, then we want to make sure not to insert again
lib/path.js:153:        // the drive cwd is not available. We're sure the device is not
lib/path.js:444:    // Make sure that the joined path doesn't start with two slashes, because
lib/assert.js:191:        // expression is read. Make sure multi byte characters are preserved
lib/assert.js:236:  // Make sure the limit is set to 1. Otherwise it could fail (<= 0) or it
lib/assert.js:283:    // Set the stack trace limit to zero. This makes sure unexpected token
lib/assert.js:313:    // Make sure to always set the cache! No matter if the message is
lib/assert.js:542:    // Special handle errors to make sure the name and the message are compared
lib/assert.js:730:    // Make sure we actually have a stack trace!
lib/_stream_readable.js:57:  // This is a hack to make sure that our error handler is attached before any
lib/_stream_readable.js:508:    // emit 'readable' now to make sure it gets picked up.
lib/_stream_readable.js:693:  // Make sure our error handler is attached before userland ones.
lib/async_hooks.js:11:// For userland AsyncResources, make sure to emit a destroy event when the
lib/domain.js:60:        // resource.promise instanceof Promise make sure that the
lib/domain.js:148:    // Make sure the first listener for `uncaughtException` always clears
lib/_http_common.js:197:      // Make sure the parser's stack has unwound before deleting the
lib/url.js:404:    // First, make 100% sure that any "autoEscape" chars get

Next step should be identifying which of those are actual safeguards, and which miss tests.

[antsmartian]

@ChALkeR: Just to add an update here. The first two files listed over here:

lib/internal/process/main_thread_only.js:166:    // Make sure it's not accidentally inherited by child processes.
lib/internal/bootstrap/node.js:272:        // Make sure it's not accidentally inherited by child processes.

I guess they have corresponding test case in these files:

test-child-process-fork-and-spawn.js
test-cluster-basic.js

respectively.

[antsmartian]

@ChALkeR Again few updates:

lib/internal/util/inspect.js:783:  // This limit also makes sure that huge objects don't block the event loop
lib/internal/util/inspect.js:879:      // Use the existing functionality. This makes sure the indentation and

has corresponding test cases in test-util-inspect-long-running.js and test-util-inspect-namespace.

I'm not sure how are we going to track all these. May be we can add these items along with checkboxs and check them when we find appropriate test case(s)? Just a thought..

[eragon512]

hey, I'd like to take up this issue, if its still available