Skip to content

Commit 850ff02

Browse files
RafaelGSStargos
RafaelGSS
authored and
targos
committed
src,permission: resolve path on fs_permission
Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com> PR-URL: #52761 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
1 parent dc41c13 commit 850ff02

21 files changed

+68
-131
lines changed

‎doc/api/permissions.md

-2
Original file line numberDiff line numberDiff line change
@@ -583,8 +583,6 @@ There are constraints you need to know before using this system:
583583

584584
#### Limitations and Known Issues
585585

586-
* When the permission model is enabled, Node.js may resolve some paths
587-
differently than when it is disabled.
588586
* Symbolic links will be followed even to locations outside of the set of paths
589587
that access has been granted to. Relative symbolic links may allow access to
590588
arbitrary files and directories. When starting applications with the

‎lib/internal/fs/utils.js

+1-28
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,8 @@ const {
2424
Symbol,
2525
TypedArrayPrototypeAt,
2626
TypedArrayPrototypeIncludes,
27-
uncurryThis,
2827
} = primordials;
2928

30-
const permission = require('internal/process/permission');
31-
3229
const { Buffer } = require('buffer');
3330
const {
3431
UVException,
@@ -68,8 +65,6 @@ const kType = Symbol('type');
6865
const kStats = Symbol('stats');
6966
const assert = require('internal/assert');
7067

71-
const { encodeUtf8String } = internalBinding('encoding_binding');
72-
7368
const {
7469
fs: {
7570
F_OK = 0,
@@ -736,31 +731,10 @@ const validatePath = hideStackFrames((path, propName = 'path') => {
736731
);
737732
});
738733

739-
// TODO(rafaelgss): implement the path.resolve on C++ side
740-
// See: https://github.com/nodejs/node/pull/44004#discussion_r930958420
741-
// The permission model needs the absolute path for the fs_permission
742-
const resolvePath = pathModule.resolve;
743-
const { isBuffer: BufferIsBuffer, from: BufferFrom } = Buffer;
744-
const BufferToString = uncurryThis(Buffer.prototype.toString);
745-
function possiblyTransformPath(path) {
746-
if (permission.isEnabled()) {
747-
if (typeof path === 'string') {
748-
return resolvePath(path);
749-
}
750-
assert(isUint8Array(path));
751-
if (!BufferIsBuffer(path)) path = BufferFrom(path);
752-
// Avoid Buffer.from() and use a C++ binding instead to encode the result
753-
// of path.resolve() in order to prevent path traversal attacks that
754-
// monkey-patch Buffer internals.
755-
return encodeUtf8String(resolvePath(BufferToString(path)));
756-
}
757-
return path;
758-
}
759-
760734
const getValidatedPath = hideStackFrames((fileURLOrPath, propName = 'path') => {
761735
const path = toPathIfFileURL(fileURLOrPath);
762736
validatePath(path, propName);
763-
return possiblyTransformPath(path);
737+
return path;
764738
});
765739

766740
const getValidatedFd = hideStackFrames((fd, propName = 'fd') => {
@@ -994,7 +968,6 @@ module.exports = {
994968
getValidatedFd,
995969
getValidatedPath,
996970
handleErrorFromBinding,
997-
possiblyTransformPath,
998971
preprocessSymlinkDestination,
999972
realpathCacheKey: Symbol('realpathCacheKey'),
1000973
getStatFsFromBinding,

‎src/compile_cache.cc

+2-2
Original file line numberDiff line numberDiff line change
@@ -354,15 +354,15 @@ bool CompileCacheHandler::InitializeDirectory(Environment* env,
354354
cache_dir);
355355

356356
if (UNLIKELY(!env->permission()->is_granted(
357-
permission::PermissionScope::kFileSystemWrite, cache_dir))) {
357+
env, permission::PermissionScope::kFileSystemWrite, cache_dir))) {
358358
Debug("[compile cache] skipping cache because write permission for %s "
359359
"is not granted\n",
360360
cache_dir);
361361
return false;
362362
}
363363

364364
if (UNLIKELY(!env->permission()->is_granted(
365-
permission::PermissionScope::kFileSystemRead, cache_dir))) {
365+
env, permission::PermissionScope::kFileSystemRead, cache_dir))) {
366366
Debug("[compile cache] skipping cache because read permission for %s "
367367
"is not granted\n",
368368
cache_dir);

‎src/node_modules.cc

+1
Original file line numberDiff line numberDiff line change
@@ -308,6 +308,7 @@ const BindingData::PackageConfig* BindingData::TraverseParent(
308308
// to walk upwards
309309
if (UNLIKELY(is_permissions_enabled &&
310310
!env->permission()->is_granted(
311+
env,
311312
permission::PermissionScope::kFileSystemRead,
312313
std::string(check_path) + kPathSeparator))) {
313314
return nullptr;

‎src/node_worker.cc

+1-1
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ Worker::Worker(Environment* env,
8989
// Without this check, to use the permission model with
9090
// workers (--allow-worker) one would need to pass --allow-inspector as well
9191
if (env->permission()->is_granted(
92-
node::permission::PermissionScope::kInspector)) {
92+
env, node::permission::PermissionScope::kInspector)) {
9393
inspector_parent_handle_ =
9494
GetInspectorParentHandle(env, thread_id_, url.c_str(), name.c_str());
9595
}

‎src/permission/child_process_permission.cc

+2-1
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,8 @@ void ChildProcessPermission::Apply(Environment* env,
1515
deny_all_ = true;
1616
}
1717

18-
bool ChildProcessPermission::is_granted(PermissionScope perm,
18+
bool ChildProcessPermission::is_granted(Environment* env,
19+
PermissionScope perm,
1920
const std::string_view& param) const {
2021
return deny_all_ == false;
2122
}

‎src/permission/child_process_permission.h

+2-1
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,8 @@ class ChildProcessPermission final : public PermissionBase {
1515
void Apply(Environment* env,
1616
const std::vector<std::string>& allow,
1717
PermissionScope scope) override;
18-
bool is_granted(PermissionScope perm,
18+
bool is_granted(Environment* env,
19+
PermissionScope perm,
1920
const std::string_view& param = "") const override;
2021

2122
private:

‎src/permission/fs_permission.cc

+10-6
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
#include "fs_permission.h"
22
#include "base_object-inl.h"
33
#include "debug_utils-inl.h"
4+
#include "env.h"
45
#include "path.h"
56
#include "v8.h"
67

@@ -51,21 +52,23 @@ void FreeRecursivelyNode(
5152
}
5253

5354
bool is_tree_granted(
55+
node::Environment* env,
5456
const node::permission::FSPermission::RadixTree* granted_tree,
5557
const std::string_view& param) {
58+
std::string resolved_param = node::PathResolve(env, {param});
5659
#ifdef _WIN32
5760
// is UNC file path
58-
if (param.rfind("\\\\", 0) == 0) {
61+
if (resolved_param.rfind("\\\\", 0) == 0) {
5962
// return lookup with normalized param
6063
size_t starting_pos = 4; // "\\?\"
61-
if (param.rfind("\\\\?\\UNC\\") == 0) {
64+
if (resolved_param.rfind("\\\\?\\UNC\\") == 0) {
6265
starting_pos += 4; // "UNC\"
6366
}
6467
auto normalized = param.substr(starting_pos);
6568
return granted_tree->Lookup(normalized, true);
6669
}
6770
#endif
68-
return granted_tree->Lookup(param, true);
71+
return granted_tree->Lookup(resolved_param, true);
6972
}
7073

7174
void PrintTree(const node::permission::FSPermission::RadixTree::Node* node,
@@ -146,19 +149,20 @@ void FSPermission::GrantAccess(PermissionScope perm, const std::string& res) {
146149
}
147150
}
148151

149-
bool FSPermission::is_granted(PermissionScope perm,
152+
bool FSPermission::is_granted(Environment* env,
153+
PermissionScope perm,
150154
const std::string_view& param = "") const {
151155
switch (perm) {
152156
case PermissionScope::kFileSystem:
153157
return allow_all_in_ && allow_all_out_;
154158
case PermissionScope::kFileSystemRead:
155159
return !deny_all_in_ &&
156160
((param.empty() && allow_all_in_) || allow_all_in_ ||
157-
is_tree_granted(&granted_in_fs_, param));
161+
is_tree_granted(env, &granted_in_fs_, param));
158162
case PermissionScope::kFileSystemWrite:
159163
return !deny_all_out_ &&
160164
((param.empty() && allow_all_out_) || allow_all_out_ ||
161-
is_tree_granted(&granted_out_fs_, param));
165+
is_tree_granted(env, &granted_out_fs_, param));
162166
default:
163167
return false;
164168
}

‎src/permission/fs_permission.h

+2-1
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,8 @@ class FSPermission final : public PermissionBase {
1818
void Apply(Environment* env,
1919
const std::vector<std::string>& allow,
2020
PermissionScope scope) override;
21-
bool is_granted(PermissionScope perm,
21+
bool is_granted(Environment* env,
22+
PermissionScope perm,
2223
const std::string_view& param) const override;
2324

2425
struct RadixTree {

‎src/permission/inspector_permission.cc

+2-1
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,8 @@ void InspectorPermission::Apply(Environment* env,
1414
deny_all_ = true;
1515
}
1616

17-
bool InspectorPermission::is_granted(PermissionScope perm,
17+
bool InspectorPermission::is_granted(Environment* env,
18+
PermissionScope perm,
1819
const std::string_view& param) const {
1920
return deny_all_ == false;
2021
}

‎src/permission/inspector_permission.h

+2-1
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,8 @@ class InspectorPermission final : public PermissionBase {
1515
void Apply(Environment* env,
1616
const std::vector<std::string>& allow,
1717
PermissionScope scope) override;
18-
bool is_granted(PermissionScope perm,
18+
bool is_granted(Environment* env,
19+
PermissionScope perm,
1920
const std::string_view& param = "") const override;
2021

2122
private:

‎src/permission/permission.cc

+2-2
Original file line numberDiff line numberDiff line change
@@ -50,10 +50,10 @@ static void Has(const FunctionCallbackInfo<Value>& args) {
5050
return;
5151
}
5252
return args.GetReturnValue().Set(
53-
env->permission()->is_granted(scope, *utf8_arg));
53+
env->permission()->is_granted(env, scope, *utf8_arg));
5454
}
5555

56-
return args.GetReturnValue().Set(env->permission()->is_granted(scope));
56+
return args.GetReturnValue().Set(env->permission()->is_granted(env, scope));
5757
}
5858

5959
} // namespace

‎src/permission/permission.h

+8-6
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ namespace permission {
2727

2828
#define THROW_IF_INSUFFICIENT_PERMISSIONS(env, perm_, resource_, ...) \
2929
do { \
30-
if (UNLIKELY(!(env)->permission()->is_granted(perm_, resource_))) { \
30+
if (UNLIKELY(!(env)->permission()->is_granted(env, perm_, resource_))) { \
3131
node::permission::Permission::ThrowAccessDenied( \
3232
(env), perm_, resource_); \
3333
return __VA_ARGS__; \
@@ -37,7 +37,7 @@ namespace permission {
3737
#define ASYNC_THROW_IF_INSUFFICIENT_PERMISSIONS( \
3838
env, wrap, perm_, resource_, ...) \
3939
do { \
40-
if (UNLIKELY(!(env)->permission()->is_granted(perm_, resource_))) { \
40+
if (UNLIKELY(!(env)->permission()->is_granted(env, perm_, resource_))) { \
4141
node::permission::Permission::AsyncThrowAccessDenied( \
4242
(env), wrap, perm_, resource_); \
4343
return __VA_ARGS__; \
@@ -48,10 +48,11 @@ class Permission {
4848
public:
4949
Permission();
5050

51-
FORCE_INLINE bool is_granted(const PermissionScope permission,
51+
FORCE_INLINE bool is_granted(Environment* env,
52+
const PermissionScope permission,
5253
const std::string_view& res = "") const {
5354
if (LIKELY(!enabled_)) return true;
54-
return is_scope_granted(permission, res);
55+
return is_scope_granted(env, permission, res);
5556
}
5657

5758
FORCE_INLINE bool enabled() const { return enabled_; }
@@ -73,11 +74,12 @@ class Permission {
7374
void EnablePermissions();
7475

7576
private:
76-
COLD_NOINLINE bool is_scope_granted(const PermissionScope permission,
77+
COLD_NOINLINE bool is_scope_granted(Environment* env,
78+
const PermissionScope permission,
7779
const std::string_view& res = "") const {
7880
auto perm_node = nodes_.find(permission);
7981
if (perm_node != nodes_.end()) {
80-
return perm_node->second->is_granted(permission, res);
82+
return perm_node->second->is_granted(env, permission, res);
8183
}
8284
return false;
8385
}

‎src/permission/permission_base.h

+2-1
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,8 @@ class PermissionBase {
4444
virtual void Apply(Environment* env,
4545
const std::vector<std::string>& allow,
4646
PermissionScope scope) = 0;
47-
virtual bool is_granted(PermissionScope perm,
47+
virtual bool is_granted(Environment* env,
48+
PermissionScope perm,
4849
const std::string_view& param = "") const = 0;
4950
};
5051

‎src/permission/worker_permission.cc

+2-1
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,8 @@ void WorkerPermission::Apply(Environment* env,
1515
deny_all_ = true;
1616
}
1717

18-
bool WorkerPermission::is_granted(PermissionScope perm,
18+
bool WorkerPermission::is_granted(Environment* env,
19+
PermissionScope perm,
1920
const std::string_view& param) const {
2021
return deny_all_ == false;
2122
}

‎src/permission/worker_permission.h

+2-1
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,8 @@ class WorkerPermission final : public PermissionBase {
1515
void Apply(Environment* env,
1616
const std::vector<std::string>& allow,
1717
PermissionScope scope) override;
18-
bool is_granted(PermissionScope perm,
18+
bool is_granted(Environment* env,
19+
PermissionScope perm,
1920
const std::string_view& param = "") const override;
2021

2122
private:

‎test/fixtures/permission/fs-traversal.js

+16-16
Original file line numberDiff line numberDiff line change
@@ -31,49 +31,49 @@ const uint8ArrayTraversalPath = new TextEncoder().encode(traversalPath);
3131
fs.writeFile(traversalPath, 'test', common.expectsError({
3232
code: 'ERR_ACCESS_DENIED',
3333
permission: 'FileSystemWrite',
34-
resource: path.toNamespacedPath(resolve(traversalPath)),
34+
resource: path.toNamespacedPath(traversalPath),
3535
}));
3636
}
3737

3838
{
3939
fs.readFile(traversalPath, common.expectsError({
4040
code: 'ERR_ACCESS_DENIED',
4141
permission: 'FileSystemRead',
42-
resource: path.toNamespacedPath(resolve(traversalPath)),
42+
resource: path.toNamespacedPath(traversalPath),
4343
}));
4444
}
4545

4646
{
4747
assert.throws(() => {
48-
fs.mkdtempSync(traversalFolderPath)
49-
}, {
48+
fs.mkdtempSync(traversalFolderPath);
49+
}, common.expectsError({
5050
code: 'ERR_ACCESS_DENIED',
5151
permission: 'FileSystemWrite',
52-
resource: resolve(traversalFolderPath + 'XXXXXX'),
53-
});
52+
resource: traversalFolderPath + 'XXXXXX',
53+
}));
5454
}
5555

5656
{
57-
fs.mkdtemp(traversalFolderPath, common.expectsError({
58-
code: 'ERR_ACCESS_DENIED',
59-
permission: 'FileSystemWrite',
60-
resource: resolve(traversalFolderPath + 'XXXXXX'),
61-
}));
57+
fs.mkdtemp(traversalFolderPath, common.expectsError({
58+
code: 'ERR_ACCESS_DENIED',
59+
permission: 'FileSystemWrite',
60+
resource: traversalFolderPath + 'XXXXXX',
61+
}));
6262
}
6363

6464
{
6565
fs.readFile(bufferTraversalPath, common.expectsError({
6666
code: 'ERR_ACCESS_DENIED',
6767
permission: 'FileSystemRead',
68-
resource: resolve(traversalPath),
68+
resource: traversalPath,
6969
}));
7070
}
7171

7272
{
7373
fs.readFile(uint8ArrayTraversalPath, common.expectsError({
7474
code: 'ERR_ACCESS_DENIED',
7575
permission: 'FileSystemRead',
76-
resource: resolve(traversalPath),
76+
resource: traversalPath,
7777
}));
7878
}
7979

@@ -93,7 +93,7 @@ const uint8ArrayTraversalPath = new TextEncoder().encode(traversalPath);
9393
}, common.expectsError({
9494
code: 'ERR_ACCESS_DENIED',
9595
permission: 'FileSystemRead',
96-
resource: resolve(cwd.toString()),
96+
resource: cwd.toString(),
9797
}));
9898
}
9999

@@ -118,15 +118,15 @@ const uint8ArrayTraversalPath = new TextEncoder().encode(traversalPath);
118118
}, common.expectsError({
119119
code: 'ERR_ACCESS_DENIED',
120120
permission: 'FileSystemRead',
121-
resource: resolve(traversalPathWithExtraChars),
121+
resource: traversalPathWithExtraChars,
122122
}));
123123

124124
assert.throws(() => {
125125
fs.readFileSync(new TextEncoder().encode(traversalPathWithExtraBytes.toString()));
126126
}, common.expectsError({
127127
code: 'ERR_ACCESS_DENIED',
128128
permission: 'FileSystemRead',
129-
resource: resolve(traversalPathWithExtraChars),
129+
resource: traversalPathWithExtraChars,
130130
}));
131131
}
132132

0 commit comments

Comments
 (0)