Allow unpacking symlinks that don't escape cwd

[nwalters512]

Given that I was able to write a failing test to demonstrate my issue, I opted to open a PR to facilitate discussion. If you'd rather I close this and open an issue instead, please let me know!

340eb28 ended up being a regression for my application, which relies on being able to un-tar archives that contain symlinks that point within the archive itself. Note that I do not use, and do not want to use, preservePaths: true because of the obvious security implications. To the best of my knowledge, there's no security reason to avoid resolving symlinks that don't refer to other paths outside the archive.

If there's agreement that (a) this is a problem and (b) a reasonable fix would be to permit relative symlinks that don't point outside of the tarball, I'd be happy to add an attempted fix to this PR. My proposal would be to replace this check with one that doesn't just check for the presence of ..:

node-tar/src/unpack.ts

Line 278 in 911c886

parts.includes('..') ||

If behaving this way under the default preservePaths: false still isn't acceptable, I'd propose a new option to opt into what should still be safe and correct behavior.

[isaacs]

Oh, I see, so the target is within the tarball cwd, but just includes ... Yeah, that's overly broad, I see. It should check that resolving the linkpath doesn't take it outside the cwd.

[nwalters512]

It should check that resolving the linkpath doesn't take it outside the cwd.

Exactly, yeah. I'll put up a proposed fix in this PR - feel free to take it or rework it yourself. Thanks for the quick response.

[nwalters512]

@isaacs I updated the implementation and some more of the existing tests in 5b7e937.

[isaacs]

Not quite. For example, it could be an absolute path that includes .., like consider what would happen if the linkpath is /../etc/passwd.

I'd add a field === 'path' in the next section, but let it fall through to the absolute strip in either case.

[isaacs]

I think this fixes it:

diff --git a/src/unpack.ts b/src/unpack.ts
index 8f256d0..51c9488 100644
--- a/src/unpack.ts
+++ b/src/unpack.ts
@@ -275,43 +275,43 @@ export class Unpack extends Parser {
 
     const parts = p.split('/')
 
-    // For linkpath, check if the resolved path escapes cwd rather than
-    // just rejecting any path with '..' - relative symlinks like
-    // '../sibling/file' are valid if they resolve within the cwd.
-    if (field === 'linkpath' && parts.includes('..')) {
-      // Resolve linkpath relative to the entry's directory. `path.posix` is
-      // safe to use because we're operating on tar paths, not a filesystem.
-      const entryDir = path.posix.dirname(entry.path)
-      const resolved = path.posix.normalize(
-        path.posix.join(entryDir, p),
-      )
-      // If the resolved path escapes (starts with ..), reject it
-      if (resolved.startsWith('../') || resolved === '..') {
-        this.warn(
-          'TAR_ENTRY_ERROR',
-          `${field} escapes extraction directory`,
-          {
-            entry,
-            [field]: p,
-          },
-        )
-        return false
-      }
-      // Valid relative symlink that stays within cwd
-      return true
-    }
-
     if (
       parts.includes('..') ||
       /* c8 ignore next */
       (isWindows && /^[a-z]:\.\.$/i.test(parts[0] ?? ''))
     ) {
-      this.warn('TAR_ENTRY_ERROR', `${field} contains '..'`, {
-        entry,
-        [field]: p,
-      })
-      // not ok!
-      return false
+      // For linkpath, check if the resolved path escapes cwd rather than
+      // just rejecting any path with '..' - relative symlinks like
+      // '../sibling/file' are valid if they resolve within the cwd.
+      // For paths, they just simply may not ever use .. at all.
+      if (field === 'path') {
+        this.warn('TAR_ENTRY_ERROR', `${field} contains '..'`, {
+          entry,
+          [field]: p,
+        })
+        // not ok!
+        return false
+      } else {
+        // Resolve linkpath relative to the entry's directory.
+        // `path.posix` is safe to use because we're operating on
+        // tar paths, not a filesystem.
+        const entryDir = path.posix.dirname(entry.path)
+        const resolved = path.posix.normalize(
+          path.posix.join(entryDir, p),
+        )
+        // If the resolved path escapes (starts with ..), reject it
+        if (resolved.startsWith('../') || resolved === '..') {
+          this.warn(
+            'TAR_ENTRY_ERROR',
+            `${field} escapes extraction directory`,
+            {
+              entry,
+              [field]: p,
+            },
+          )
+          return false
+        }
+      }
     }
 
     // strip off the root

[nwalters512]

Great catch. I added a failing test in 1f4b85b and fixed it in 46e98fd.