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

Security Review #8

Merged
merged 16 commits into from Sep 15, 2015

Conversation

Projects
None yet
3 participants
@yg-ht
Contributor

yg-ht commented Nov 27, 2014

Hi Jeremy,

Like your work on this. I started using it a little while ago!

Being a professional penetration tester I thought I would give this the once over. These are my changes. Most notably there was XSS in the form. Other than that I have just included hardening features and instructions/healpers for people if they want to enable SSL.

Cheers!

Felix

Felix added some commits Nov 27, 2014

Felix
Update index.php
Added the ability to use HTTPS and set the HSTS (Stict Transport Security) headers.
Fixed the reflective XSS attack vector by removing all non-integer chars from the variable.
Felix
Update config_sample.php
Added the "$USE_HTTPS" config var, increased the number of pings as I have a slow to boot up machine, removed the 7th octet from the MAC address array.
Felix
Create .htaccess
Setting security / hardening values
Felix
Create apacheconfig
New default apache config to enable SSL by default
Felix
Create apachesslconfig
Standard Apache2 ssl.conf file as of 20141127 minus all the comments and with two changes:

SSLCipherSuite HIGH:!MEDIUM:!LOW:!aNULL:!MD5
To specifically forbid anything other than high level encryption

and

SSLProtocol all -SSLv2 -SSLv3
To disable SSLv3 since the poodle bug
Felix
Update README.md
Added the "how-to" guide here with the extra bits required for secure running.
Felix
Update README.md
Changed the git URL to this repo until its pulled to the master

@sciguy14 sciguy14 self-assigned this Jan 27, 2015

@sciguy14

This comment has been minimized.

Owner

sciguy14 commented Jan 27, 2015

Hey Felix - this looks great. Thanks so much! Sorry I'm just getting around to reviewing it now. I just have a minor request. Some people might not want to use SSL. Though your updates make it optional, it doesn't clearly delineate what steps to do or not do when choosing to use SSL. It would be great if you could do two things:

  1. In your updated readme instructions, clearly delineate which steps are optional, and only required for enabling SSL. You should also probably note and warn people that self signed certs will generate a "scary" message about the cert in all modern browsers.
  2. Change the default config_sample.php file to not enable SSL by default.

Sound good?

@yg-ht

This comment has been minimized.

Contributor

yg-ht commented Jan 28, 2015

Hi Jeremy,

Sounds sensible enough to me. I have an idea that might make it a little easier for users too but I need to refresh myself on the code first.

Will get back to you!

Felix

On 27 January 2015 17:32:32 GMT+00:00, Jeremy Blum notifications@github.com wrote:

Hey Felix - this looks great. Thanks so much! Sorry I'm just getting
around to reviewing it now. I just have a minor request. Some people
might not want to use SSL. Though your updates make it optional, it
doesn't clearly delineate what steps to do or not do when choosing to
use SSL. It would be great if you could do two things:

  1. In your updated readme instructions, clearly delineate which steps
    are optional, and only required for enabling SSL. You should also
    probably note and warn people that self signed certs will generate a
    "scary" message about the cert in all modern browsers.
  2. Change the default config_sample.php file to not enable SSL by
    default.

Sound good?


Reply to this email directly or view it on GitHub:
#8 (comment)

Sent from my Android device with K-9 Mail. Please excuse my brevity.

Felix added some commits Feb 6, 2015

Felix
Update config_sample.php
Changed the default configuration to not enforce SSL
Felix
Update apacheconfig
Allowed Apache to run a non-TLS port by default.
Felix
Update README.md
Hints for skipping SSL/TLS stuff
Felix
Update README.md
Typo fix.
@yg-ht

This comment has been minimized.

Contributor

yg-ht commented Feb 6, 2015

Hi Jeremy,

I have made some changes. I was thinking about writing a little install
script but the reality is I won't have the time to do this for a little
while. So - to get the improvements out to anyone who wants it now, I
figure making those changes you discussed would be a good idea in the
short term.

Felix

On 27/01/15 17:32, Jeremy Blum wrote:

Hey Felix - this looks great. Thanks so much! Sorry I'm just getting
around to reviewing it now. I just have a minor request. Some people
might not want to use SSL. Though your updates make it optional, it
doesn't clearly delineate what steps to do or not do when choosing to
use SSL. It would be great if you could do two things:

  1. In your updated readme instructions, clearly delineate which steps
    are optional, and only required for enabling SSL. You should also
    probably note and warn people that self signed certs will generate a
    "scary" message about the cert in all modern browsers.
  2. Change the default config_sample.php file to not enable SSL by default.

Sound good?


Reply to this email directly or view it on GitHub
#8 (comment).

@mew1033

This comment has been minimized.

mew1033 commented Jul 22, 2015

I'm interested in seeing this PR merged in. @sciguy14, is there anything else you'd like to see changed before this request can be merged?

@sciguy14

This comment has been minimized.

Owner

sciguy14 commented Jul 22, 2015

Ooops it completely slipped my mind. I'm moving apartments on Saturday, and will need to set this back up in my new place, so that will be the perfect opportunity for me to test it. I'll test it then, and merge it in if everything looks good. I'm putting it on my calendar so I don't forget! Thanks for the reminder ;)

@mew1033

This comment has been minimized.

mew1033 commented Aug 26, 2015

@sciguy14 Ping. :-)
Hope the move went well!

@sciguy14

This comment has been minimized.

Owner

sciguy14 commented Sep 7, 2015

Hey sorry - finally testing it.

First, some changes needed to the readme:

  1. In the readme, please put a line about what you should fill in when doing the key request. For example, people might think they need to enter a passphrase.
  2. Copying ssl.conf directly into mods-enabled doesn't work. That line needs to change to say that you copy it into mods-available. Then run the a2enmod command.
  3. You need to add another line noting that you need to restart apache: sudo service apache2 restart after enabling the headers module
  4. When you mention port forwarding, note that port 443 will need to be forwarded if https is enforced. Otherwise, just 80 can be forwarded.

Functionally...
When I have https enabled in the config file, If I navigate to the HTTP URL with a machine name not in the URL, then it redirects to an HTTPS version of the page. However, if I include the machine name in the URL of a HTTP request by appending ?computer=0 the page remains as HTTP instead of HTTPS. that behavior should be made consistent. The else statement on line 29 needs to be updated to match the functionality of the if statement above it.

Furthermore, when I do disable https in the config file, it still redirects to an HTTPS version of the page. This is because you incorrectly have the $USE_HTTPS variable in the config file defined as a string, when it should be a boolean (just remove the quotes). In PHP, a string that is not empty is defined as true, so it was always being set as true, since you had it in quotes.

Felix added some commits Sep 14, 2015

Felix
Update to cast a variable properly
Removed two quotes to make sure PHP doesn't think it is a string.
Felix
Bug fix
Bug existed whereby it was possible to still request an HTTP page even though HTTPS was required.  Essentially had to add lines #23 to #28 whilst the inline IF statement on #33 must remain to ensure that the correct protocol is used when mapping it later on.
@yg-ht

This comment has been minimized.

Contributor

yg-ht commented Sep 14, 2015

I think these are now done...

On 07/09/15 19:56, Jeremy Blum wrote:

Hey sorry - finally testing it.

First, some changes needed to the readme:

  1. In the readme, please put a line about what you should fill in when
    doing the key request. For example, people might think they need to
    enter a passphrase.
  2. Copying |ssl.conf| directly into |mods-enabled| doesn't work. That
    line needs to change to say that you copy it into |mods-available|.
    Then run the |a2enmod| command.
  3. You need to add another line noting that you need to restart
    apache: |sudo service apache2 restart| after enabling the headers module
  4. When you mention port forwarding, note that port 443 will need to
    be forwarded if https is enforced. Otherwise, just 80 can be forwarded.

Functionally...
When I have https enabled in the config file, If I navigate to the
HTTP URL with a machine name not in the URL, then it redirects to an
HTTPS version of the page. However, if I include the machine name in
the URL of a HTTP request by appending |?computer=0| the page remains
as HTTP instead of HTTPS. that behavior should be made consistent. The
else statement on line 29 needs to be updated to match the
functionality of the if statement above it.

Furthermore, when I do disable https in the config file, it still
redirects to an HTTPS version of the page. This is because you
incorrectly have the $USE_HTTPS variable in the config file defined as
a string, when it should be a boolean (just remove the quotes). In
PHP, a string that is not empty is defined as true, so it was always
being set as true, since you had it in quotes.


Reply to this email directly or view it on GitHub
#8 (comment).

Thanks

Felix


To verify my identity please visit:

https://keybase.io/felixrr

@sciguy14 sciguy14 merged commit 80d63f3 into sciguy14:master Sep 15, 2015

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