Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

spam_answers should not be attr_accessible #15

Closed
jf opened this Issue · 14 comments

2 participants

@jf
jf commented

Or else you could simply ignore the question, set spam_answers to something for which you have an answer already (like say, a previously answered question!), set spam_answer to the appropriate answer... and you're good to go!

EDIT: ok whoops, I guess i might have spoken too soon. This isnt so much a vulnerability issue, as a logical issue. spam_answers shouldnt be revealed or visible publicly; but I fail to see why it should be made attr_accessible... Care to explain?

@matthutchinson

spam_answers needs to attr_accessible since its sent with the form submission (along with your spam answer). View source on the demo here to see that in action. It looks a little something like this;

<input id="comment_spam_answers" name="comment[spam_answers]" type="hidden" value="$2a$10$1QeCVEjqAg.YBwPwF5V1TOAe4Fy.qCpsXyoParBe9vx1fvNpuJMvS">

Basically we are encrypting the content of spam_answers so that it cannot be cracked. You'd need to know the Bcrypt encoding salt defined in your config file, to break this. It's not NSA proof, but Bcrypt encryption with a decent bcrypt cost is good enough.

Most importantly, the built-in Rails authenticity_token on every form, ensures that the form submission came from the page that presented the form (see docs here for details)

And yes, you could implement this a different way - using the session (potentially insecure if using cookie session store) or some backend store to hold these answers rather than send them with the form submission, but I wanted this gem to be as simple to integrate as possible, and not force you to have to have cookies ON or use any particular backend store.

Ps.. hope that makes sense, happy to discuss further if you'd like

@jf
jf commented

I see. Thanks for the explanation! I did attempt to look at the source prior, but I guess i missed that... Thanks!

@jf jf closed this
@jf jf reopened this
@matthutchinson

No worries

@jf
jf commented

hey sorry Matt, I actually re-opened it for more discussion. So I get why spam_answers is attr_accessible. Thanks for the explanation. Is there any other way to do it, though? (see next point)

About authenticity_token: the authenticity_token takes care of CSRF (submitting without requesting the form in the first place); but there is still a sense in which you could basically store one value of a set of spam_answers (it's now given to you in the form!) + correct spam_answer... and then for all future forms that you request, simply overwrite spam_answers + send in the correct spam_answer for that spam_answers?

@matthutchinson

the thing is for all future forms you request, to submit that form you'd must have a valid authenticity_token and the only way to get that token is to ask for a brand new form, and when asking for a new form you will always generate a new question (and new answers), making your previous answer invalid you tried to submit invalid.

And yes, there are other ways to do it (and not include the answer within the form) e.g. storing the answers in the session, or a temporary cache or store server side and 2 possibilities.

@jf
jf commented

Getting a valid authenticity_token isnt a problem. But as I've highlighted, if spam_answers is attr_accessible, that means it doesnt matter what value I get as a user. It's within my control; and I can overwrite it by submitting a different value! So what if the question is different? You only validate by checking the submitted spam_answer, against the submitted spam_answers..... And spam_answers is writable and under my control, because it's attr_accessible.

@matthutchinson
@matthutchinson

Are you OK with this response? If you like you can try to crack it, I use acts_as_textcaptcha on post pages on my blog. I'd actually welcome a stress test :smile: :telescope:

@jf
jf commented

Ok, how do I say this properly? You're asking me to test your blog, which doesnt even seem to use your own solution? I see no "comment[spam_answers]" input in the form at http://matthewhutchinson.net/2013/9/16/lolcommits-v05-released#new-comment, and instead see a "possible_answers" key in the session (which is my recommended solution and definitely better than allowing for posting from the form although you still need to take care of session security!), implemented as a straight md5sum without a salt?

Instead, let's crack the demo instead (which is the one that actually runs the code here): http://textcaptcha.herokuapp.com/

Run the POST in a loop; run as many times as you like:


#crackit.rb

require 'mechanize'

agent = Mechanize.new

page = agent.get 'http://textcaptcha.herokuapp.com/'

# Captcha???? WHAT captcha???? We're a bot, and we dont need a human!!!
page.forms[0].fields[2].value, page.forms[0].fields[3].value = '$2a$10$1QeCVEjqAg.YBwPwF5V1TOwd0orKeS3.eXS8jWNlGdyKIRdtu/ny2', 'mary'

# SPAM with impunity!
page.forms[0].fields[4].value, page.forms[0].fields[5].value = 'SPAM SPAM', 'with_impunity@spam.com'

result = agent.submit page.forms[0]
puts result.search( '#error_explanation' ).to_html
puts result.search( 'p' ).first.to_html

@matthutchinson

Hi again,

Thanks for replying, my blog (I should have mentioned) runs an older version of the gem. I think you could be on to something here, and I may have misunderstood how CSRF protection works with Rails. I should get some time this week (or weekend) to look at fixing this and issuing a new gem.

& thanks again. I'll leave this issue open to track progress.

@jf
jf commented

Thanks for reopening it. I am on to something. CSRF is meant for protecting against cross-site fake requests (CSRF - Cross-Site Request Forgery). If you're going to serve up a form available from your own site and make it available for people to fill in values, what makes one particular value within the form special, so that I, the form submitter and visitor to your site, can't fill in whatever value I choose to?

@matthutchinson

On account of this issue, I've just released v4.0.0 of this gem, and updated the demo to use it.

The solution I opted for was to use the Rails.cache to store possible answers in the backend. It is accessed with a random (and mutating on every request) cache key (with a 10 minute TTL).

Let me know what you think of this approach. I think it's good enough to close this issue.

The README has also been updated to explain what changed and how to upgrade from the previous release.

Thanks again for spotting this and raising the issue. Much appreciated! :+1:

Ps.. I'm going to re-issue a release of the previous version (v3.0.10) to print a noisy warning encouraging users to upgrade to this latest 4.0.0 release.

@matthutchinson

Just pushed a v3.0.11 release, it shows a deprecation warning encouraging users to upgrade to v4.0.0 (or higher).

@matthutchinson

Closing issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.