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

script hash rails integration PoC #67

Closed
wants to merge 15 commits into from
Closed

script hash rails integration PoC #67

wants to merge 15 commits into from

Conversation

oreoshake
Copy link
Contributor

@oreoshake oreoshake commented Sep 4, 2013

Application demonstrating this: https://github.com/oreoshake/script_hash_test
Video demo: https://www.youtube.com/embed/Bc2hvziTRxg

Two pieces:

  • Rake task to find all inline script, compute the script-hash value, store in config/script_hash.yml

AND

  • Contains new before_filter to subscribe to ActiveSupport::Notifications events (rendering partials/templates)
  • All rendered templates write to an env value that contains all hashes for each rendered template/partial
  • Middleware then (stupidly) does a find/replace or adds a directive containing the script hash values

Based on "simple" script hash proposal.
Need to verify hashed values with various methods.

This makes me think the before_filters should create a ContentSecurityPolicy object and build the entire header in the middleware.

@oreoshake
Copy link
Contributor Author

oreoshake commented Sep 4, 2013

Super hacky, no tests, no haml support(?), only works for rails 3.1+. Not going to supply a Capistrano task for now... suggesting guard-rake because if somehow you forgot to update your hashes before deploy... clearly you didn't run/have integration tests.

@oreoshake
Copy link
Contributor Author

oreoshake commented Sep 4, 2013

Only works if entire tag is on one line lol. Don't use scan.

# jank to make sure we're messing w/ the right header
header_name = ContentSecurityPolicy.new(nil, :ua => env["HTTP_USER_AGENT"]).name

csp = headers[header_name]
Copy link
Contributor Author

@oreoshake oreoshake Sep 6, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error if no CSP policy set

@oreoshake
Copy link
Contributor Author

oreoshake commented Sep 6, 2013

Writeup of the why and how of this branch: http://nmatatal.blogspot.com/2013/09/how-my-script-hash-poc-works.html

@oreoshake
Copy link
Contributor Author

oreoshake commented Sep 12, 2013

JS equivalent hash computation with jQuery and cryptojs

$.each($('script'), function(index, x) { console.log(CryptoJS.SHA256(x.innerHTML).toString(CryptoJS.enc.Base64));});

@oreoshake
Copy link
Contributor Author

oreoshake commented Oct 15, 2013

This PoC has served it's purpose

@oreoshake oreoshake closed this Oct 15, 2013
@oreoshake
Copy link
Contributor Author

oreoshake commented Jan 17, 2014

Reopening because someone else might be motivated to make this a thing before I get to it.

Some people would rather support dynamic hashing (in development only) rather than have an external process (guard) grep/populate hashes.

I still disagree. Whatevs.

@oreoshake oreoshake reopened this Jan 17, 2014
@spikebrehm
Copy link

spikebrehm commented Aug 21, 2014

Sweet, thanks for this PoC. What do you think about a more simple approach of using a specific Rails helper, say, inline_script_with hash:

<%= inline_script_with_hash do %>
  var timestamp = Date.now();
<% end %>

which explicitly computes a hash, and saves it to a list for later (controller instance variable, request.env, whatever), and the SecureHeaders gem utilizes the list when building the header?

Or is this the approach you mention here:

Some people would rather support dynamic hashing (in development only) rather than have an external process (guard) grep/populate hashes.

@spikebrehm
Copy link

spikebrehm commented Aug 21, 2014

FWIW I got this working by copying over your SecureHeaders::ScriptHash middleware into our app, and with this helper:

module InlineScriptHelper
  def inline_script_with_hash(&block)
    content = "\n#{capture(&block)}"
    (request.env['script_hashes'] ||= []) << compute_inline_script_hash(content)
    content_tag :script, content
  end

  def compute_inline_script_hash(content)
    Digest::SHA256.base64digest(content)
  end
end

@oreoshake
Copy link
Contributor Author

oreoshake commented Aug 23, 2014

@spikebrehm I like it. A lot. I still have to support mustache but that shouldn't be too hard.

@spikebrehm
Copy link

spikebrehm commented Aug 25, 2014

Are there plans to change secureheaders to support computing the header in a middleware, in a less-hacky way? I notice this is a year old. If not, I suppose we'll fork this to support it, but I'd rather not fork if we can help it.

@oreoshake
Copy link
Contributor Author

oreoshake commented Aug 25, 2014

This branch was just a PoC. I welcome any pull requests on this! Don't fork!

@oreoshake
Copy link
Contributor Author

oreoshake commented Aug 25, 2014

One issue with this dynamic hashing is that it doesn't buy you anything. I'd want the hash values to be static or otherwise limited in production

@spikebrehm
Copy link

spikebrehm commented Aug 25, 2014

