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
base: master
from

Conversation

Projects
None yet
3 participants
@oreoshake
Collaborator

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 and others added some commits Sep 3, 2013

@oreoshake

This comment has been minimized.

Collaborator

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

This comment has been minimized.

Collaborator

oreoshake commented Sep 4, 2013

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

Neil Matatall added some commits Sep 4, 2013

# 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]

This comment has been minimized.

@oreoshake

oreoshake Sep 6, 2013

Collaborator

Error if no CSP policy set

tmp_file.unlink
raise "Base64 values do not match!!! '#{base64}' != '#{openssl_base64}'" unless base64 == openssl_base64
raise "Hex values do not match!!! '#{hex}' != '#{openssl_hex}'" unless Regexp.new(hex) =~ openssl_hex

This comment has been minimized.

@oreoshake

oreoshake Sep 6, 2013

Collaborator

openssl adds the filename to the output with some decoration. There's probably a flag I'm missing to get hex output without the framing.

csp += "script-src 'none' " + source_expression
end
headers['Content-Security-Policy-Report-Only'] = csp

This comment has been minimized.

@oreoshake

oreoshake Sep 6, 2013

Collaborator

LOL reading the write header, writing the potentially wrong header. Just another reason to store the ContentSecurityPolicy object in env and set a single string rather than doing gsub after the fact.

This comment has been minimized.

@spikebrehm
def ensure_security_headers options = {}
begin
self.script_hashes = ::YAML::load_file('config/script_hashes.yml')

This comment has been minimized.

@oreoshake

oreoshake Sep 6, 2013

Collaborator

ensure this can be set during runtime (for guard-like support without restarting the server)

@@ -31,8 +34,24 @@ def secure_headers_options
end
end
def script_hashes

This comment has been minimized.

@oreoshake

oreoshake Sep 6, 2013

Collaborator

this is C/P'd from above. Generalize.

tmp_file.write(inline_script)
tmp_file.close
openssl_base64 = `openssl dgst -sha256 -binary #{tmp_file.path} | base64`.chomp

This comment has been minimized.

@coderanger

coderanger Sep 6, 2013

Contributor

Any reason you aren't doing this in Ruby? Only hurts platform compat and requires file I/O for no obvious reason.

This comment has been minimized.

@oreoshake

oreoshake Sep 6, 2013

Collaborator

It's a super paranoid integration test that shows it works to non ruby people. It's behind the debug flag and probably won't be in the final branch anyways.

@oreoshake

This comment has been minimized.

Collaborator

oreoshake commented Sep 6, 2013

@oreoshake

This comment has been minimized.

Collaborator

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

This comment has been minimized.

Collaborator

oreoshake commented Oct 15, 2013

This PoC has served it's purpose

@oreoshake oreoshake closed this Oct 15, 2013

@oreoshake

This comment has been minimized.

Collaborator

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Collaborator

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

This comment has been minimized.

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

This comment has been minimized.

Collaborator

oreoshake commented Aug 25, 2014

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

@oreoshake

This comment has been minimized.

Collaborator

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

This comment has been minimized.

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

This comment has been minimized.

@spikebrehm

spikebrehm Sep 2, 2014

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.

This comment has been minimized.

@oreoshake

oreoshake Sep 2, 2014

Collaborator

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 :)

This comment has been minimized.

@spikebrehm

spikebrehm Sep 3, 2014

Here we go: #109

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

This comment has been minimized.

@oreoshake

oreoshake Sep 3, 2014

Collaborator

Oh sweet. Didn't know that.

spikebrehm and others added some commits Sep 3, 2014

Fix support for mutliple script hashes
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'
Neil Matatall
Merge branch 'master' into script_hash
Conflicts:
	lib/secure_headers/railtie.rb
	spec/lib/secure_headers_spec.rb
Merge pull request #109 from spikebrehm/fix-multiple-script-hash
Fix support for mutliple script hashes
Neil Matatall
@oreoshake

This comment has been minimized.

Collaborator

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

This comment has been minimized.

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

This comment has been minimized.

Collaborator

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

This comment has been minimized.

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

This comment has been minimized.

@spikebrehm

spikebrehm Sep 3, 2014

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'?

This comment has been minimized.

@oreoshake

oreoshake Sep 3, 2014

Collaborator

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 :shrug:

@oreoshake

This comment has been minimized.

Collaborator

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