feat(fs): add safeguard to ensure all paths are within expected directory

[Churro]

Changes

A preemptive check to ensure that potentially insecure relative or absolute paths are strictly contained within an expected folder. This is a safeguard to prevent potential path traversal attacks that may occur in case paths from processed repositories are used in file system operations.

node provides a guideline to prevent directory traversal. With an arbitrary path in fileName, the check could be simply:

const { localDir } = GlobalConfig.get();
const localDirName = upath.join(localDir, fileName);
if (localDir && !localFileName.startsWith(localDir)) {
  logger.warn('Preventing access to file outside the base directory');
  return null;
}

• upath.join() concats the two parameters and normalizes the result (see here).
  This means that upath.join('/tmp/renovate', '../../etc/passwd') becomes /etc/passwd

The only issue with path.join() is that this breaks when file system logic like . or ./ is used. E.g., with localDir = '.' and fileName = 'asdf', the join result is simply asdf and the startsWith check would fail. This would be the case with the tests in https://github.com/renovatebot/renovate/blob/main/lib/modules/manager/npm/post-update/yarn.spec.ts
(Note: Changing these tests to use '' instead of '.' would essentially be an alternative to changing all join ops to resolve)

The alternative used in this PR is path.resolve(), which basically enforces that all paths are relative to localDir. Good examples on the differences between join and resolve can also be found here.

Test repository: https://github.com/Churro/renovate-applyfrom-traversal/blob/master/dummy/build.gradle
-> only works so far when the commit in this PR is applied on #16030

Output:

WARN: Preventing access to file outside the base directory (repository=Churro/renovate-applyfrom-traversal)
WARN: Failed to process Gradle file (repository=Churro/renovate-applyfrom-traversal)
       "scriptFilePath": "../somefile.gradle"
WARN: Failed to process Gradle file (repository=Churro/renovate-applyfrom-traversal)
       "scriptFilePath": "somefile_other_file.gradle"
WARN: Failed to process Gradle file (repository=Churro/renovate-applyfrom-traversal)
       "scriptFilePath": "dummy/etc/somefile_other_other_file.gradle"

What can be noticed is that wandering around directories using ../ still works unless you leave the base dir. Other absolute paths are rewritten by path.resolve() to be relative to base dir.

Context

Related to #16030 (comment)

Documentation (please check one with an [x])

• I have updated the documentation, or
• No documentation update is required

How I've tested my work (please tick one)

I have verified these changes via:

• Code inspection only, or
• Newly added/modified unit tests, or
• No unit tests but ran on a real repository, or
• Both unit tests + ran on a real repository

[rarkins]

Should we warn for each of these? Ie they aren't expect yo occur so are either something malicious or will be surfacing bugs we would have otherwise missed?

[Churro]

Should we warn for each of these? Ie they aren't expect yo occur so are either something malicious or will be surfacing bugs we would have otherwise missed?

If unusual or malicious, it may useful to see that in logs too. I guess that more covert bugs or edge cases with artifact updates may simply go unnoticed otherwise.

[viceice]

we also should make those properties mandatory, as they never should be null or undefined at runtime.

[Churro]

we also should make those properties mandatory, as they never should be null or undefined at runtime.

Probably in another PR? That change would affect > 20 tests and it's not really related to this safeguard but more of a general thing...

[zharinov]

I like the concept of file traversal guarding is introduced in the code now, and further improvements can be made in separate PRs. This one looks good to me.

[Churro]

@viceice, do you agree with @zharinov that this PR is fine for now and that improvements to properties can still be made separately? IMO the safeguard itself already adds noteworthy value

[viceice]

i do not agree @zharinov we should make this as safe as possible now. I'm pretty sure it will be forgotten to do.

[zharinov]

i do not agree @zharinov we should make this as safe as possible now. I'm pretty sure it will be forgotten to do.

My point was to wait for this PR to be merged, so I'll be able to apply these safeguards to the functions of proxies.ts as well. I have a draft issue to do this on my personal GitHub project.

[viceice]

i do not agree @zharinov we should make this as safe as possible now. I'm pretty sure it will be forgotten to do.

My point was to wait for this PR to be merged, so I'll be able to apply these safeguards to the functions of proxies.ts as well. I have a draft issue to do this on my personal GitHub project.

ok, sounds good 👍

[viceice]

@rarkins @zharinov we should probably throw an error here and abort further processing.

so function shou be rename to validatePathInBaseDir or something like that and return never?

[rarkins]

"abort further processing" = stop running on the repo?

[viceice]

yes

[zharinov]

Sounds reasonable + further we need to ensure we don't use fs and fs-extra directly (i.e. migrate and add lint rule)

[zharinov]

@Churro Thank you for your help. If you don't mind, I'll continue to work on it on my own

Superseded by #16313