Security and Login functionality for SS4

199 views
Skip to first unread message

Simon Erkelens

unread,
Apr 1, 2016, 7:18:37 PM4/1/16
to SilverStripe Core Development
The questions below are to improve the extendability of the security and login related functionality currently implemented. The security of the framework or CMS is not at risk. I just want to improve it.

Continuing from what I've already done (Sorry, can't show it off yet), I'd like to have your feedback, are there issues you run into with the security process, which I/we can take into account improving the backend?
Currently on my list of todo is:
  • Sanitation: Make sure we don't end up with injection issues by addressing $_REQUEST and/or $_POST directly
  • Extendability: Make sure the code has it's hooks and extendability in all major parts of the security implementation, without compromising the security itself.
  • Flexibility: Making sure the core security doesn't revolve around Email field, as it does in SilverStripe 2 and 3, but is flexible towards user request (e.g. use FirstName/Surname combination instead of Email).
  • Security: Making it easier to implement your own security methods, if you wish to do so. Right now, for example, the salt and encryption methods are stored in the member database table. Should this be moved to a new table, or maybe be peppered with a hash (I don't think the last part is needed, but if highly requested, I can look into it).
  • Extraction: Right now, Security and Framework are very much tied together. I'd like to extract it to support custom CMS modules. This is a far-fetched goal though.
Are there any wannahaves, things you would like to see in Security, to for example hook in easier or have more control?

Patrick Nelson

unread,
Apr 1, 2016, 7:47:23 PM4/1/16
to silverst...@googlegroups.com
First of a few questions regarding your to-do list:
  • Sanitation: Make sure we don't end up with injection issues by addressing $_REQUEST and/or $_POST directly
In what ways specifically are external input not already sanitized properly for authentication reasons? 

  • Extendability: Make sure the code has it's hooks and extendability in all major parts of the security implementation, without compromising the security itself.
 
Are there specific examples of what you'd like to have be more extensible which are not currently able to be done without core modification?

  • Security: Making it easier to implement your own security methods, if you wish to do so. Right now, for example, the salt and encryption methods are stored in the member database table. Should this be moved to a new table, or maybe be peppered with a hash (I don't think the last part is needed, but if highly requested, I can look into it).
A quick look at the code (from what I can tell) reveals that you can either A.) Override MemberAuthenticator with your own custom class and use the Injector to ensure the system uses your overridden version of that class to more completely/fundamentally override how authentication currently works (including I believe decoupling the tight binding to the email field I suppose), or B.) Do the same thing for the Member class as well and simply change how (or where) those fields are stored, if you wanted to retain the "Member" table but store it somewhere else for whatever reason.

  • Extraction: Right now, Security and Framework are very much tied together. I'd like to extract it to support custom CMS modules. This is a far-fetched goal though.
You mean abstracting "Security" into it's own module (similar to how SiteConfig was for v3.2 or v3.3 I believe) correct?

Anyway, for my "wannahaves", I'd love to see:
  • Very Important: Require a user to enter their own password first prior to resetting their password. At the moment, the user only needs to confirm the new password they just typed (for accuracy). Not to confirm that they're not just some person who walked up to a person's already-open session.

  • An easier and more intuitive way to allow users to change their own password. Amazingly there's no obvious option to do this in the CMS "out of the box"!

  • Email notifications to notify users that their password has been changed or reset.

  • An easy way to incorporate two factor authentication. How that'd be done I have no clue :) e.g. 
    • Maybe expose an API to salt the password with an RSA ID prefix (rolling code, shared secret) as the user types it in or
    • Maybe allowing the injection of a second authentication form which requires that second input (e.g. value from a text message).

  • Specific use case (but common one) as an example to help set a goal: Perform a review of the core code base to ensure that the API is opened up enough add standard support for SAML SSO (Single Sign On). In case that's not already possible.
