Permission logic in fs.open(), fs.openSync(), and fsPromises.open() can easily be bypassed

[tniessen]

The permission logic in these functions seems flawed. Using fs.open() or fs.openSync(), both read and write restrictions can easily be bypassed simply by passing extra flags. Some examples:

• O_RDWR | 0x10000000 gives both read and write access - regardless of any restrictions.
• O_RDONLY | O_NOCTTY gives read access - even if all file system access has been blocked.
• O_RDONLY | O_TEMPORARY allows deleting files on Windows - even without write access.

fsPromises.open() contains similarly obvious flaws, but it also contains a mostly redundant (and likely also incorrect) check that always requires read permission, even if opening in a write-only mode. Still, as long as read permission is granted, code can open the file for writing using, for example, O_RDWR | O_NOFOLLOW.

Overall, this combination of multiple logical flaws trivially allows arbitrary read and write access to any file, even when access should be restricted.

---

I'm opening this as a public issue because the feature hasn't been released yet due to the previous vulnerability that was found by @cjihrig (see #46975 (comment)).

---

The flawed validation logic is implemented here:

node/src/node_file.cc

Lines 1968 to 1982 in 334bb17

	// Open can be called either in write or read
	if (flags == O_RDWR) {
	// TODO(rafaelgss): it can be optimized to avoid O(2*n)
	THROW_IF_INSUFFICIENT_PERMISSIONS(
	env, permission::PermissionScope::kFileSystemRead, pathView);
	THROW_IF_INSUFFICIENT_PERMISSIONS(
	env, permission::PermissionScope::kFileSystemWrite, pathView);
	} else if ((flags & ~(UV_FS_O_RDONLY | UV_FS_O_SYNC)) == 0) {
	THROW_IF_INSUFFICIENT_PERMISSIONS(
	env, permission::PermissionScope::kFileSystemRead, pathView);
	} else if ((flags & (UV_FS_O_APPEND | UV_FS_O_TRUNC | UV_FS_O_CREAT |
	UV_FS_O_WRONLY)) != 0) {
	THROW_IF_INSUFFICIENT_PERMISSIONS(
	env, permission::PermissionScope::kFileSystemWrite, pathView);
	}

The incorrect and/or redundant additional check in fsPromises.open() is implemented here:

node/src/node_file.cc

Lines 2014 to 2015 in 334bb17

	THROW_IF_INSUFFICIENT_PERMISSIONS(
	env, permission::PermissionScope::kFileSystemRead, pathView);

Followed by the same validation logic as above:

node/src/node_file.cc

Lines 2023 to 2037 in 334bb17

	// Open can be called either in write or read
	if (flags == O_RDWR) {
	// TODO(rafaelgss): it can be optimized to avoid O(2*n)
	THROW_IF_INSUFFICIENT_PERMISSIONS(
	env, permission::PermissionScope::kFileSystemRead, pathView);
	THROW_IF_INSUFFICIENT_PERMISSIONS(
	env, permission::PermissionScope::kFileSystemWrite, pathView);
	} else if ((flags & ~(UV_FS_O_RDONLY | UV_FS_O_SYNC)) == 0) {
	THROW_IF_INSUFFICIENT_PERMISSIONS(
	env, permission::PermissionScope::kFileSystemRead, pathView);
	} else if ((flags & (UV_FS_O_APPEND | UV_FS_O_TRUNC | UV_FS_O_CREAT |
	UV_FS_O_WRONLY)) != 0) {
	THROW_IF_INSUFFICIENT_PERMISSIONS(
	env, permission::PermissionScope::kFileSystemWrite, pathView);
	}