From 328a58b292aac3f32e123b2cee43473d72ff1fee Mon Sep 17 00:00:00 2001 From: Ben Schwartz Date: Thu, 3 Feb 2022 16:51:29 -0500 Subject: [PATCH 1/3] Add a pre-routing filter This ensures that unauthorized requests never reach the routing step. --- src/shadowbox/server/manager_service.spec.ts | 54 +++++++++++++++++++- src/shadowbox/server/manager_service.ts | 27 ++++++++++ 2 files changed, 80 insertions(+), 1 deletion(-) diff --git a/src/shadowbox/server/manager_service.spec.ts b/src/shadowbox/server/manager_service.spec.ts index 363594a0..c65f28dd 100644 --- a/src/shadowbox/server/manager_service.spec.ts +++ b/src/shadowbox/server/manager_service.spec.ts @@ -798,7 +798,7 @@ describe('bindService', () => { `${PREFIX}/does-not-exist`, ].forEach(path => { it(`404 (${path})`, async () => { - // Ensure no methods are called. + // Ensure no methods are called on the Service. spyOnAllFunctions(service); jasmine.setDefaultSpyStrategy(fail); bindService(server, PREFIX, service); @@ -807,12 +807,64 @@ describe('bindService', () => { const response = await fetch(url); const body = await response.json(); + expect(response.status).toEqual(404); expect(body).toEqual({ code: 'ResourceNotFound', message: `${path} does not exist` }); }); }); + + // This is primarily a reverse testcase for the unauthorized case. + it(`standard routing for authorized queries`, async () => { + bindService(server, PREFIX, service); + // Verify that ordinary routing goes through the Router. + spyOn(server.router, "lookup").and.callThrough(); + + // This is an authorized request, so it will pass the prefix filter + // and reach the Router. + url.pathname = `${PREFIX}`; + const response = await fetch(url); + expect(response.status).toEqual(404); + await response.json(); + + expect(server.router.lookup).toHaveBeenCalled(); + }); + + // Check that unauthorized queries are rejected without ever reaching + // the routing stage. + [ + '/', + '/T', + '/TestApiPre', + '/TestApi123456', + '/TestApi123456789', + ].forEach(path => { + it(`no routing for unauthorized queries (${path})`, async () => { + bindService(server, PREFIX, service); + // Ensure no methods are called on the Router. + spyOnAllFunctions(server.router); + jasmine.setDefaultSpyStrategy(fail); + + // Try bare pathname. + url.pathname = path; + const response1 = await fetch(url); + expect(response1.status).toEqual(404); + await response1.json(); + + // Try a subpath that would exist if this were a valid prefix + url.pathname = `${path}/server`; + const response2 = await fetch(url); + expect(response2.status).toEqual(404); + await response2.json(); + + // Try an arbitrary subpath + url.pathname = `${path}/does-not-exist`; + const response3 = await fetch(url); + expect(response3.status).toEqual(404); + await response3.json(); + }); + }); }); class ShadowsocksManagerServiceBuilder { diff --git a/src/shadowbox/server/manager_service.ts b/src/shadowbox/server/manager_service.ts index 7ea57c7f..d4791af6 100644 --- a/src/shadowbox/server/manager_service.ts +++ b/src/shadowbox/server/manager_service.ts @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +import * as crypto from 'crypto'; import * as ipRegex from 'ip-regex'; import * as restify from 'restify'; import * as restifyErrors from 'restify-errors'; @@ -74,8 +75,34 @@ enum HttpSuccess { NO_CONTENT = 204, } +// Similar to String.startsWith(), but constant-time. +function timingSafeStartsWith(input: string, prefix: string): boolean { + if (input.length < prefix.length) { + return false; + } + const prefixBuf = Buffer.from(prefix); + const inputBuf = Buffer.from(input); + const overlap = inputBuf.slice(0, prefixBuf.length); + return crypto.timingSafeEqual(overlap, prefixBuf); +} + +// Returns a pre-routing hook that injects a 404 if the request does not +// start with `apiPrefix`. This filter runs in constant time. +function prefixFilter(apiPrefix: string): restify.RequestHandler { + return (req: restify.Request, res: restify.Response, next: restify.Next) => { + if (timingSafeStartsWith(req.path(), apiPrefix)) { + return next(); + } + // This error matches the router's default 404 response. + next(new restifyErrors.ResourceNotFoundError('%s does not exist', req.path())); + }; +} + export function bindService( apiServer: restify.Server, apiPrefix: string, service: ShadowsocksManagerService) { + // Reject unauthorized requests in constant time before they reach the routing step. + apiServer.pre(prefixFilter(apiPrefix)); + apiServer.put(`${apiPrefix}/name`, service.renameServer.bind(service)); apiServer.get(`${apiPrefix}/server`, service.getServer.bind(service)); apiServer.put( From 0219dc252b05174025aace75a19805fb96f3809b Mon Sep 17 00:00:00 2001 From: Ben Schwartz Date: Fri, 4 Feb 2022 16:44:32 -0500 Subject: [PATCH 2/3] Reduce special-case behavior for short inputs --- src/shadowbox/server/manager_service.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/shadowbox/server/manager_service.ts b/src/shadowbox/server/manager_service.ts index d4791af6..ae96a1c0 100644 --- a/src/shadowbox/server/manager_service.ts +++ b/src/shadowbox/server/manager_service.ts @@ -77,13 +77,13 @@ enum HttpSuccess { // Similar to String.startsWith(), but constant-time. function timingSafeStartsWith(input: string, prefix: string): boolean { - if (input.length < prefix.length) { - return false; - } const prefixBuf = Buffer.from(prefix); const inputBuf = Buffer.from(input); - const overlap = inputBuf.slice(0, prefixBuf.length); - return crypto.timingSafeEqual(overlap, prefixBuf); + const L = Math.min(inputBuf.length, prefixBuf.length) + const inputOverlap = inputBuf.slice(0, L); + const prefixOverlap = prefixBuf.slice(0, L); + const match = crypto.timingSafeEqual(inputOverlap, prefixOverlap); + return inputBuf.length >= prefixBuf.length && match; } // Returns a pre-routing hook that injects a 404 if the request does not From c27c82c20a821cae09cffe0ad37a85f855d55ee0 Mon Sep 17 00:00:00 2001 From: Ben Schwartz Date: Fri, 4 Feb 2022 16:53:33 -0500 Subject: [PATCH 3/3] Missing semicolon --- src/shadowbox/server/manager_service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shadowbox/server/manager_service.ts b/src/shadowbox/server/manager_service.ts index ae96a1c0..8dd934fa 100644 --- a/src/shadowbox/server/manager_service.ts +++ b/src/shadowbox/server/manager_service.ts @@ -79,7 +79,7 @@ enum HttpSuccess { function timingSafeStartsWith(input: string, prefix: string): boolean { const prefixBuf = Buffer.from(prefix); const inputBuf = Buffer.from(input); - const L = Math.min(inputBuf.length, prefixBuf.length) + const L = Math.min(inputBuf.length, prefixBuf.length); const inputOverlap = inputBuf.slice(0, L); const prefixOverlap = prefixBuf.slice(0, L); const match = crypto.timingSafeEqual(inputOverlap, prefixOverlap);