FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src,permission: resolve path on fs_permission · nodejs/node@850ff02 · GitHub

/ node Public

Commit 850ff02

Browse files
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

Lines changed: 68 additions & 131 deletions

‎doc/api/permissions.md‎

Lines changed: 0 additions & 2 deletions
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‎

Lines changed: 1 addition & 28 deletions
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‎

Lines changed: 2 additions & 2 deletions
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‎

Lines changed: 1 addition & 0 deletions
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‎

Lines changed: 1 addition & 1 deletion
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‎

Lines changed: 2 additions & 1 deletion
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‎

Lines changed: 2 additions & 1 deletion
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‎

Lines changed: 10 additions & 6 deletions
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‎

Lines changed: 2 additions & 1 deletion
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‎

Lines changed: 2 additions & 1 deletion
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
}

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL