Page MenuHomePhabricator

Security review of IEG grant review
Closed, ResolvedPublic

Description

Security review needed before initial production deployment.

Code is at https://git.wikimedia.org/summary/wikimedia%2Fiegreview and a test server is available at http://iegreview.wmflabs.org/.


Version: wmf-deployment
Severity: normal

Details

Reference
bz71624

Event Timeline

bzimport raised the priority of this task from to High.Nov 22 2014, 3:43 AM
bzimport set Reference to bz71624.

The app is based on code base from the Wikimania Scholarships app. Hopefully knowing that will make it easier for Chris to focus his review.

Some general comments so far:

  • The inclusion of <script> tags in the markdown seems really problematic, and I think needs a better design
  • It would help if we had a strict content security policy, so that <script> tags wouldn't be rendered. From an initial look, I think you could get away with just setting "Content-Security-Policy: default-src 'self'; img-src 'self' data:" and everything would still work.
  • You should also set an "X-Frame-Options: DENY" header
  • Cookies should be set with "httponly" flag. If someone finds an xss, that will keep the session cookie from being (easily) stolen.
  • You should set the charset to UTF-8, so browsers don't have to guess (and attacks in utf7 won't work). So set a "Content-Type:text/html; charset=UTF-8" header.
  • Password.php - don't fall back to the "high entropy seed", just throw an exception if the other methods aren't available. It's ok for salts, but for generating random passwords, it's probably bruteforceable.

Due to the number of libraries this pulls in, I have a long way to go in the review, but the markdown is going to take some time.

Change 165431 had a related patch set uploaded by BryanDavis:
Remove markdown support

https://gerrit.wikimedia.org/r/165431

Change 165432 had a related patch set uploaded by BryanDavis:
Support formatting messages using Parsoid

https://gerrit.wikimedia.org/r/165432

(In reply to Chris Steipp from comment #2)

Some general comments so far:

  • The inclusion of <script> tags in the markdown seems really problematic,

and I think needs a better design

  • It would help if we had a strict content security policy, so that <script>

tags wouldn't be rendered. From an initial look, I think you could get away
with just setting "Content-Security-Policy: default-src 'self'; img-src
'self' data:" and everything would still work.

Easy add. I'll post a patch.

  • You should also set an "X-Frame-Options: DENY" header

Ok.

  • Cookies should be set with "httponly" flag. If someone finds an xss, that

will keep the session cookie from being (easily) stolen.

Another easy one.

  • You should set the charset to UTF-8, so browsers don't have to guess (and

attacks in utf7 won't work). So set a "Content-Type:text/html;
charset=UTF-8" header.

Ok. I am setting <meta charset="utf-8"/> in my base template, but that isn't seen until the user agent processes the content.

  • Password.php - don't fall back to the "high entropy seed", just throw an

exception if the other methods aren't available. It's ok for salts, but for
generating random passwords, it's probably bruteforceable.

Patch incoming.

Due to the number of libraries this pulls in, I have a long way to go in the
review, but the markdown is going to take some time.

I've posted patches to remove markdown support and instead use Parsoid to provide rich markup support. I will cherry-pick these to the testing server soon.

Change 165692 had a related patch set uploaded by BryanDavis:
Add headers to make attacking the site harder

https://gerrit.wikimedia.org/r/165692

Change 165693 had a related patch set uploaded by BryanDavis:
Do not use weak random for password hashing

https://gerrit.wikimedia.org/r/165693

(In reply to Bryan Davis from comment #5)

Ok. I am setting <meta charset="utf-8"/> in my base template, but that isn't
seen until the user agent processes the content.

I missed that. That should be good enough.

Change 165431 merged by jenkins-bot:
Remove markdown support

https://gerrit.wikimedia.org/r/165431

Change 165432 merged by jenkins-bot:
Support formatting messages using Parsoid

https://gerrit.wikimedia.org/r/165432

Change 165692 merged by jenkins-bot:
Add headers to make attacking the site harder

https://gerrit.wikimedia.org/r/165692

Change 165693 merged by jenkins-bot:
Do not use weak random for password hashing

https://gerrit.wikimedia.org/r/165693

Patches are all merged and the testing server has been updated.

I don't feel like I've been able to give this as thorough a review as I would like (primarily the included libraries), but:

  • the app is running on a separate server
  • most parts of the app are only accessible to reasonably trusted users
  • it's using mostly libraries that are already in use on the cluster
  • dynamic scanning has an acceptable level of issues reported

As long as we get bug 72193 finished soon (which is a bigger problem), then I think we're ok deploying this now.