Harden creation of child processes

[watson]

Add general protection against RCE vulnerabilities similar to the one described in CVE-2019-7609.

Unfortunately, our normal testing framework Jest, changes the behavior of process.env in a way so that we can't test that our patches works as expected. We, therefore, have to introduce another testing framework in this PR that we can use to test our hardening functionality. I've chosen tape because it's very simple, is widely used in the Node.js community, and doesn't add any magic to the environment.

Closes #49605

[watson]

@elasticmachine merge upstream

[watson]

@elasticmachine merge upstream

[jportner]

I still want to play around with this, but some initial feedback:

1. This hardening is pretty important, can we add some unit tests to verify it's working as intended? This will also help with maintainability for others who may not be well-versed in this topic.
2. What do you think about updating the Codeowners file to tag the Security team as a joint co-owner of the src/setup_node_env directory?

[jportner]

Okay, I tested this out and it looks great so far!

Two more questions:

1. Does it also make sense to harden subprocess.send?
2. I tested this out by creating a file named "scripts/test.ts", and it doesn't appear that the process.env hardening works. I'm not sure what I'm doing wrong, can you take a look below?

require('../src/setup_node_env');

const { create, ObjectPrototype } = require('object-prototype');

const obj1 = process.env;
const obj2 = {};

console.log(Object.prototype.isPrototypeOf(obj1)); // expected: false, actual: true
console.log(ObjectPrototype.isPrototypeOf(obj1)); // expected: true, actual: false

console.log(Object.prototype.isPrototypeOf(obj2)); // expected: true, actual: true
console.log(ObjectPrototype.isPrototypeOf(obj2)); // expected: false, actual: false

Object.prototype.foo = 42;

console.log(obj1.foo); // expected: undefined, actual: 42
console.log(obj2.foo); // expected: 42, actual: 42

[watson]

@jportner Good catch! I found the bug and pushed a fix. This just shows the importance of tests, so as you suggested, I'll add some and push again.

[watson]

@elasticmachine merge upstream

[kobelb]

ACK: reviewing!

[kobelb]

I'm not overly fond of us splitting up this logic into two external node packages: object-prototype and object-prototype-functions.

In general, we've refrained from creating external repos and packages until there was a reason to do so. And when that time came, they've generally resided under the elastic organization. While I do see the merit in allowing others to benefit from the same approach that Kibana is taking to harden itself, it feels like we're doing so prematurely. I'd prefer we move this code to be within the Kibana repo until it's more mature and we deem it beneficial to make a separate package.

[watson]

@elasticmachine merge upstream

[watson]

@elasticmachine merge upstream

[watson]

The results of the new tests: https://github.com/elastic/kibana/pull/55697/checks?check_run_id=423032610

[kobelb]

ACK: will re-review first thing tomorrow

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 5042132

History

• 💚 Build #31657 succeeded 7ec6fe3
• 💚 Build #30465 succeeded 57c39fd
• 💚 Build #29696 succeeded dc6ea29
• 💔 Build #29683 failed dc6ea29
• 💚 Build #29673 succeeded b08d478

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[mistic]

I've tested that PR with #47998 and it did not affect it at all.
It LGTM