Sure, I can agree that hashing of dynamic script content may be a bad idea, and that the helper approach makes it easy to do that.

However, I do think that the helper approach is simpler and easier to use than Rake task / YAML approach.


# need to settle on stuffs
def hash_source_expression(hashes, format = "sha256", delimeter = "-", hash_delimeter = " ", wrapper = "'")
wrapper + format + delimeter + hashes.join(hash_delimeter) + wrapper

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This causes improperly formatted script-src when there's multiple script hashes. As is, the output for two hashes, abcde and fjhij is:

script-src 'sha256-abcde fghij'

This should be:

script-src 'sha256-abcde' 'sha256-fghij'

Here's the proper source code:

def hash_source_expression(hashes, format = "sha256", delimeter = "-", hash_delimeter = " ", wrapper = "'")
  hashes.map { |hash| wrapper + format + delimeter + hash + wrapper }.join(hash_delimeter)
end

If you like I can make a PR to this branch that fixes.

Copy link
Contributor Author

@oreoshake oreoshake Sep 2, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should also be sha1. Only sha1 is supported for now afaik.

Please do submit a PR if you have the time, including your other changes. Or submit a fresh PR :)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we go: #109

Should it really be sha1? sha256 works for me in Chrome 37.

Copy link
Contributor Author

@oreoshake oreoshake Sep 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sweet. Didn't know that.

spikebrehm and others added 5 commits Sep 3, 2014
There exists a bug which causes ian improperly formatted `script-src`
expression for multiple script hashes. As is, the output for two hash
`abcde` and `fjhij` is:

    script-src 'sha256-abcde fghij'

This should be:

    script-src 'sha256-abcde' 'sha256-fghij'
Conflicts:
	lib/secure_headers/railtie.rb
	spec/lib/secure_headers_spec.rb
Fix support for mutliple script hashes
@oreoshake
Copy link
Contributor Author

oreoshake commented Sep 3, 2014

However, I do think that the helper approach is simpler and easier to use than Rake task / YAML approach.

Totally. So how about the helper works in !production and then we just make the hash generation just another deploy step?

I'd love to iron all this out and ship something somewhat soon. I should have the cycles too.

@spikebrehm
Copy link

spikebrehm commented Sep 3, 2014

Totally. So how about the helper works in !production and then we just make the hash generation just another deploy step?

Today I've actually been implementing the YAML approach, and there does seem to be a benefit in the fact that it's literally impossible to whitelist dynamic scripts. The helper approach doesn't have that benefit, because you will always be whitelisting the dynamic value post-evaluation. That is certainly a nice development experience, but it may be too permissive given an organization's needs.

The downside of the YAML approach is that the developer would have to run the Rake task and restart the Rails server every time they add/update a <script> tag. That sounds like a PITA, but hopefully in practice app developers shouldn't actually be writing script tags; there should be patterns in place that make writing script tags a special case.

You're proposing a hybrid approach? What would that look like?

@oreoshake
Copy link
Contributor Author

oreoshake commented Sep 3, 2014

I received some feedback that a helper that dynamically computes the values during development is pretty much necessary.

One option is to always calculate hashes dynamically AND compare to a list of known hashes and respond to mismatches in a configurable way. i.e. "OH NOEZ raise exception" vs "yeah, we logged that out and we'll clean it up in the next release, don't blow up in the meantime"

csp = {
  ...,
  rescue_from_script_hash_mismatch: lambda {
    if RAILS.env.production?
      raise "go home, you're drunk"
    elsif RAILS.env.development?
      puts "oops, unexpected hash, run be rake secure_headers:generate_hashes"
    end
  }
}

Perhaps the lack of a rescue handler implies that the dynamic calculation should not even happen, and the inline script helper becomes a noop, falling back to the yaml-only behavior.

@spikebrehm
Copy link

spikebrehm commented Sep 3, 2014

Interesting, I like that. So the script-finding regex will also have to support finding the ERB statement containing the call to the helper.

def script_hashes
if @script_hashes
@script_hashes
elsif superclass.respond_to?(:script_hashes) # stop at application_controller

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be an issue if the superclass is itself a subclass of ApplicationController, yeah? i.e.:

MyController < AuthenticatedController < ApplicationController

Is it too hacky to recursively look up the superclass tree until it finds superclass.name == 'ApplicationController'?

Copy link
Contributor Author

@oreoshake oreoshake Sep 3, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds about right, I think that's only the case when you override settings from one controller to another. I guess this has just never come up 🤷

@oreoshake
Copy link
Contributor Author

oreoshake commented Sep 3, 2014

Yeah, I just hope we don't run into whitespace issues.

@oreoshake oreoshake closed this Nov 20, 2014
@oreoshake oreoshake deleted the script_hash branch Dec 6, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants