Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix RCE in gateway.downloadBackup #2171

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions server/lib/gateway/gateway.downloadBackup.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
const path = require('path');
const fse = require('fs-extra');
const fs = require('fs');
const { isURL, validateUrl } = require('../../utils/url');
const logger = require('../../utils/logger');
const { EVENTS, WEBSOCKET_MESSAGE_TYPES } = require('../../utils/constants');
const { exec } = require('../../utils/childProcess');
Expand All @@ -20,15 +21,15 @@ async function downloadBackup(fileUrl) {
if (encryptKey === null) {
throw new NotFoundError('GLADYS_GATEWAY_BACKUP_KEY_NOT_FOUND');
}
// Extract file name
const fileWithoutSignedParams = fileUrl.split('?')[0];

const value = isURL(fileUrl) ? validateUrl(fileUrl) : fileUrl;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure it still works like that, for context, the fileUrl looks like that (it's a dummy URL) :

https://example_storage.com/b6131425-8f90-4a05-ba00-12daf37f8279.enc?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=SDFDSFDSFDSFDSFFDF%2F20241202%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20241202T110928Z&X-Amz-Expires=21600&X-Amz-Signature=6768767678HHkjhjdhfjdshfjhdksfhjdshf&X-Amz-SignedHeaders=host

So the get parameters should be removed before doing path.basename()

Having the variable being name value isn't very clear

const restoreFolderPath = path.join(this.config.backupsFolder, RESTORE_FOLDER);
// we ensure the restore backup folder exists
await fse.ensureDir(restoreFolderPath);
// we empty the restore backup folder
await fse.emptyDir(restoreFolderPath);

const encryptedBackupName = path.basename(fileWithoutSignedParams, '.enc');
const encryptedBackupName = path.basename(value, '.enc');
const encryptedBackupFilePath = path.join(restoreFolderPath, `${encryptedBackupName}.enc`);
const compressedBackupFilePath = path.join(restoreFolderPath, `${encryptedBackupName}.gz`);

Expand Down
16 changes: 15 additions & 1 deletion server/test/lib/gateway/gateway.downloadBackup.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ const GladysGatewayClientMock = require('./GladysGatewayClientMock.test');
const getConfig = require('../../../utils/getConfig');

const { EVENTS, WEBSOCKET_MESSAGE_TYPES } = require('../../../utils/constants');
const { NotFoundError } = require('../../../utils/coreErrors');
const { NotFoundError, InvalidURL } = require('../../../utils/coreErrors');

const { fake, assert } = sinon;
const Gateway = proxyquire('../../../lib/gateway', {
Expand Down Expand Up @@ -94,4 +94,18 @@ describe('gateway.downloadBackup', () => {
},
});
});

it('should throw an error for invalid backup url', async () => {
const backupUrl = 'https://test.example/path/test.enc#&?`id`';
try {
await gateway.downloadBackup(backupUrl);
assert.fail();
} catch (e) {
expect(e)
.instanceOf(InvalidURL)
.haveOwnProperty('message', 'INVALID_URL');
}

assert.notCalled(event.emit);
});
});
26 changes: 26 additions & 0 deletions server/test/utils/url.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
const { expect } = require('chai');
const { isURL, validateUrl } = require('../../utils/url');

describe('url.validation', () => {
it('should return true for a valid url', () => {
const url = 'https://example.com';
expect(isURL(url)).to.equal(true);
});

it('should return false for an invalid url', () => {
const url = '/a/b';
expect(isURL(url)).to.equal(false);
});

it('should return valid url', () => {
const url = 'https://example.com/test';
expect(validateUrl(url)).to.be.equal('https://example.com/test');
});

it('should throw an error for malicious url', () => {
const url = 'https://example.com/test?#a';
expect(() => {
validateUrl(url);
}).to.throw(Error);
});
});
7 changes: 7 additions & 0 deletions server/utils/coreErrors.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,12 @@ class NotFoundError extends Error {
}
}

class InvalidURL extends Error {
constructor(message) {
super();
this.message = message;
}
}
class NoValuesFoundError extends Error {
constructor(message) {
super();
Expand Down Expand Up @@ -80,4 +86,5 @@ module.exports = {
ConflictError,
ForbiddenError,
TooManyRequests,
InvalidURL,
};
42 changes: 42 additions & 0 deletions server/utils/url.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
const Joi = require('joi');
const { InvalidURL } = require('./coreErrors');

/**
* @description Typeof url.
* @param {string} str - The url of the backup.
* @returns {boolean} Return true for valid url.
* @example
* isURL();
*/
const isURL = (str) => {
try {
const url = new URL(str);
return url.protocol === 'http:' || url.protocol === 'https:';
} catch (_) {
return false;
}
};

/**
* @description Validate the url.
* @param {string} url - The url of the backup.
* @returns {string} Return a valid url.
* @example
* validateUrl();
*/
function validateUrl(url) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You created a generic function "validateUrl" in utils, but isn't this behavior very gateway.downloadBackup specific?

Will this be used elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What change should I make so that the GET parameter is removed before it’s passed to path.basename()? The validator I wrote, in my opinion, can be used in other places, but it’s largely tied to gateway.downloadBackup.

const schema = Joi.string()
.uri()
.pattern(/^[^?#]*$/, '');
const { error, value } = schema.validate(url);
if (error) {
throw new InvalidURL('INVALID_URL');
} else {
return value;
}
}

module.exports = {
isURL,
validateUrl,
};
Loading