Talk:Security for developers/Architecture

Failure to ensure that new or upgraded extensions function properly with other core extensions

It should be required that all upgraded or new extensions that permit the addition of content visible to the public operate with the revision deletion/suppression module, and that any actions related to content creation be included in the logs reported to checkusers - before installation on any non-testing project. This should be a mandatory criterion before installing, even as a test example, on any "real" project; failure to do this has resulted in extremely inappropriate content addition or difficulty for checkusers to identify and block vandals. AFT5 did not have this ability designed in, and required significant re-engineering to fix the problem; after that, a promise was made not to release future extensions on production wikis, even as tests, until the ability to revision delete/suppress and checkuser was demonstrated. Then Flow was released without the ability to checkuser contributions, or to revision delete/suppress. (Incidentally, the reverse is also true - any actions taken to revision delete/suppress any form of content addition needs to show up in the deletion and/or suppression logs.)

I am certain there are other core extensions with which anything new needs to be able to interact appropriately; these are the two I'm most familiar with, so I'm giving this example. Risker (talk) 01:22, 8 May 2014 (UTC)

I have copied and pasted this message from the talk page of the performance guidelines. Thank you, Risker! Sharihareswara (WMF) (talk) 15:00, 9 May 2014 (UTC)

A few thoughts/ideas

Chris, thank you so much for working on this. I look forward to working through the implementation/testing/continuing responsibilities sections when they are more fleshed out.

Some questions, thoughts, and ideas:

"What are we trying to protect?" Are there subcategories here? Also, what about:

• the fact that a particular IP address or username visited/read a page? (the specific way we keep this private is by not logging it?)
• entire wikis? e.g., officewiki, Board wiki
• integrity of the user experience? (like, around CSRF stuff or the insertion of ads?)

(I would not be surprised if none of those are applicable, but I want to bring them up.) Sharihareswara (WMF) (talk) 02:51, 16 May 2014 (UTC)

"assess your feature's security" - can we give an example or 2? Sharihareswara (WMF) (talk) 02:51, 16 May 2014 (UTC)

In the examples for "Secure design principles" it's fine to include examples *from the past* and say "we used to do this wrong and now we have fixed it." If you're having trouble finding examples of a few of those principles, that's one source. Sharihareswara (WMF) (talk) 02:51, 16 May 2014 (UTC)

In the "implementation" section I think it's fine to have "redemption narratives" :-) in which a particular bit of code used to do the wrong thing but now we have fixed it to do things properly. One source of examples: you could go through some security fixes from the last 3-4 years. Sharihareswara (WMF) (talk) 02:51, 16 May 2014 (UTC)

Idea re unit tests

@Khorn (WMF): suggested an additional rule (perhaps this should go into the implementation guide and not the overall guidelines):

Don't ever contact third parties in automated testing. When you are writing automated tests, including unit tests, make super sure that your tests are not contacting the "outside world" (other sites, Tool Labs tools, financial services, non-Wikimedia APIs, and so on). Sharihareswara (WMF) (talk) 17:05, 18 June 2014 (UTC)

<K4-713> sumanah: Yeah... so this is one of those things where it might be good to imply that all third party communication should use one function that's easily overrideable.

<K4-713> People run into issues if they don't do that.

<K4-713> sumanah: I guess what I'm getting at is "wrap anything that you will need to override for testing". In this case, cURL functionality. Does that make sense?

<K4-713> Or even... "wrap anything that will make testing tricky."

<K4-713> ...then you can just not go there.

(#wikimedia-fundraising, just now)

Sharihareswara (WMF) (talk) 17:30, 18 June 2014 (UTC)

Good example of simplicity

I asked for one last week, and we still need a positive example of where we've created a simple design, implementation, or interface whose simplicity guards against future errors or attacks.

HTMLForm: Suggestion: "HTMLForm, while incredibly complex, has a relatively simple interface for security, i.e., built-in CSRF tokens and validation." DonationInterface: @Khorn (WMF): suggested an example from Fundraising. The DonationInterface extension "provides fundraising mechanisms for collecting and tracking payments through various payment gateways." Thus the team wrote unit tests that ensure DonationInterface correctly reads from and writes to those gateways. Instead of writing dozens of individual mocks that might fail and thus incur charges, add noise to logs, cause inaccurate financial reports, or worse, the team:

• subclassed each unit test from a class that managed all interaction with gateways
• for the tests, overrode the "actually talking to the outside world" functionality and silenced it

Thus it's simpler to add a new gateway and write a new test, and we're less likely to accidentally break security rules with a bad test. Sharihareswara (WMF) (talk) 17:35, 18 June 2014 (UTC)

Corrections from Horn:

For clarification, we do mock gateway communication. It's not really an "instead". We just started by damaging the default, for increased safety. (...where in this case, the default was actually talking to other systems.)

So, as it turns out, the concept is much simpler than the actual code. :/ ... The problem is that php doesn't do multiple inheritance....

...that one file, is sort of ridiculous. Let me see if I can explain: Every 3rd party gets its own "gateway" class on our end, in the actual code. These classes are all descended from a common parent class. For the tests, we have to do one further descendant for each gateway class... ...and each one overrides the same parent communication function. So, it looks like a totally insane amount of code duplication. Because you can't inherit from multiple classes.

But, given the choice between ugly production code and ugly tests, I decided to shovel all the insanity into the test code. Until we think of something better. The relevant file that creates all these descendants for testing, is DonationInterface/tests/includes/test_gateway/test.adapter.php

Maybe you could reclassify this as an example of desperation in code.

Sharihareswara (WMF) (talk) 18:27, 18 June 2014 (UTC)

next steps

• Sumana - polish prose
• Sumana - make examples better (ask Steipp for code when needed)
• Chris - make a data flow diagram

(all by Wed of next week)

Next week- talk on June 25 :) Sharihareswara (WMF) (talk) 17:29, 20 June 2014 (UTC)

@CSteipp: could I have links to the code for MediaWiki's html generation classes and the Simplicity bad example? Also, should we mention that capturing IP addresses for non-registered contributors is one of our security principles? and: Security non-functional requirements - what does that mean? Can we use clearer language? Sharihareswara (WMF) (talk) 02:54, 9 July 2014 (UTC)

Html generation: https://git.wikimedia.org/blob/mediawiki%2Fcore/HEAD/includes%2FHtml.php

User session management: core, and any extensions that implement UserLoadFromSession, like CentralAuth

IPs of non-registered users: I don't think it's really a principle. We allow ourselves to do it because it would be too hard to change, but it's something a lot of people would like to see go away.

Non-functional requirements: w:Non-functional_requirement is probably a good starting description, although the explanation and suggestions in http://www.methodsandtools.com/archive/archive.php?id=113 are also really good. Basically, teams should find a way to plan for security across their features and track any requirements for it to satisfy our security goals. CSteipp (talk) 23:47, 10 July 2014 (UTC)

Integration

Security for developers is where this information seems to belong to (as opposed to the subpages and checklist, which go into details). When this document is ready, it should be merged to the main one. --Nemo 15:02, 31 July 2014 (UTC)

Third party libraries

Including third party libraries is an important part of development. Implicit in Requests_for_comment/Composer_managed_libraries_for_use_on_WMF_cluster is a desire to include more libraries that are maintained by other developers, to get the benefit of outside expertise and reducing the maintenance burden. The last few security reviews I've done have had a major reliance on third party code to function. So I think we do need a section in this document that at least links to a policy about what libraries we will accept.

ABaso(WMF) made a policy suggestion that I thought was good. For us to include a library, it should have 2 of these 3 characteristics. We should annually ensure our libraries continue to meet this standard, and replace any that don't:

1. Org has demonstrated security commitment (e.g., OpenStack/Google/Etsy/Twitter)
2. Org has a statement of security/process compliance
3. Org does not have a major security incident in the previous year, or if it did, demonstrated commitment to proper remediation

As part of the composer RFC discussion, I asked BDavis (WMF) to help work our a process to make sure someone (preferably inside the WMF, but an active volunteer is fine too) who can "sponsor" the library and make sure they will stay informed about security fixes in that library, and apply updates as needed. A potential substitute for this may be an automated check by SensioLabs, which could notify ops or some developers if a security flaw was found in a library.

I'd appreciate comments from anyone else.