Last modified: 2014-10-17 18:41:07 UTC
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/.
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.
