Directory traversal security vulnerability in serveStatic

[slashnick]

• [x] Used appropriate template for the issue type
• [x] Searched both open and closed issues for duplicates of this issue
• [x] 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]

https://github.com/restify/node-restify/blob/839fb4a2b5e5434d43e60e1abb936e153c659c31/lib/plugins/static.js#L169-L184

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: https://github.com/restify/node-restify/blob/839fb4a2b5e5434d43e60e1abb936e153c659c31/lib/plugins/static.js#L92-L93

https://github.com/restify/node-restify/blob/839fb4a2b5e5434d43e60e1abb936e153c659c31/lib/plugins/static.js#L194-L197

But the normalization of file got lost by these changes of @ap0 https://github.com/restify/node-restify/commit/9f84a8542d6d502ef3e094c0b321173c13780094

[kolbma]

Seems to be also a problem with the check...
https://github.com/restify/node-restify/pull/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().