Add Security Checks Log

[dblythy]

Hello all,

I've worked on implementing a log that picks up schema, allowClientClassCreation, beforeFileSave, beforeConnect, mongoDB auth issues, and anything else I could think of. This isn't intended to pen-test a Parse Server, but just give developers some easy to follow pointers to make their backends safer.

It currently runs on creation of ParseServer, and returns a nice little payload that can hopefully easily be rendered on the dashboard. Can be turned off with disableSecurityChecks:true.

This PR is not ready for merge just yet - I was interested in your thoughts and suggestions, also suggestions around suggested CLPs for internal classes. I also need to write test cases.

This code also makes a few tests break with "open connections" - I was also wondering if there were any suggestions around that.

Here's what the current version outputs:

We found 17 potential security issues with your Parse Server:

Issue: Allow Client Class Creation is not recommended.
   Allow client class creation is potentially insecure as it allows any user - authorized or not - to create a new class.

Issue: No beforeFileSave Trigger
   Even if you don't store files, we strongly recommend using a beforeFileSave trigger to prevent unauthorized uploads.

Issue: Unrestricted access to port 27017
   It is possible to connect to the admin port of your mongoDb without authentication.

Issue: Unrestricted access to your database
   It is possible to connect to your mongoDb without username and password on your connection string.


CLP Issue in Class: _Role
   Unrestricted access to find.
   Unrestricted access to count.
   Unrestricted access to get.
   Unrestricted access to create.
   Unrestricted access to update.
   Unrestricted access to delete.
   Unrestricted access to addField.

CLP Issue in Class: _User
   Unrestricted access to find.
   Unrestricted access to count.
   Unrestricted access to get.
   Unrestricted access to create.
   Unrestricted access to update.
   Unrestricted access to delete.
   Unrestricted access to addField.

Reference to discussion in community forum.

[codecov]

Codecov Report

Merging #6973 (b4faede) into master (7f47b04) will decrease coverage by 0.15%.
The diff coverage is 83.54%.

@@            Coverage Diff             @@
##           master    #6973      +/-   ##
==========================================
- Coverage   94.00%   93.85%   -0.16%     
==========================================
  Files         172      174       +2     
  Lines       12835    12992     +157     
==========================================
+ Hits        12066    12194     +128     
- Misses        769      798      +29     

Impacted Files	Coverage Δ
src/Adapters/Storage/Mongo/MongoStorageAdapter.js	92.84% <ø> (-0.68%)	⬇️
src/Options/index.js	100.00% <ø> (ø)
src/SecurityChecks.js	77.46% <77.46%> (ø)
src/SecurityCheck.js	84.21% <84.21%> (ø)
src/ParseServer.js	90.25% <95.65%> (+0.66%)	⬆️
src/Options/Definitions.js	100.00% <100.00%> (ø)
src/Routers/CloudCodeRouter.js	100.00% <100.00%> (ø)
src/RestWrite.js	93.67% <0.00%> (ø)
... and 1 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f47b04...b4faede. Read the comment docs.

[cbaker6]

I think this is a great idea. If we end up adding code snippets for some of the security features in the docs, then it could be helpful to provide links to them.

For checking the database port, are you pulling these from the URI or using the default Mongo (should probably check Postgres also)?

Update: I’m looking from my phone, but I skimmed the code and it looks like you may have covered much of what I mentioned. My original comments was based on the output above, but it looks like the code may output more details?

[dblythy]

@cbaker6 I've added a few options that I could think of that would need to be checked for - and yeah this is functional. Just the advice on what it outputs will need to be adjusted, and I'm hoping that it'll evolve over time, and the function can be implemented into Parse Dashboard, with hopefully some click-through links to the relevant sections (e.g the schema warnings take you to the schema for that class).

As you mentioned, the code currently is intended to get the current databaseURI, and then removes any username:password combo, and attempts to connect. Maybe we could also do some validation around password strength for DB strings and masterKeys. I'm honestly not sure of the security issues with postgres as I've always used Mongo. If you have the capacity, would you be willing to add it to the security checks?

[mtrezza]

Can be turned off with disableSecurityChecks:true.

It looks like the checks would be on by default and be switched off using the optional parameter. This sounds like a good idea in the sense that Parse Server gives proactive security guidance.

However, we want to make sure that we don't make deployments actually more vulnerable by disclosing vulnerabilities in the logs without some developers being aware of it. We also want to make sure these vulnerabilities cannot be easily revealed through the PR, which requires some feature maturity. So I am not sure we should introduce that immediately.

I suggest to start by introducing the checks as disabled by default with a warning in the logs, if checks are not enabled. Later on, after we got some real-world feedback and potentially fixed some flaws, we could enable the check by default, with a highlighted warning in the change log. Therefore I would rename the option to securityChecks.

This is a great PR, one of many in the past days by @dblythy. If we had an award "PR machine of the month", it would surely be yours 😉

[dblythy]

I agree with all the points made @mtrezza. Maybe for an introduction, disabled by default, and then the function verifySecurityChecks could be called by the dashboard, in a Security section or something. That way, the only people accessing it, are the ones that actually intend on improving their security. What would you think of this approach?

I am honored Manuel, as I've said before, Parse really helped me when I was getting started and all my production servers are better off thanks to the great work of this community. I am in debt to you all, and it's good to be able to give back a little bit!

[mtrezza]

Yes, sounds like a good strategy. Disable securityChecks by default as long as we output only in the logs. Enable securityChecks by default once it's visible in the dashboard where authentication is required and when the feature is mature.

Log output of security warnings sounds like bad practice to me, so I think we should never enable log output by default. However, there may be users who do not use parse dashboard, so they could temporarily enable the output in the logs. So we'd have at least 2 security parameters and the option could look like this:

securityCheck: {
  enabled: true, // Is true if Parse Server should self-check the security of its current configuration. The results are visible in the Parse Dashboard. Default is true.
  logOutput: false // Is true if security warnings should be written to logs. This should only be enabled temporarily to not expose vulnerabilities in logs. Default is false.
}

As a side note, to clearly separate the terminology from issues that need to be fixed in source code rather than an unsafe configuration, we probably should not use the terms "vulnerability" or "issue" in relation to the security checks in the docs or source code.

[cbaker6]

I've been attempting to get any JS PR's I'm tied to merged in hopes to not have to look at JS for awhile and focus on the Parse-Swift SDK.

With that being said, I've made it a point to not let anything parse/postgres related slip behind mongo any further (and have attempted to catch it up) as I use Postgres instead of mongo. If no one else can add the Postgres parts, I can look. The makeup of the URI is similar to mongo (except for rs, Postgres only points to 1 Postgres server, which will then have it's own replication setup) and there's a custom URI parser that parse-server is using that can probably be leverage to do the field detection you are looking for. The fields the parser pulls are documented here, https://docs.parseplatform.org/parse-server/guide/#postgres

The usage of the parser occurs here:

parse-server/src/Adapters/Storage/Postgres/PostgresClient.js

Lines 7 to 26 in 7f3ea3f

	if (uri) {
	dbOptions = parser.getDatabaseOptionsFromURI(uri);
	}
	for (const key in databaseOptions) {
	dbOptions[key] = databaseOptions[key];
	}
	const initOptions = dbOptions.initOptions || {};
	initOptions.noWarnings = process && process.env.TESTING;
	const pgp = require('pg-promise')(initOptions);
	const client = pgp(dbOptions);
	if (dbOptions.pgOptions) {
	for (const key in dbOptions.pgOptions) {
	pgp.pg.defaults[key] = dbOptions.pgOptions[key];
	}
	}

[dblythy]

I like the suggestions. I've updated the PR, and I'll try work on postgres shortly.

As a side note, it feels bit clunky in my view to recommend a fileTrigger for users that never intend to have any files uploaded to their servers. Could we have a preventFileUpload:true or preventFileUpload:"*" // for !req.user maybe in the configuration?

How do we feel about maybe having a applyDefaultSchema (either in securityChecks or config) or something similar that applies a recommended CLP to _User, _Role, etc? Could be a good way to introduce people to CLP.

[davimacedo]

I like the suggestions. I've updated the PR, and I'll try work on postgres shortly.