I think that first item is more important than everything else (albeit it doesn't relate to initial login). But it's an important feature that I'm amazed doesn't already exist; at least not in 3.3.




--
You received this message because you are subscribed to the Google Groups "SilverStripe Core Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to silverstripe-d...@googlegroups.com.
To post to this group, send email to silverst...@googlegroups.com.
Visit this group at https://groups.google.com/group/silverstripe-dev.
For more options, visit https://groups.google.com/d/optout.

Patrick Nelson

unread,
Apr 1, 2016, 7:55:12 PM4/1/16
to silverst...@googlegroups.com
Also Simon, another response to this:

  • Security: Making it easier to implement your own security methods, if you wish to do so. Right now, for example, the salt and encryption methods are stored in the member database table. Should this be moved to a new table, or maybe be peppered with a hash (I don't think the last part is needed, but if highly requested, I can look into it).
The answer to your question I think is "No," it should not. It doesn't increase security in any way. Password hash itself is already peppered -- with a salt (lol, excuse the irony). The point of the salt is to reduce the exposure to rainbow table attacks as well as any other already pre-stored/pre-calculated hash tables. i.e. You are actually required to do the calculation/work to calculate the given hash (default being blowfish algorithm). This means that currently by default we're basically using BCrypt with a "cost" (number of rounds) of 10. That means that you have to do the computational overhead of performing 10 rounds of blowfish encryption (with the salt, I believe) in order to compare your version of the hash with the stored one. The idea here is that the hash is not reversible (given) but you can generate a hash that may match the stored hash (by hashing from scratch) and comparing the results. 

So from what I can tell, this isn't necessary, since it appear to already be "secure" (in some sense of the term -- i.e. within reason) due to the cost/overhead required to crack these passwords. Especially with the salt already there "peppering" the hash to help prevent issues relating to the hash already existing somewhere on the web and ensuring that salt varies per person and (also) ensuring the salt is properly randomized (which can be a hairy compsci topic, I guess).
 

Simon Erkelens

unread,
Apr 1, 2016, 8:45:13 PM4/1/16
to SilverStripe Core Development


On Saturday, 2 April 2016 12:47:23 UTC+13, Patrick Nelson wrote:
First of a few questions regarding your to-do list:
  • Sanitation: Make sure we don't end up with injection issues by addressing $_REQUEST and/or $_POST directly
In what ways specifically are external input not already sanitized properly for authentication reasons? 
For example, at a few points, there is direct access to $_REQUEST in the Security class. This is an unhealthy approach. 

  • Extendability: Make sure the code has it's hooks and extendability in all major parts of the security implementation, without compromising the security itself.
 
Are there specific examples of what you'd like to have be more extensible which are not currently able to be done without core modification?
No, the other way around. Currently, when you want to implement or override some parts of authentication, you have to copy-and-paste almost every function in order to override just one little thing. 

  • Security: Making it easier to implement your own security methods, if you wish to do so. Right now, for example, the salt and encryption methods are stored in the member database table. Should this be moved to a new table, or maybe be peppered with a hash (I don't think the last part is needed, but if highly requested, I can look into it).
A quick look at the code (from what I can tell) reveals that you can either A.) Override MemberAuthenticator with your own custom class and use the Injector to ensure the system uses your overridden version of that class to more completely/fundamentally override how authentication currently works (including I believe decoupling the tight binding to the email field I suppose), or B.) Do the same thing for the Member class as well and simply change how (or where) those fields are stored, if you wanted to retain the "Member" table but store it somewhere else for whatever reason.
I'm not saying, at this moment it's "bad", I mean it could and should be improved. From the looks of it, it hasn't had much love recently.
Regarding the peppering etc. I am aware that it doesn't add much security and only adds overhead. But if it gets a high demand, why not add the possibility to do so?

  • Extraction: Right now, Security and Framework are very much tied together. I'd like to extract it to support custom CMS modules. This is a far-fetched goal though.
You mean abstracting "Security" into it's own module (similar to how SiteConfig was for v3.2 or v3.3 I believe) correct?
Partially. My main goal is to have it more flexible. Maybe not make it it's own module, I'm not sure of that yet. But also, a very far-fetched goal at the moment. 

Anyway, for my "wannahaves", I'd love to see:
  • Very Important: Require a user to enter their own password first prior to resetting their password. At the moment, the user only needs to confirm the new password they just typed (for accuracy). Not to confirm that they're not just some person who walked up to a person's already-open session.

  • An easier and more intuitive way to allow users to change their own password. Amazingly there's no obvious option to do this in the CMS "out of the box"!

  • Email notifications to notify users that their password has been changed or reset.

  • An easy way to incorporate two factor authentication. How that'd be done I have no clue :) e.g. 
    • Maybe expose an API to salt the password with an RSA ID prefix (rolling code, shared secret) as the user types it in or
    • Maybe allowing the injection of a second authentication form which requires that second input (e.g. value from a text message).

  • Specific use case (but common one) as an example to help set a goal: Perform a review of the core code base to ensure that the API is opened up enough add standard support for SAML SSO (Single Sign On). In case that's not already possible.
I think that first item is more important than everything else (albeit it doesn't relate to initial login). But it's an important feature that I'm amazed doesn't already exist; at least not in 3.3.

Nice feedback. The current change-password functionality is slightly under fire on GitHub as well ;) 

Patrick Nelson

unread,
Apr 1, 2016, 9:03:01 PM4/1/16
to silverst...@googlegroups.com
With, with regards to "peppering" -- I'm saying it's unnecessary because a salt is already being used. Having a BCrypt password hash + a salt in the same table as the member email/etc is a good process, so that doesn't need to change from what I can tell; we're already peppering passwords with that salt so, this rightly "high demand" feature is already in place :)

