Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: Add check for versioning secret_token #200

Closed
brynary opened this Issue Dec 9, 2012 · 11 comments

Comments

Projects
None yet
4 participants
@brynary
Copy link
Contributor

commented Dec 9, 2012

This may be a bit controversial. I believe Rails' default of versioning the value of config.secret_token is a security issue. In effect, it means anyone who has a copy of your source code can spoof your sessions.

Pretty much everyone has this versioned in their code, but I think it's a terrible idea. Critical secrets should be limited to the production environment only.

Thoughts?

@presidentbeef

This comment has been minimized.

Copy link
Owner

commented Dec 9, 2012

Is it common to actually use that value, or to replace it in production? I'm not sure. I'm leery of this one... maybe @oreoshake has an opinion.

@brynary

This comment has been minimized.

Copy link
Contributor Author

commented Dec 9, 2012

I've almost never seen it replaced in production in the many apps I've worked on over the years, unfortunately.

I suspect most teams simply don't realize the significance of that value -- that is, that with it you can trivially construct a session identifying you as any (super) user in the app.

@oreoshake

This comment has been minimized.

Copy link
Contributor

commented Dec 9, 2012

You know I'm all for helping people keeps secrets out of source code ;)
coincidently enough, I was updating pass3rd to be moxie-compliant - a tool
that deals with this issue.

Anyhow, I'm torn. This would be a somewhat political stance. While I
philosophically agree, that's just another thing to make people complain
about noisy results. Generally I don't care about "their" opinions on this
one, but I'm still torn.

If sadb was out or we had the fp marking branch done, I wouldn't flinch to
say we should do this.
On Dec 8, 2012 7:06 PM, "Bryan Helmkamp" notifications@github.com wrote:

I've almost never seen it replaced in production in the many apps I've
worked on over the years, unfortunately.

I suspect most teams simply don't realize the significance of that value
-- that is, that with it you can trivially construct a session identifying
you as any (super) user in the app.


Reply to this email directly or view it on GitHubhttps://github.com//issues/200#issuecomment-11166974.

@presidentbeef

This comment has been minimized.

Copy link
Owner

commented Dec 9, 2012

People could always skip the check?

@presidentbeef

This comment has been minimized.

Copy link
Owner

commented Dec 28, 2012

Given the recent publicity of this issue (http://phenoelit.org/blog/archives/2012/12/21/let_me_github_that_for_you/index.html), I think this is a good idea.

@brynary

This comment has been minimized.

Copy link
Contributor Author

commented Dec 28, 2012

I've also started a discussion with some of the Rails Core guys about a
systematic improvement for dealing with secrets in version control.
Session_token.rb should be git ignored by default, at an absolute minimum,
but there are safer solutions.

Newd to check if calling .findby_id() with a :select => sql_string Hash is
still allowed in Rails. That's a separate vulnerability in and of itself.

On Friday, December 28, 2012, Justin wrote:

Given the recent publicity of this issue (
http://phenoelit.org/blog/archives/2012/12/21/let_me_github_that_for_you/index.html),
I think this is a good idea.


Reply to this email directly or view it on GitHubhttps://github.com//issues/200#issuecomment-11741019.

@presidentbeef

This comment has been minimized.

Copy link
Owner

commented Dec 28, 2012

find_by_id is a problem in Rails 3.2.9 for sure, I just tested. I am not sure why these methods are doing something more complicated than they should be (i.e., checking a single column for a single value).

@oreoshake

This comment has been minimized.

Copy link
Contributor

commented Dec 28, 2012

If only there was a library for keeping secrets out of code...
On Dec 28, 2012 2:09 PM, "Justin" notifications@github.com wrote:

find_by_id is a problem in Rails 3.2.9 for sure, I just tested. I am not
sure why these methods are doing something more complicated than they
should be (i.e., checking a single column for a single value).


Reply to this email directly or view it on GitHubhttps://github.com//issues/200#issuecomment-11742840.

@brynary

This comment has been minimized.

Copy link
Contributor Author

commented Dec 28, 2012

Yikes. That's news to me and sounds quite serious. I am going to contact
the Rails security team about that ASAP.

On Friday, December 28, 2012, Justin wrote:

find_by_id is a problem in Rails 3.2.9 for sure, I just tested. I am not
sure why these methods are doing something more complicated than they
should be (i.e., checking a single column for a single value).


Reply to this email directly or view it on GitHubhttps://github.com//issues/200#issuecomment-11742840.

@matt-glover

This comment has been minimized.

Copy link

commented Dec 29, 2012

find_by_id is a problem in Rails 3.2.9 for sure, I just tested. I am not sure why these methods are doing something more complicated than they should be (i.e., checking a single column for a single value).

I suspect it ties into the same code used for the find_all_by_ dynamic finders that needed some way to chain in other parts of the call like select and order. Seems that the find_all_by_ stuff will be deprecated in rails 4 in favor of chain-able stuff like where.

presidentbeef added a commit that referenced this issue Jan 2, 2013

Add warning for secret_token
as suggested in #200
@presidentbeef

This comment has been minimized.

Copy link
Owner

commented Jan 16, 2013

Done in #277

Repository owner locked and limited conversation to collaborators Feb 16, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.