As a side note, it feels bit clunky in my view to recommend a fileTrigger for users that never intend to have any files uploaded to their servers. Could we have a preventFileUpload:true or preventFileUpload:"*" // for !req.user maybe in the configuration?

How do we feel about maybe having a applyDefaultSchema (either in securityChecks or config) or something similar that applies a recommended CLP to _User, _Role, etc? Could be a good way to introduce people to CLP.

I think it would be good but I'd do that on a separate PR.

[mtrezza]

How do we feel about maybe having a applyDefaultSchema (either in securityChecks or config) or something similar that applies a recommended CLP to _User, _Role, etc? Could be a good way to introduce people to CLP.

If I understand the suggestion correctly, it is related to this. My impression from the discussion there is that Parse Server should apply the secure ACL by default for these classes, which can be changed manually by the dev, but depending on the security impact of that change, Parse Server would then log an "unsafe configuration warning", thanks to this PR.

[dblythy]

I have pushed a new commit with the following changes:

/GET endpoint for /advisoryChecks. This will return all the required data for (someone who is good with react, not me 😔) the creation of an Advisory panel in the dashboard.

Otherwise, you can run the server with advisoryChecks:true to view checks in the logger.

[mtrezza]

/GET endpoint for /advisoryChecks. (...) Otherwise, you can run the server with advisoryChecks:true to view checks in the logger.

• It should facilitate comprehension to keep the terminology uniform and use the configuration term securityChecks for the endpoint (or another term, but then it should be used in all places). The term advisoryChecks seems a bit too common and unintuitive, "advisory / check for what?", so I think securityChecks is the best term so far.

• I think we should add a test case for the /securityChecks endpoint that ensures it does not become accessible without master key at some point in the future.

• It is obvious, but we have to be careful here; an endpoint that reveals the weak security spots of a deployment is a gift to bots that scan for Parse Server deployments and get all vulnerabilities served on a silver platter if they can access that endpoint. In that context, it is beneficial that securityChecks becomes an extra endpoint, because it can be easily sealed off with a firewall rule.

Otherwise, you can run the server with advisoryChecks:true to view checks in the logger.

• Form the previous comments I would understand that logOutput also has to be set true to see the output in the logs, correct?

[dblythy]

It should facilitate comprehension to keep the terminology uniform and use the configuration term securityChecks for the endpoint (or another term, but then it should be used in all places). The term advisoryChecks seems a bit too common and unintuitive, "advisory / check for what?".

I was conscious of adding the word "security" to the code, hence why I used advisory. I misunderstood what you were saying about being sensitive regarding using the word "vulnerability". I'll change it back.

[mtrezza]

I misunderstood what you were saying about being sensitive regarding using the word "venerability". I'll change it back.

Apologies for my previous comment being unclear, what I meant was that the terms security issue and security vulnerability should not be used related to this feature in code, the Parse Dashboard UI or the docs, because their use would collide with the use as security bugs that needs to be fixed. I think securityCheck is different enough in its meaning that we can use it. Besides, it also describes the feature pretty accurately. Thanks for changing it back.

[dblythy]

No stress @mtrezza it was my misunderstanding.

[dblythy]

/GET endpoint for /advisoryChecks. (...) Otherwise, you can run the server with advisoryChecks:true to view checks in the logger.

• It should facilitate comprehension to keep the terminology uniform and use the configuration term securityChecks for the endpoint (or another term, but then it should be used in all places). The term advisoryChecks seems a bit too common and unintuitive, "advisory / check for what?", so I think securityChecks is the best term so far.
• I think we should add a test case for the /securityChecks endpoint that ensures it does not become accessible without master key at some point in the future.
• It is obvious, but we have to be careful here; an endpoint that reveals the weak security spots of a deployment is a gift to bots that scan for Parse Server deployments and get all vulnerabilities served on a silver platter if they can access that endpoint. In that context, it is beneficial that securityChecks becomes an extra endpoint, because it can be easily sealed off with a firewall rule.

Otherwise, you can run the server with advisoryChecks:true to view checks in the logger.

• Form the previous comments I would understand that logOutput also has to be set true to see the output in the logs, correct?

I only just realised you edited this comment

Test case added.

