-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Add Security Checks Log #6973
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
Add Security Checks Log #6973
Changes from 6 commits
b7257cb
83aaf62
f70e002
060b922
d972e8c
61412fb
152a02b
47650b4
3a002fb
d332f05
c68ac63
52b3b76
b06fe25
1ae8fbc
77364f4
60dc661
aa935b4
373fcdf
379e84a
2188f70
fff6b7e
b4faede
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| 'use strict'; | ||
| const Parse = require('parse/node'); | ||
| const request = require('../lib/request'); | ||
|
|
||
| const masterKeyHeaders = { | ||
| 'X-Parse-Application-Id': 'test', | ||
| 'X-Parse-Rest-API-Key': 'rest', | ||
| 'X-Parse-Master-Key': 'test', | ||
| 'Content-Type': 'application/json', | ||
| }; | ||
| const masterKeyOptions = { | ||
| headers: masterKeyHeaders, | ||
| json: true, | ||
| }; | ||
|
|
||
| describe('SecurityChecks', () => { | ||
| it('can get security advice', async done => { | ||
| await reconfigureServer({ | ||
| securityChecks: { | ||
| enabled: true, | ||
| logOutput: true, | ||
| }, | ||
| }); | ||
| const options = Object.assign({}, masterKeyOptions, { | ||
| method: 'GET', | ||
| url: Parse.serverURL + '/securityChecks', | ||
| }); | ||
| request(options).then(res => { | ||
| expect(res.data.Security).not.toBeUndefined(); | ||
| expect(res.data.CLP).not.toBeUndefined(); | ||
| expect(res.data.Total).not.toBeUndefined(); | ||
| done(); | ||
| }); | ||
| }); | ||
|
|
||
| it('can get security on start', async done => { | ||
| await reconfigureServer({ | ||
| securityChecks: { | ||
| enabled: true, | ||
| logOutput: true, | ||
| }, | ||
| }); | ||
| const logger = require('../lib/logger').logger; | ||
| spyOn(logger, 'warn').and.callFake(() => {}); | ||
| await new Promise(resolve => { | ||
| setTimeout(() => { | ||
| resolve(); | ||
| }, 2000); | ||
| }); | ||
| expect(logger.warn.calls.mostRecent().args[0]).toContain( | ||
| 'Allow Client Class Creation is not recommended.' | ||
| ); | ||
| done(); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,6 +40,7 @@ import { AggregateRouter } from './Routers/AggregateRouter'; | |
| import { ParseServerRESTController } from './ParseServerRESTController'; | ||
| import * as controllers from './Controllers'; | ||
| import { ParseGraphQLServer } from './GraphQL/ParseGraphQLServer'; | ||
| import { securityChecks } from './SecurityChecks.js'; | ||
|
|
||
| // Mutate the Parse object to add the Cloud Code handlers | ||
| addParseCloud(); | ||
|
|
@@ -80,6 +81,9 @@ class ParseServer { | |
| if (serverStartComplete) { | ||
| serverStartComplete(); | ||
| } | ||
| if (options.securityChecks.logOutput) { | ||
| this.getSecurityChecks(); | ||
| } | ||
|
Comment on lines
+84
to
+97
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Run security check after serverStartComplete promise resolved (if serverStartComplete return a promise) to avoid false/positive, since developers can inject some initialization script into serverStartComplete 😉 |
||
| }) | ||
| .catch(error => { | ||
| if (serverStartComplete) { | ||
|
|
@@ -109,6 +113,34 @@ class ParseServer { | |
| return this._app; | ||
| } | ||
|
|
||
| async getSecurityChecks() { | ||
| const config = Config.get(this.config.appId); | ||
| const response = await securityChecks(config); | ||
| const logger = logging.getLogger(); | ||
| const warnings = response.response; | ||
| const security = warnings.Security || []; | ||
| const clp = warnings.CLP || []; | ||
| const total = warnings.Total; | ||
| if (total == 0) { | ||
| return; | ||
| } | ||
| let errorString = `We found ${total} improvement${ | ||
|
dblythy marked this conversation as resolved.
Outdated
|
||
| total == 1 ? '' : 's' | ||
| } for you to make on your Parse Server:\n\n`; | ||
| for (const issue of security) { | ||
| errorString += ` -${issue.title}\n`; | ||
| errorString += ` ${issue.message}\n\n`; | ||
| } | ||
| for (const issue in clp) { | ||
| errorString += `\n Add CLP for Class: ${issue}\n`; | ||
| const classData = clp[issue]; | ||
| for (const clpIssue of classData) { | ||
| errorString += ` ${clpIssue.title}\n`; | ||
| } | ||
| } | ||
| logger.warn(errorString); | ||
| } | ||
|
|
||
| handleShutdown() { | ||
| const promises = []; | ||
| const { adapter: databaseAdapter } = this.config.databaseController; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import PromiseRouter from '../PromiseRouter'; | ||
| import Parse from 'parse/node'; | ||
| import rest from '../rest'; | ||
| import { securityChecks } from '../SecurityChecks.js'; | ||
| const triggers = require('../triggers'); | ||
| const middleware = require('../middlewares'); | ||
|
|
||
|
|
@@ -53,6 +54,7 @@ export class CloudCodeRouter extends PromiseRouter { | |
| middleware.promiseEnforceMasterKeyAccess, | ||
| CloudCodeRouter.deleteJob | ||
| ); | ||
| this.route('GET', '/securityChecks', middleware.promiseEnforceMasterKeyAccess, securityChecks); | ||
| } | ||
|
Comment on lines
56
to
63
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think about removing the endpoint also if the options is not enabled ?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What would be the argument for removing the endpoint?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think we should avoid exposing endpoints (even if it's protected by master key), if the feature is disabled ! Here, it is even more important because it provides advanced security feedback from the server.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not related but i don't know if notifications works correctly: Also @mtrezza could you check your parse community inbox ? I would like to open a security advisory. But it seems that i do not have rights on the repo explain the issue.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I expect that we will enable this feature by default in the future (as discussed previously in the related issue), and that most deployments will have this enabled. A production system should really prevent unauthorized access using firewall rules, we can add that to the docs as recommendation. The downside I see is that removing an endpoint means returning 404 but the correct response is really 403, either by Parse Server, or by a firewall. If someone decides that they want to obfuscate the 403 to a 404 based on their security strategy, that can still be done by the developer using a firewall, but I think that strategy should not be mandated by Parse Server.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right the endpoint should throw 403 if option not enabled ! |
||
|
|
||
| static getJobs(req) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,167 @@ | ||
| import { getTrigger } from './triggers.js'; | ||
| import url from 'url'; | ||
| import Parse from 'parse/node'; | ||
| export async function securityChecks(req) { | ||
|
dblythy marked this conversation as resolved.
Outdated
|
||
| try { | ||
| const options = req.config || req; | ||
| if (!options.securityChecks.enabled && options.securityChecks.enabled != null) { | ||
| return { error: { code: 1, error: 'Security checks are not enabled.' } }; | ||
| } | ||
| const clpWarnings = {}; | ||
| const securityWarnings = []; | ||
| let totalWarnings = 0; | ||
| if (options.allowClientClassCreation) { | ||
| securityWarnings.push({ | ||
| title: 'Allow Client Class Creation is not recommended.', | ||
| message: | ||
| 'Allow client class creation is not recommended for production servers it allows any user - authorized or not - to create a new class.', | ||
| link: 'https://docs.parseplatform.org/js/guide/#restricting-class-creation', | ||
| }); | ||
| } | ||
| const schema = await options.database.loadSchema(); | ||
| const all = await schema.getAllClasses(); | ||
| for (const field of all) { | ||
| const className = field.className; | ||
| const clp = field.classLevelPermissions; | ||
| const thisClassWarnings = clpWarnings[className] || []; | ||
| if (!clp) { | ||
| totalWarnings++; | ||
| thisClassWarnings.push({ | ||
| title: `No Class Level Permissions on ${className}`, | ||
| message: | ||
| 'Class level permissions are a security feature from that allows one to restrict access on a broader way than the ACL based permissions. We recommend implementing CLPs on all database classes.', | ||
| link: 'https://docs.parseplatform.org/parse-server/guide/#class-level-permissions', | ||
| }); | ||
| clpWarnings[className] = thisClassWarnings; | ||
| continue; | ||
| } | ||
| const keys = ['find', 'count', 'get', 'create', 'update', 'delete', 'addField']; | ||
| for (const key of keys) { | ||
| const option = clp[key]; | ||
| if (className === '_User' && key === 'create') { | ||
| continue; | ||
| } | ||
| if (!option || option['*']) { | ||
| totalWarnings++; | ||
| thisClassWarnings.push({ | ||
| title: `Unrestricted access to ${key}.`, | ||
| message: `We recommend restricting ${key} on all classes`, | ||
| link: 'https://docs.parseplatform.org/parse-server/guide/#class-level-permissions', | ||
| }); | ||
| } else if (Object.keys(option).length != 0 && key === 'addField') { | ||
| totalWarnings++; | ||
| thisClassWarnings.push({ | ||
| title: `Certain users can add fields.`, | ||
| message: | ||
| 'Class level permissions are a security feature from that allows one to restrict access on a broader way than the ACL based permissions. We recommend implementing CLPs on all database classes.', | ||
| link: 'https://docs.parseplatform.org/parse-server/guide/#class-level-permissions', | ||
| }); | ||
| } | ||
| } | ||
| clpWarnings[className] = thisClassWarnings; | ||
| } | ||
| const fileTrigger = getTrigger('@File', 'beforeSaveFile', options.appId); | ||
| if (!fileTrigger) { | ||
| totalWarnings++; | ||
| securityWarnings.push({ | ||
| title: `No beforeFileSave Trigger`, | ||
| message: | ||
| "Even if you don't store files, we strongly recommend using a beforeFileSave trigger to prevent unauthorized uploads.", | ||
| link: 'https://docs.parseplatform.org/cloudcode/guide/#beforesavefile', | ||
| }); | ||
| } else { | ||
| try { | ||
| const file = new Parse.File('testpopeye.txt', [1, 2, 3], 'text/plain'); | ||
| await file.save(); | ||
| totalWarnings++; | ||
| securityWarnings.push({ | ||
| title: `Unrestricted access to file uploads`, | ||
| message: | ||
| 'Even though you have a beforeFileSave trigger, it allows unregistered users to upload.', | ||
| link: 'https://docs.parseplatform.org/cloudcode/guide/#beforesavefile', | ||
| }); | ||
| await options.filesController.deleteFile(file._name); | ||
| } catch (e) { | ||
| /* */ | ||
| } | ||
| } | ||
| let https = false; | ||
| try { | ||
| const serverURL = url.parse(options.serverURL); | ||
| https = serverURL.protocol === 'https:'; | ||
| } catch (e) { | ||
| /* */ | ||
| } | ||
| if (!https) { | ||
| totalWarnings++; | ||
| securityWarnings.push({ | ||
| title: `Server served over HTTP`, | ||
| message: 'We strongly recommend using a HTTPS protocol.', | ||
| }); | ||
| } | ||
| let databaseURI = options.databaseURI; | ||
| let protocol; | ||
| try { | ||
| const parsedURI = url.parse(databaseURI); | ||
| protocol = parsedURI.protocol ? parsedURI.protocol.toLowerCase() : null; | ||
| } catch (e) { | ||
| /* */ | ||
| } | ||
| if (protocol !== 'postgres:') { | ||
| if (databaseURI.includes('@')) { | ||
| databaseURI = `mongodb://${databaseURI.split('@')[1]}`; | ||
| const pwd = options.databaseURI.split('//')[1].split('@')[0].split(':')[1] || ''; | ||
| if (!pwd.match('^(?=.*[a-z])(?=.*[A-Z])(?=.*[0-9])(?=.*[!@#$%^&*])(?=.{14,})')) { | ||
| // DB string must contain at least 1 lowercase alphabetical character | ||
| // DB string must contain at least 1 uppercase alphabetical character | ||
| // DB string must contain at least 1 numeric character | ||
| // DB string must contain at least one special character | ||
| // DB string must be 14 characters or longer | ||
| securityWarnings.push({ | ||
| title: `Weak Database Password`, | ||
| message: 'The password used to connect to your database could be stronger.', | ||
| link: 'https://docs.mongodb.com/manual/security/', | ||
| }); | ||
| totalWarnings++; | ||
| } | ||
| } | ||
| let databaseAdmin = '' + databaseURI; | ||
| try { | ||
| const parsedURI = url.parse(databaseAdmin); | ||
| parsedURI.port = '27017'; | ||
| databaseAdmin = parsedURI.toString(); | ||
| } catch (e) { | ||
| /* */ | ||
| } | ||
| const mongodb = require('mongodb'); | ||
| const MongoClient = mongodb.MongoClient; | ||
| try { | ||
| await MongoClient.connect(databaseAdmin, { useNewUrlParser: true }); | ||
| securityWarnings.push({ | ||
| title: `Unrestricted access to port 27017`, | ||
| message: | ||
| 'It is possible to connect to the admin port of your mongoDb without authentication.', | ||
| link: 'https://docs.mongodb.com/manual/security/', | ||
| }); | ||
| totalWarnings++; | ||
| } catch (e) { | ||
| /* */ | ||
| } | ||
| try { | ||
| await MongoClient.connect(databaseURI, { useNewUrlParser: true }); | ||
| securityWarnings.push({ | ||
| title: `Unrestricted access to your database`, | ||
| message: | ||
| 'It is possible to connect to your mongoDb without username and password on your connection string.', | ||
| link: 'https://docs.mongodb.com/manual/security/', | ||
| }); | ||
| totalWarnings++; | ||
| } catch (e) { | ||
| /* */ | ||
| } | ||
| } | ||
| return { response: { Security: securityWarnings, CLP: clpWarnings, Total: totalWarnings } }; | ||
| } catch (error) { | ||
| return { error: { code: 1, error: error.message || 'Internal Server Error.' } }; | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.