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 Cmd Exec Vulns #204

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

Meatballs1
Copy link
Contributor

This prevents the abuse of command exec vulns in the Apache commands. These values should be relatively static (even across environments) so there isn't really a good reason to have them dynamically configurable.

Configuration of Apache commands/paths now takes place in config/initializers/apache.rb

  • GlobalSettings is now a Singleton and forced to be so in the database due to a uniqueness check.
  • An ApacheController library class has been added which takes away some of the control burden from Campaign. This could probably go further to simplify the Campaign logic.
  • Have moved to follow the Apache standard of writing to sites-available and creating a symlink when the site is activated.

end

add_index "baits", ["message_id"], name: "index_baits_on_message_id", using: :btree
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm not sure where this has crept in from.

@Meatballs1
Copy link
Contributor Author

Only because I have defined instance !

`#{first.command_apache_status} 2>&1`
def self.instance
begin
find(1)
Copy link
Member

Choose a reason for hiding this comment

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

I noticed within my testing that this operated better when changed to first vs find(1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this because it is lazy-populated? E.g. the first time you tried it it didn't exist. When you changed it the record had been created?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah in my case the first GlobalSettings object was with an id of 2. This is my dev env and probably got dirty somewhere.

…fix_cmd_exec

Conflicts:
	app/models/campaign.rb
	app/models/global_settings.rb
	app/views/admin/global_settings.html.erb
	config/application.rb
	db/schema.rb
@Meatballs1
Copy link
Contributor Author

n.b. I expect code exec is probable when previewing emails via erb?

@zeknox
Copy link
Member

zeknox commented Jul 20, 2015

Throwing a bunch of errors on the tests due to the unique validation. Perhaps we need to write a db cleaner between each test?

# rspec
.........................FFFFFFFFFFF......................F..FF..........FFF
Failures:
  1) Campaigns GET /campaigns/1 populate campaign options and ensure all settings saved to campaign
     Failure/Error: click_on("Save Settings")
     ActionView::Template::Error:
       Mysql2::Error: Duplicate entry '0' for key 'index_global_settings_on_singleton': INSERT INTO `global_settings` (`created_at`, `singleton`, `updated_at`) VALUES ('2015-07-20 22:51:02', 0, '2015-07-20 22:51:02')

@Meatballs1
Copy link
Contributor Author

Not sure if the test is causing the object to be created, or if its just rspec doing something magic in the background to instantiate it?

@Meatballs1
Copy link
Contributor Author

It could be a multithreading thing from rspec? Hmm. Maybe should try to access GlobalSettings.instance in a :before or some kind of rspec initializor. Will need to check up where best to do it :)

@zeknox
Copy link
Member

zeknox commented Jul 22, 2015

I feel like the tests are not clearing the database upon each request. I believe it should purge the TEST db instance between each spec to alleviate this isssue. Just my hunch.

Conflicts:
	app/models/campaign.rb
	app/models/global_settings.rb
	app/views/admin/global_settings.html.erb
	config/application.rb
	db/schema.rb
I think because I only have a single admin user, when we destroy the
first admin it causes devise to auto logout. Therefore we destroy the
last admin!
Also force GlobalSettings ID to be 1. This prevents rspecs which seem to
choose random ID numbers from throwing errors.
Conflicts:
	app/models/global_settings.rb
	config/application.rb
	db/schema.rb
@technion
Copy link
Contributor

Just as note here that for a user deploying on CentOS - none of these defaults are valid. It's perfectly fine of course to edit a config file and update them, but I wouldn't say they are static "across environments".

@Meatballs1
Copy link
Contributor Author

Yes correct, maybe static across many environments :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants