Fix RCE in gateway.downloadBackup

[bnematzadeh]

Description

There is a Remote Code Execution vulnerability in the gateway controller that allows a logged-in user to execute arbitrary commands on the service. The vulnerable sink is located in the gateway.downloadBackup file. The fileUrl value corresponds to the file_url field in the /api/v1/gateway/backup/restore endpoint.

In the gateway.downloadBackup file, the GLADYS_GATEWAY_BACKUP_KEY value is first checked, and if it is not set, an error is thrown (this can be triggered by calling the POST /api/v1/variable/GLADYS_GATEWAY_BACKUP_KEY endpoint). To decrypt the backup, the exec function is used. However, the value of encryptedBackupFilePath is used in exec without being sanitized.

Before executing exec, the server sends a request to the fileUrl. If the link is problematic, an Axios error is returned. If a query string is included in the URL, its value is ignored. The issue here is that a malicious link containing an encrypted file and a hash value with an arbitrary command can be crafted. When this hash value is used in exec, the command gets executed.

Below, I have attached a sample POC and the command I executed. To fix this issue, the # character must also be stripped from the fileWithoutSignedParams.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.53%. Comparing base (7a24976) to head (c58a539).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2171      +/-   ##
==========================================
+ Coverage   98.52%   98.53%   +0.01%     
==========================================
  Files         868      876       +8     
  Lines       14295    14402     +107     
==========================================
+ Hits        14084    14191     +107     
  Misses        211      211              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests
• JS Bundle Analysis - Avoid shipping oversized bundles

[relativeci]

#2888 Bundle Size — 10.34MiB (0%).

c58a539(current) vs 8fff0ca master#2883(baseline)

Warning

Bundle contains 3 duplicate packages – View duplicate packages

Bundle metrics  no changes

	Current #2888	Baseline #2883
Initial JS	5.62MiB	5.62MiB
Initial CSS	304.89KiB	304.89KiB
Cache Invalidation	0%	57.81%
Chunks	51	51
Assets	172	172
Modules	1507	1507
Duplicate Modules	21	21
Duplicate Code	0.84%	0.84%
Packages	124	124
Duplicate Packages	3	3

Bundle size by type  no changes

	Current #2888	Baseline #2883
JS	7.41MiB	7.41MiB
IMG	2.49MiB	2.49MiB
CSS	321.68KiB	321.68KiB
Fonts	93.55KiB	93.55KiB
Other	17.7KiB	17.7KiB
HTML	13.58KiB	13.58KiB

Bundle analysis report Branch bnematzadeh:gladys-sec-2 Project dashboard

---

^{Generated by RelativeCI Documentation Report issue}

[Pierre-Gilles]

Thanks for this PR! I'm not sure this could ever be exploited as the user is hosting the server himself, but it's still a good thing to fix this :)

Why not use an URL validator to make sure the URL is really valid?

We use Joi in Gladys:

const Joi = require('joi');

const schema = Joi.string().uri();

// Example validation
const url = 'https://example.com';
const { error, value } = schema.validate(url);

Don't forget to add the tests :)

[bnematzadeh]

This scenario can be exploited by a new user created by an admin by calling the mentioned endpoint. Using the JoI library you referred to, I defined a regex with the pattern method to remove the specified characters if they appear at the end of a URL and return the cleaned URL. This is because simply using the uri method in this case is not sufficient and can be bypassed with other payloads. I made some changes to coreErrors and added a new custom error for URLs, which I have used both in the gateway file and in its corresponding test file.

[bnematzadeh]

Oops, I think we have a problem. With the changes I've made to the downloadBackup file, two other tests are failing because when the downloadBackup method is called with a non-URL input, it reaches the validator and causes an error. I made a change in the file where, using a function, it checks whether the input is a URL or not. If it is a URL, the validateUrl function is called on it, and if the URL is valid, the value is returned. Otherwise, we're dealing with a local file.

[Pierre-Gilles]

@bnematzadeh I tried with Joi with the native uri check and it works:

Why not use this one? Do you have example of security issues with this ?

[bnematzadeh]

@bnematzadeh I tried with Joi with the native uri check and it works:

Why not use this one? Do you have example of security issues with this ?

The reason I didn’t use the native URI is that it recognizes the following URL as a valid URL and doesn’t handle the rest of the fragment. A custom command can be used inside $() after the fragment, and if the native URI is used, this technique can be bypassed. That’s why I stripped these two characters entirely.

[Pierre-Gilles]

Ok thanks for you answer! I have a few feedbacks on the PR

[Pierre-Gilles]

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

[Pierre-Gilles]

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

Will this be used elsewhere?

[bnematzadeh]

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.

[Pierre-Gilles]

Using fileWithoutSignedParams instead ? (as it was before)

const fileWithoutSignedParams = fileUrl.split('?')[0];