Now I see what you mean about working with $_REQUEST (et al). I think this is part and parcel with the global state that's maintained in SS at large, so it's a perfect example of what shouldn't be done. Goes well with your suggestion to abstract things more (to more methods, in the case of your point about having to copy/paste everything) so that then you can just override one much smaller method. this includes properly passing around dependencies, e.g. SS_HTTPRequest from which you extract the necessary input. This helps with scaffolding for unit testing and etc but also for good isolation and separation of concerns. I feel like we should have more PR's like this; not just monkey patching some extra features, but just in cleaning stuff up and reorganizing (v4 is our opportunity for this in case there needs to be API breaking changes).

Simon Erkelens

unread,
Apr 1, 2016, 10:39:46 PM4/1/16
to silverst...@googlegroups.com
Excellent summary of my train of thought ;)
That's why I started this topic. To get at least feedback. I'll probably soon open up a github repo where I propose my changes and ofcourse more feedback and RFC docs etc.

--
You received this message because you are subscribed to a topic in the Google Groups "SilverStripe Core Development" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/silverstripe-dev/308k1-RP-4M/unsubscribe.
To unsubscribe from this group and all its topics, send an email to silverstripe-d...@googlegroups.com.

To post to this group, send email to silverst...@googlegroups.com.
Visit this group at https://groups.google.com/group/silverstripe-dev.
For more options, visit https://groups.google.com/d/optout.



--
Simon Erkelens | Developer
SilverStripe

Jonathon Menz

unread,
Apr 5, 2016, 1:31:33 PM4/5/16
to SilverStripe Core Development
Hey Simon,

If you're considering UX as well, carrying on from https://github.com/silverstripe/silverstripe-framework/issues/5203 I think that after a password has been reset or changed a user should always be shown a success message. I think it might be good idea to do this when logging out as well, as some websites might not provide an obvious indicator of whether or not you're logged out if you're taken back to the home page. Also consider that when you log out of the CMS, you're redirected to a login form that intends to bring you back in to the CMS. Why would I want to do that if I just chose to log out? Think it would be better to display a 'You successfully logged out' page using a consistent approach to a 'Your password was successfully changed' message.

Simon Erkelens

unread,
Apr 7, 2016, 3:50:14 AM4/7/16
to SilverStripe Core Development
Hi Jonathan,

Yep, that was also on my list of things to fix. Implementation might differ from yours, but I have been keeping an eye on GitHub issues around the security functions etc. as well.

The CMS logout should, I think, go to a "you're logged out, go to home or back to CMS" option or something. I'll need to fix my git repo now first though, because if my laptop collapses in some weird earthquake or something, most of my current work will be gone...

swaiba

unread,
Apr 7, 2016, 11:10:58 AM4/7/16
to SilverStripe Core Development
Hey Simon, if you could address this... https://silverstripe.uservoice.com/forums/251266-new-features/suggestions/9393579-signpost-clearly-if-logged-in-during-reset-passswo

I'd be grateful as a month doesn't go by when I have to explain this to someone...

Jonathon Menz

unread,
Apr 7, 2016, 12:51:54 PM4/7/16
to SilverStripe Core Development
Sounds good Simon, thanks. Get that work in the cloud :D another earthquake in NZ wouldn't be all that weird (we're on a fault line here on Vancouver Island too... home insurance is bonkers)

Mark Muller

unread,
Apr 7, 2016, 12:57:38 PM4/7/16
to SilverStripe Core Development
Hi Simon,

If you want my two cents it would be fantastic to add redirects based on group or permission roles into the admin of security along with offering the ability to choose the templates in use for each group/member.

I have just spent some time extending both the Security class and the AdminLogin stuff to achieve this but it was complicated to say the least. I'm not with the code at the moment but I can send it to you later of you want to see my working example?

All the best,

Mark

Simon Erkelens

unread,
Apr 12, 2016, 5:04:25 AM4/12/16
to SilverStripe Core Development
To anyone interested.
Here's my progress so far. It's far from complete and very WIP, but feel free to create issues.