Regarding your second point - should we perhaps disable the security checks by default? I'm concerned that configurations with an exposed masterKey will potentially become more exposed (or, is this the least of their problems if their masterKey is exposed).

[mtrezza]

should we perhaps disable the security checks by default?

Yes, as discussed

I'm concerned that configurations with an exposed masterKey will potentially become more exposed (or, is this the least of their problems if their masterKey is exposed).

I don't think Parse Server can become more exposed than with the master key, aside from a (undiscovered) security vulnerability that exposes Parse Server internals or the underlying infrastructure. Do you have an example?

[dblythy]

Yes, as discussed

Done!

I don't think Parse Server can become more exposed than with the master key, aside from a (undiscovered) security vulnerability that exposes Parse Server internals or the underlying infrastructure. Do you have an example?

No, I don't think so either.

[mtrezza]

Related aspects to test for security: https://github.com/netreconlab/parse-hipaa

[mtrezza]

@dblythy I may just pick up this PR and finish it if there's not too much left, if that's ok with you. Let me know if you have any thoughts to incorporate, or any obstacles you think should be addressed, that are not already mentioned here.

[dblythy]

@mtrezza you're welcome to crack in. I've been a bit busy on my end so I haven't been able to finish it off.

I was looking at evolving it so that new features can be tested with something like your example.

const check = new SecurityCheck({

});

[dblythy]

@mtrezza did you manage to pick this up? If not, all good. I'm looking to keep working on it this week.

[dblythy]

As discussed @mtrezza I have introduced a helper class 'SecurityCheck'.

The idea is that adding a new security check can happen anywhere, without needing to import any files.

Here's how to use it:

new Parse.SecurityCheck({
    group: Parse.SecurityCheck.Category.ServerConfiguration,
    title: 'Client class creation allowed',
    warning: 'Clients are currently allowed to create new classes. This allows an attacker to create new classes without restriction and potentially flood the database. Change the Parse Server configuration to allowClientClassCreation: false.',
    success: `Client class creation is turned off`,
    check() {
      return !options.allowClientClassCreation;
    },
});

Returning false or throwing in check means that the check failed.

Alternatively:

const check = new Parse.SecurityCheck({
    group: Parse.SecurityCheck.Category.ServerConfiguration,
    title: 'Client class creation allowed',
    warning: 'Clients are currently allowed to create new classes. This allows an attacker to create new classes without restriction and potentially flood the database. Change the Parse Server configuration to allowClientClassCreation: false.',
    success: `Client class creation is turned off`
});
if (condition) {
  check.setFailed();
}

Also sets the check to failed.

Let me know your thoughts on the way I've coded it thus far.

Still need to work on DB error stuff, and increase coverage.

[mtrezza]

Well, done! I think you got all the components there, I only suggest to change the structure, rearrange the parts if you will.

The idea is that adding a new security check can happen anywhere, without needing to import any files.

I see that such checks have been added to the MongoDB adapter. I would opine against that because:

• it goes against the modular structure of Parse Server
• these are checks, not blocking assertions, so there is no need for them to be implemented in synchronous structure
• we want to call the status of the security checks at arbitrary points in time, this also ensures compatibility if an option in Parse Server becomes dynamic instead of statically set once during server initialization (think of the current effort to dynamically re-load Cloud Code on file change)

So ideally, we want to keep the security checks centralized in a single file.

Other thoughts:

• Forward thinking I would split the warning message from the solution / recommendation message, i.e. how to solve the warning. We can always concat if we need to, but we can't split if we need to, e.g. to display it nicely in Parse Server Dashboard.
• Why do we need a SecurityCheckStore and hold these checks in memory? Besides, making the class SecurityCheck an entry point for the store violates some OOP principles.

To simplify the structure, I suggest to create a directory /src/SecurityCheck with something like:

• SecurityCheck.js --> the Security check class (remove the store unless there is a need for it)
• SecurityCheckDefinitions.js --> the definition of all security checks, this is where developers add a check for their new PR
• SecurityCheckRunner.js --> the script that loads the definitions and executes them

[dblythy]

Well, done! I think you got all the components there, I only suggest to change the structure, rearrange the parts if you will.

Thank you for the feedback. Most of your questions can be answered by the fact that I'm pretty new to Javascript class syntax / OOP and not overly sure what i'm doing 🤨 .

Other thoughts:

Thanks for your suggestions. The reason why I had them stored was thinking that for the dashboard; where the security checks will be visible after the cloud route is called.

Do you have any suggestions as to how I can do this without storing them?

[mtrezza]

I imagine the dashboard having a page that executes SecurityChecks on demand and displays the results. Then there is an update button that executes the checks again and displays the updated results.

The checks are executed in REST fashion.

There is no need to cache them in Parse Server. The only purpose I can think of to store them would be a historic log, but that also would not happen in memory but in the database. I don't think such a historic view should be in scope here.

In memory storing in Parse Server is also useless when you think of an environment with multiple instances, where only one instance has these in memory.

[dblythy]

I don't really understand the difference between what I have now, and your example, e.g:

checks.push(check);
for ... { await check.run(); }

The securityCheckStore adds the securityChecks to an array, where they are then run using check.run(), on the REST call or start of server.

How else should I do this? Isn't this the same as securityChecks.js push to an array, and then on the REST call run the checks?

[mtrezza]

This should never be undefined, instead set a default value in option definitions.

[mtrezza]

How else should I do this? Isn't this the same as securityChecks.js push to an array, and then on the REST call run the checks?

Your securityChecks.js is currently permanently referenced. It keeps all the checks and their check status in memory, which is unnecessary. The security checks are stateless and run on demand.

[dblythy]

So it would be better to have:

const checks = [];
const masterKeyCheck = new SecurityCheck(...
checks.push(masterKeyCheck);

function runChecks() {
  checks.run(); // something like this
}

[mtrezza]

Same here

[mtrezza]

All this logic should go into the security checks files, not into the server file.

[mtrezza]

I finally found some time to look into this branch in detail:

• Security Checks should be a module rather than scattered into adapters and controllers.
• These files are supposed to be touched many times by developers who are not familiar with this PR and should be able to add their security checks with minimal effort, easily and intuitively, without reading another documentation or this PR. The code structure here is more important than usually. Looking at the current code, I took me a while to figure out where and how I'd have to add a specific test. It may make sense to extract sub-modules into separate files.
• There is some redundant code that makes this hard to maintain, especially in the tests.
• The tests need refactoring and be more strict in the outcome they test, e.g. error code rather than just any error.

I think this needs a major rewrite. Coordinating the necessary changes takes probably more time than just writing it from scratch. I hope to find the time soon to write this from scratch in a new branch. I'll post an update here when the basic structure is done and then maybe we can discuss together which tests to add.

[dblythy]

Ok, I think that’s the best way to go, if you have the willingness and capacity to implement this feature - rather than going back and forth attempting to reach your vision. I look forward to seeing your version. Thanks

[mtrezza]

Thanks @dblythy, you did a good job already in bringing this ahead. If it's ok with you and if it turns out to be faster, I may just do the changes directly in your branch, so maybe keep it for a while.

[dblythy]

Thanks Manuel, it was fun to try to learn OOP and I look forward to seeing your implementation, so I can learn how it should look like.

You’re welcome to use any code / branch of mine for the benefit of the community, please don’t hesitate if it saves you time :)

[MobileVet]

@dblythy I just wanted to give you a 👍 for your work and graciousness on this. It is a great concept and will definitely be a solid contribution. I am really pleased that you clearly haven't taken offense, but instead recognized that this will be a great learning opportunity. In the end, the community will have a useful new feature and you will have seen an alternative approach that is beneficial for a project of this size and scope.

[dblythy]

@MobileVet thanks for the lovely thoughts - it’s no big deal though. At the end of the day we all want Parse Server to evolve, and whether that’s my code in the final version or not, it ultimately doesn’t really matter. :)

[mtrezza]

It was clearly just meant to be a pragmatic suggestion. @dblythy made a great contribution with this PR and has put a lot of effort into this. Without his initiative we would probably still be talking about the idea. This feature has been a community effort all along, from the first idea by $davimacedo to the initiative to create this PR by $dblythy and all the feedback from others that went into it. I will pick this up for refactoring now and we can go from there to decide which tests to add.