Thread

I’m working with a news organisation where members can instantly post images, and anonymous (i.e. non-members) can also post images but they go into a moderation queue.

Using the following SafeCracker pair tags:

{exp:safecracker channel="image" logged_out_member_id="3" status="Pending Moderation" }
{/exp:safecracker]

Will generate the following hidden variables:

<input type="hidden" name="status" value="Pending Moderation">

The problem is if one just changes the “Pending Moderation” value to “open” within the DOM and submit the form, it works and the image is submitted with it’s status as open, thus bypassing the queue.

How can we lock down this security hole?

He Michael, you may want to try going this route: http://ellislab.com/expressionengine/user-guide/cp/admin/channels/statuses_edit.html#restrict-status-to-members-of-specific-groups

That should keep your status tinkerers from adjusting/changing settings.

Unfortunately that’s just a bandaid fix, a-little bit more of an investigation and it’s clear that the underlying architecture of SafeCracker is fatally flawed as per my bug report.

With a tag pair like:

{exp:safecracker channel=“test” return=”/test/safecracker” rules:article_excerpt=“required|min_length[20]” rules:article_content=“required|min_length[100]” status=“Pending”}

Will result in the following encrypted hidden fields:

<input type=“hidden” name=“rules[article_excerpt]” value=“cmVxdWlyZWR8bWluX2xlbmd0aFsyMF04NzM1MjJiNjU4MDk0YjRkMGM4NzQ4NTUxNTYxNWIxOA==”>
<input type=“hidden” name=“rules[article_content]” value=“cmVxdWlyZWR8bWluX2xlbmd0aFs0XTM1Y2IyOTliOWFlODcyZGMyOGY1MThmYjBhOGEzNmQ3”>

And one can just easily copy/paste the encrypted strings from one rule to another; effectively making all your rules as strong as the weakest rule.

<input type=“hidden” name=“rules[article_excerpt]” value=“cmVxdWlyZWR8bWluX2xlbmd0aFsyMF04NzM1MjJiNjU4MDk0YjRkMGM4NzQ4NTUxNTYxNWIxOA==”>
<input type=“hidden” name=“rules[article_content]” value=“cmVxdWlyZWR8bWluX2xlbmd0aFsyMF04NzM1MjJiNjU4MDk0YjRkMGM4NzQ4NTUxNTYxNWIxOA==”>

The whole client side storage and rules, even in an ideal world of it being rock solid and safe is just too big of a gamble in my opinion. I am investigating now how much work it’ll be to re-jig SafeCracker to not store anything client side, or just ditch it all together and bring in Codeigniter’s core form handling classes and provide some ExpressionEngine glue.

Michael,

I’ve been having similar problems with SafeCracker, I currently have a hidden price field that gets auto updated (And a Status field) BUT, I realised that anybody using Inspect Element can just change the value and submit the form. Getting away with a cheap bargain and ruining my whole setup.

How did you ‘encrypt’ your values? I can’t sem to find any parameter in your exp:safecracker tag - or is this a default setting?

I’d love to hear how you go about solving this problem, can you post updates if you master it?

Hi Michael,

I’ve brought this to the Engineers to get you a solid answer.

Thanks!

~

Hey all.  The best way to follow the specifics on this one is to follow the bug report here.  But Dan thought a more general overview of the issues involved in frontend forms might be useful.  So…

As noted- it’s easy to overwrite form data- or add your own key/values.  This is particularly problematic because with forms, really the only way to pass along parameters is via the post data (typically they get added as hidden fields).  Since hidden fields are as easy to manipulate as any other, there are 2 basic ways we typically keep folks from being able to ‘rewrite’ those parameters however they please:

1. Keep key settings in the database (or config or what have you).  This is pretty much what Michael’s talking about with keeping them out of client side.  So a good example is SC’s ability to specify some settings via the CP.  Or how you set comment posting permissions in the cp- rather than just having a parameter.

Of course, it’s less flexible to have settings stored this way- particularly if you may want different settings for multiple forms.  Or SC’s ‘rules’- which you can have multiple of for every custom field you have.  Imagine the CP if you’re trying to set all possible rules/limitations/defaults/etc for multiple forms in the CP.

2. Pass parameters as hidden fields- but encrypt the data so that it can’t be meaningfully manipulated.  SC is taking each parameter and encrypting the value- and adding them as hidden fields.  It uses the sess_crypt_key, which is unique to each user (and site specific).  This would likely be sufficient in most cases- but Michael’s right in that some values you could copy over- knowing that a number is going to be a number and such.  I’m not sure how massively useful that is, but in some instances I’m sure it could be problematic.

Fortunately this approach can be improved on- see the _build_meta_array() method in mod.search.php.  It lumps ALL possible parameters together, serializes, then encodes that based on site specific data.  You end up with one big, nasty, useless to anyone else, hidden field.  So that may be the route we take re: the bug.

All that said- Shape?  If we’re talking products and money?  I’d personally go with option #1- store the prices in the DB and double check that the product_id/price that comes through the form matches up to the official price.  It’s easy to do something like that with a hook/extension, and I really like that sort of double check when it comes down to money. 

Hope that clarifies things a bit- and like I say- the bug tracker will be updated once a more specific answer to the issue has been coded.

Thanks and if GDMac is reading, I’ve taken a quick look at your Safecracker Lockdown extension, and while a solid effort and is defintely a step in the right direction - it’s still not going to solve our use case.

Particularly if you may want different settings for multiple forms.

This is actually what we’re doing, we’re got a news content submission form that accepts news stories from logged in journalists as-well as whistle blowing-style stories from anonymous users. Both needing to be moderated. The fact that we were able to sneak past the moderation queue was a red flag, regardless of whether or not a rule was set-up in the CP or tag pair - there shouldn’t be any circumstances were a rule can be overridden by the client.

That’s my big concern. The fact that any validation is within the view, no matter how secure it could ever be - and given that encrypted fields can already be fiddled with, means SafeCracker must be considered a genuine attack vector.

You end up with one big, nasty, useless to anyone else, hidden field.  So that may be the route we take re: the bug.

Understandably that the EE architecture is different to that of CodeIgniters, but I would strongly suggest that the permeant fix be sought that does not store anything client side, encrypted or not. Similar to the CI route of placing any requirements in the controller, and not the view.

If we’re talking products and money? I’d personally go with option #1

In our case, we’re talking about real journalists livelihoods and possible persecution from government if an anonymous user was able to sneak something past the form, and assign to to a journalist’s member id, so security is big for us.

We’ve been working (and loving!) with CI since 2008, hence the pull to EE. We’d already worked in the past with pulling in custom CI controllers into EE, so our interim solution will be to do the same (bringing in CI form controllers into EE) for any form submissions on the site and completely remove SafeCracker from the development install.

The production site is still months upon months away (we’re not even begun writing the damn site! just performing some security assessments), so hopefully EE can sort something out in the upcoming months.

Thanks for the follow-up on this, Michael. Looks like the engineers have what they need for now. Is there anything else we can do for you?