Simon Erkelens

unread,
Apr 20, 2016, 7:05:28 AM4/20/16
to SilverStripe Core Development
I've ripped out most of the security parts out of a member. It's secure heart and brains are now living on in MemberSecurity.

The reason for this, is this way, it is possible to use the CMS with members, without relying on the security backend, and my wish is to extend it further.

If you want to have a play, look at my git repo, and download the packages (I don't think they're on packagist).
I'll update my no-security-here framework version asap, so they play together nice.

Note, I ripped out security completely, just to be able to work on it separately in a completely separate repo, until I'm at least somewhat happy with the results, to merge back in.
Also, I use ZenHub, so if your issue suddenly gets weird notifications about getting assigned to epics, stories, or get numbers assigned, that's ZenHub stuff. :)

And, to close it off:
Feedback please, and pull requests is good too!

swaiba

unread,
Apr 20, 2016, 8:12:23 AM4/20/16
to SilverStripe Core Development
Hey Simon,

We discussed some security stuff on IRC the other day in relation to this.  What we will do as a first step is a more detailed email... to this group I guess... to explain and add code snippets for what we have done - as a pull request would be a great amount of work in the case that it isn't going to make it forward.

Barry

Martijn

unread,
Apr 20, 2016, 8:15:15 AM4/20/16
to SilverStripe Core Development
I will try to create some pull requests and review the code more indepth.

But I what I can come up with now :

  • ForgotPasswordEmail is in the MemberLoginForm
    • sometimes you want to do $member->resetPassword(); or $member->sendPasswordResetLink(); from code. ofcourse I could copy the code, but its a pretty common piece of funtionality that does not belong to the Form only.
  • Custom Redirect urls after login
    • Currently only supported with BackUrl
    • You can't really hook into all the $this->controller->redirect() calls
  • I think the registration of Authenticaters is pretty cumbersome and a lot assumes on the existince of a Form to authenticate
    • Maybe code against an interface instead of the need to subclass AbstractAuthenticator ?
  • It would be nice to not really on Zend for Date and Time and other thinks... ;)
  • It would be nice to have default DateFormat and TimeFormat in Config instead of picking one from Locale. (I always remove those fields for my clients once set in $defaults[], but it requires a DataExtension just for that ..)
  • Personaly I would rename Security to Users/Members in the CMS. Member is used also just to hold data from People, that do not necessarily have a 'security clearance'..


Regarding the hooks. Its not a Security only issue, but an SS issue, but :


All those onAfterSomething(); method call hooks. I cant remember them or find them if they are added somewhere. And those only work for Extensions


An event()->fire('user_logged_in'); that can be used in an EventListener would be nice. (yes we can list the alle events quite easy then)


But after all it works pretty decent for most thinks what I need for more then over 7 years by now ;)


Mark Muller

unread,
Apr 20, 2016, 11:50:00 AM4/20/16
to SilverStripe Core Development
Martijn,

I agree 'Security' in Modeladmin is a bit misleading. 'Users' sits better imho. 'Members' always gets a ** snigger ** from my clients! Don't ask me...

Custom Redirect URLs after login <<-- Huge plus, would be good to specify from a list of sitetree pages or even controllers with available index within the system?

Along with, if feasible, custom templates/images. I have just done something similar, mine works by creating a custom url for each Group stored in a DataExtension along with the file uploads necessary, these are then used by the Security controller to get the specific images and extra info during ->customise->renderWith for each URL

Understand that this is a wish list but its good to discuss as it seems at times that Security is the least 'loved' part of SS :D

Regards,

Mark

Simon Erkelens

unread,
Apr 20, 2016, 8:37:14 PM4/20/16
to SilverStripe Core Development
Do you have a time I can look at in the logs?

Simon Erkelens

unread,
Apr 21, 2016, 3:03:54 AM4/21/16
to SilverStripe Core Development
Could you create a bunch of issues for this, and if you have time, also feel free to create PR's for it. I'm currently working on cleaning up the Member GodObject by splitting things out.

Martijn

unread,
Apr 26, 2016, 11:23:07 AM4/26/16
to SilverStripe Core Development

I added them as issues on the git repo, but may post them here as well for further discussion :

Move all Security.php methods to a Trait
https://github.com/CasaLaguna/silverstripe-security/issues/15

Decouple LoggedIn state from Member

https://github.com/CasaLaguna/silverstripe-security/issues/14
Reply all
Reply to author
Forward
0 new messages