Directory traversal security vulnerability in serveStatic

[slashnick]

• Used appropriate template for the issue type
• Searched both open and closed issues for duplicates of this issue
• Title adequately and concisely reflects the feature or the bug

Restify Version: 8.6.1
Node.js Version: 17.6.0

Expected behaviour

Users can only request files from src/static/.

Actual behaviour

Users can request files in src/ outside of static/ by requesting /static/../<path>.

Repro case

Imagine this directory structure:

• package.json
• src/
  • app.js
  • static/
    • foo.txt

If you want to serve files out of static/, you might write some code like this:

const restify = require('restify');
const server = restify.createServer();

server.get('/static/*', restify.plugins.serveStatic({
  directory: './src',
}));

server.listen(8080);

With the above code, an attacker can read app.js by requesting /static/../app.js.

Note: this is hard to reproduce with curl or a browser, because they typically normalize the request path client-side. I reproduced this locally by running:

$ (echo -n 'GET /static/../app.js HTTP/1.1\r\n\r\n' && sleep .5) | nc localhost 8080

Cause

Path normalization occurs in serveStatic, after the request routing has occurred. A request for /static/../app.js matches the route for /static/*.

Possible fixes

• Normalize the request path before doing any routing
• Deprecate or remove appendRequestPath: true
• Deprecate or remove serveStatic in favor of serveStaticFiles

Are you willing and able to fix this?

No

[kolbma]

node-restify/lib/plugins/static.js

Lines 169 to 184 in 839fb4a

	file = path.join(opts.directory, decodeURIComponent(opts.file));
	} else if (opts.appendRequestPath) {
	file = path.join(opts.directory, decodeURIComponent(req.path()));
	} else {
	var dirBasename = path.basename(opts.directory);
	var reqpathBasename = path.basename(req.path());
	if (
	path.extname(req.path()) === '' &&
	dirBasename === reqpathBasename
	) {
	file = opts.directory;
	} else {
	file = path.join(
	opts.directory,
	decodeURIComponent(path.basename(req.path()))

Because of the decodeURIComponent() the traversal might also be possible with appendRequestPath: false: %2F..%2Fapp.js

There should be a check if file is below opts.directory.

[kolbma]

The check is there:

node-restify/lib/plugins/static.js

Lines 92 to 93 in 839fb4a

	var p = path.normalize(opts.directory).replace(/\\/g, '/');
	var re = new RegExp('^' + escapeRE(p) + '/?.*');

node-restify/lib/plugins/static.js

Lines 194 to 197 in 839fb4a

	if (!re.test(file.replace(/\\/g, '/'))) {
	next(new NotAuthorizedError('%s', req.path()));
	return;
	}

But the normalization of file got lost by these changes of @ap0 9f84a85

[kolbma]

Seems to be also a problem with the check...
#1692

And the correct error for this case would be not NotAuthorizedError but ForbiddenError.

[kolbma]

And the check could be done faster without RE-matching -> string startsWith().