-
Notifications
You must be signed in to change notification settings - Fork 115
Add Subresource Integrity (SRI) verification for remote stylesheets #190
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
Changes from all commits
4a374ad
5b0d739
7f5e236
3d54323
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,20 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require 'strscan' | ||
| require 'digest' | ||
| require 'base64' | ||
|
|
||
| module CssParser | ||
| # Exception class used for any errors encountered while downloading remote files. | ||
| class RemoteFileError < IOError; end | ||
|
|
||
| # Exception class used when a fetched remote file fails Subresource Integrity | ||
| # verification (see the `:integrity` option on `Parser#load_uri!`). A subclass of | ||
| # `RemoteFileError` so existing `rescue RemoteFileError` callers are unaffected; | ||
| # callers that want to distinguish an integrity failure from other fetch failures | ||
| # (404, SSRF rejection, timeout, etc.) can rescue this class specifically. | ||
| class IntegrityError < RemoteFileError; end | ||
|
|
||
| # Exception class used if a request is made to load a CSS file more than once. | ||
| class CircularReferenceError < StandardError; end | ||
|
|
||
|
|
@@ -19,6 +28,9 @@ class CircularReferenceError < StandardError; end | |
| # [<tt>io_exceptions</tt>] Throw an exception if a link can not be found. Boolean, default is <tt>true</tt>. | ||
| # [<tt>allow_local_network</tt>] Permit http(s) fetches against loopback / private / link-local / cloud-metadata addresses. Boolean, default is <tt>false</tt>. When <tt>false</tt> (the default), outbound HTTP requests are routed through <tt>ssrf_filter</tt>, which resolves the host and rejects unsafe IP ranges. Set to <tt>true</tt> only when the destination is known to be safe (e.g. local fixture servers in tests). Independent of <tt>allow_file_uris</tt>. | ||
| # [<tt>allow_file_uris</tt>] Permit <tt>file://</tt> URIs via <tt>load_uri!</tt>. Boolean, default is <tt>false</tt>. When <tt>false</tt> (the default), a caller that passes a <tt>file://</tt> URI to <tt>load_uri!</tt> — directly or via a CSS <tt>@import</tt> resolved against a <tt>file://</tt> base_uri — is refused, closing the local-file-disclosure vector when the URI is influenced by user input. <tt>load_file!</tt> is unaffected: it is the explicit local-file API and takes a caller-supplied path. Independent of <tt>allow_local_network</tt>. | ||
| # | ||
| # <tt>load_uri!</tt> also accepts a per-call <tt>:integrity</tt> option (see its documentation) for | ||
| # verifying a remote stylesheet against a Subresource Integrity value before it is parsed. | ||
| class Parser | ||
| USER_AGENT = "Ruby CSS Parser/#{CssParser::VERSION} (https://github.com/premailer/css_parser)".freeze | ||
| RULESET_TOKENIZER_RX = /\s+|\\{2,}|\\?[{}\s"]|[()]|.[^\s"{}()\\]*/.freeze | ||
|
|
@@ -37,6 +49,18 @@ class Parser | |
| # was GHSA-9pmc-p236-855h. | ||
| REMOTE_ALLOWED_SCHEMES = %w[http https].freeze | ||
|
|
||
| # Subresource Integrity hash algorithms this library can verify, mapped to their | ||
| # Digest class, ordered strongest first. A single structure (rather than a | ||
| # separate priority list and digest-class lookup) so the two can't drift out of | ||
| # sync. Mirrors the SRI spec's "agility" rule (https://www.w3.org/TR/SRI/#agility): | ||
| # when a caller-supplied `integrity` value lists more than one algorithm, only | ||
| # the strongest one present is checked. | ||
| INTEGRITY_ALGORITHMS = { | ||
| 'sha512' => Digest::SHA512, | ||
| 'sha384' => Digest::SHA384, | ||
| 'sha256' => Digest::SHA256 | ||
| }.freeze | ||
|
|
||
| # Array of CSS files that have been loaded. | ||
| attr_reader :loaded_uris | ||
|
|
||
|
|
@@ -492,7 +516,13 @@ def parse_block_into_rule_sets!(block, options = {}) # :nodoc: | |
| # | ||
| # You can also pass in file://test.css | ||
| # | ||
| # See add_block! for options. | ||
| # See add_block! for options. In addition to those, <tt>:integrity</tt> accepts a | ||
| # Subresource Integrity value (https://www.w3.org/TR/SRI/) -- e.g. the value of an | ||
| # HTML <tt><link integrity="..."></tt> attribute -- and, for http(s) URIs, verifies the | ||
| # fetched response body against it before the CSS is parsed. When the digest does not | ||
| # match, <tt>CssParser::IntegrityError</tt> (a subclass of <tt>RemoteFileError</tt>) is | ||
| # raised if <tt>io_exceptions</tt> is enabled, otherwise nothing is loaded. Ignored for | ||
| # <tt>file://</tt> URIs. | ||
| # | ||
| # Deprecated: originally accepted three params: `uri`, `base_uri` and `media_types` | ||
| def load_uri!(uri, options = {}, deprecated = nil) | ||
|
|
@@ -538,7 +568,7 @@ def load_uri!(uri, options = {}, deprecated = nil) | |
| end | ||
| read_local_file(uri) | ||
| else | ||
| src_and_charset, = read_remote_file(uri) # skip charset | ||
| src_and_charset, = read_remote_file(uri, integrity: opts[:integrity]) # skip charset | ||
| src_and_charset | ||
| end | ||
|
|
||
|
|
@@ -677,10 +707,14 @@ def read_local_file(uri) # :nodoc: | |
| # is still validated on every redirect hop, so cross-scheme | ||
| # redirect to `file://` (the original GHSA-9pmc-p236-855h sink) | ||
| # remains closed even on this opt-in path. | ||
| # | ||
| # `integrity:`, when given, is verified against the raw response body | ||
| # (before charset decoding, matching Subresource Integrity semantics) | ||
| # -- see `integrity_matches?` and `load_uri!`'s documentation. | ||
| #-- | ||
| # TODO: add option to fail silently or throw and exception on a 404 | ||
| #++ | ||
| def read_remote_file(uri) # :nodoc: | ||
| def read_remote_file(uri, integrity: nil) # :nodoc: | ||
| uri = Addressable::URI.parse(uri.to_s) | ||
|
|
||
| unless circular_reference_check(uri.to_s) | ||
|
|
@@ -711,18 +745,57 @@ def read_remote_file(uri) # :nodoc: | |
| return '', nil | ||
| end | ||
|
|
||
| if integrity && !integrity_matches?(res.body, integrity) | ||
| raise IntegrityError, uri.to_s if @options[:io_exceptions] | ||
|
|
||
| return nil, nil | ||
| end | ||
|
|
||
| charset = res.respond_to?(:charset) ? res.encoding : 'utf-8' | ||
| src = res.body | ||
| src.encode!('UTF-8', charset) if charset | ||
|
|
||
| [src, charset] | ||
| rescue IntegrityError | ||
| # Let the IntegrityError raised above propagate with its specific | ||
| # class intact, rather than being downgraded to a generic | ||
| # RemoteFileError by the catch-all below. Other RemoteFileErrors | ||
| # raised within this method (e.g. from fetch_via_net_http on a | ||
| # cross-scheme redirect) are intentionally still caught by the | ||
| # catch-all: it discards their (potentially redirect-target-scoped) | ||
| # message in favor of this method's own `uri`, which several | ||
| # existing tests depend on. | ||
| raise | ||
| rescue | ||
| raise RemoteFileError, uri.to_s if @options[:io_exceptions] | ||
|
|
||
| [nil, nil] | ||
| end | ||
| end | ||
|
|
||
| # Verifies +body+ (raw response bytes, not yet charset-decoded) against a | ||
| # Subresource Integrity value -- a single `<algorithm>-<base64 digest>` token, or | ||
| # several whitespace-separated tokens (https://www.w3.org/TR/SRI/#the-integrity-attribute). | ||
| # Tokens using an algorithm this library doesn't recognize are ignored; per the spec's | ||
| # "agility" rule, when multiple recognized algorithms are present only the strongest one | ||
| # is checked. A value containing no recognized algorithm is treated as unverifiable and | ||
| # matches by default, rather than failing every fetch whenever a caller passes a stronger | ||
| # or newer algorithm than this library currently supports. | ||
| def integrity_matches?(body, integrity) # :nodoc: | ||
| candidates = integrity.to_s.split.filter_map do |token| | ||
| algorithm, value = token.split('-', 2) | ||
| [algorithm, value] if algorithm && value && INTEGRITY_ALGORITHMS.key?(algorithm) | ||
| end | ||
| return true if candidates.empty? | ||
|
|
||
| algorithms_present = candidates.map(&:first) | ||
| algorithm = INTEGRITY_ALGORITHMS.each_key.find { |a| algorithms_present.include?(a) } | ||
| expected_values = candidates.select { |a, _v| a == algorithm }.map { |_a, v| v } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. multiple values for the same algorithm is expected ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes... One browser use-case is a stylesheet change that requires supporting two hashes during a change and knowing a CDN will serve one for a while until the cache expires. This is tested by |
||
| digest_class = INTEGRITY_ALGORITHMS.fetch(algorithm) | ||
|
|
||
| expected_values.include?(Base64.strict_encode64(digest_class.digest(body))) | ||
| end | ||
|
|
||
| # Net::HTTP path used only when `allow_local_network: true`. Validates | ||
| # the URI scheme on every redirect hop so a `Location: file://...` | ||
| # cannot be followed even on this opt-in code path. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,147 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require_relative 'test_helper' | ||
|
|
||
| # Tests for `Parser#load_uri!`'s `:integrity` option, which verifies a | ||
| # fetched remote stylesheet against a Subresource Integrity value | ||
| # (https://www.w3.org/TR/SRI/) before it is parsed -- the same mechanism | ||
| # an HTML `<link integrity="...">` attribute describes. | ||
| class CssParserIntegrityTests < Minitest::Test | ||
| include CssParser | ||
| include WEBrick | ||
|
|
||
| PORT = 12_011 | ||
|
|
||
| def setup | ||
| @www_root = File.expand_path('fixtures', __dir__) | ||
| @fixture_file = File.expand_path('fixtures/simple.css', __dir__) | ||
| @fixture_body = File.binread(@fixture_file) | ||
| @uri_base = "http://127.0.0.1:#{PORT}" | ||
|
|
||
| # `:integrity` verification only applies on the remote-fetch path, so | ||
| # these tests use `allow_local_network: true` against a loopback | ||
| # fixture server rather than mocking the HTTP layer -- matching the | ||
| # approach `test_allow_local_network_opt_in_permits_loopback` uses in | ||
| # test_css_parser_ssrf.rb. | ||
| @server_thread = Thread.new do | ||
| s = WEBrick::HTTPServer.new( | ||
| Port: PORT, BindAddress: '127.0.0.1', DocumentRoot: @www_root, | ||
| Logger: Log.new(nil, BasicLog::FATAL), AccessLog: [] | ||
| ) | ||
| begin | ||
| s.start | ||
| ensure | ||
| s.shutdown | ||
| end | ||
| end | ||
|
|
||
| sleep 1 | ||
| end | ||
|
|
||
| def teardown | ||
| @server_thread.kill | ||
| @server_thread.join(5) | ||
| @server_thread = nil | ||
| end | ||
|
|
||
| def cp | ||
| Parser.new(allow_local_network: true) | ||
| end | ||
|
|
||
| def sha(algorithm, body = @fixture_body) | ||
| digest_class = {'sha256' => Digest::SHA256, 'sha384' => Digest::SHA384, 'sha512' => Digest::SHA512}.fetch(algorithm) | ||
| "#{algorithm}-#{Base64.strict_encode64(digest_class.digest(body))}" | ||
| end | ||
|
|
||
| def test_load_uri_without_integrity_option_is_unaffected | ||
| cp.load_uri!("#{@uri_base}/simple.css") | ||
| # no-op: reaching here without an exception is the assertion. | ||
| end | ||
|
|
||
| def test_matching_sha384_integrity_loads_normally | ||
| parser = cp | ||
| parser.load_uri!("#{@uri_base}/simple.css", integrity: sha('sha384')) | ||
| assert_equal 'margin: 0px;', parser.find_by_selector('p').join(' ') | ||
| end | ||
|
|
||
| def test_matching_sha256_integrity_loads_normally | ||
| parser = cp | ||
| parser.load_uri!("#{@uri_base}/simple.css", integrity: sha('sha256')) | ||
| assert_equal 'margin: 0px;', parser.find_by_selector('p').join(' ') | ||
| end | ||
|
|
||
| def test_matching_sha512_integrity_loads_normally | ||
| parser = cp | ||
| parser.load_uri!("#{@uri_base}/simple.css", integrity: sha('sha512')) | ||
| assert_equal 'margin: 0px;', parser.find_by_selector('p').join(' ') | ||
| end | ||
|
|
||
| def test_mismatched_integrity_is_refused | ||
| tampered = "#{sha('sha384')[0, 15]}not-the-real-digest-at-all==" | ||
| assert_raises(CssParser::IntegrityError) do | ||
| cp.load_uri!("#{@uri_base}/simple.css", integrity: tampered) | ||
| end | ||
| end | ||
|
|
||
| def test_mismatched_integrity_raises_a_subclass_of_remote_file_error | ||
| # CssParser::IntegrityError must remain catchable by existing | ||
| # `rescue RemoteFileError` callers. | ||
| tampered = "#{sha('sha384')[0, 15]}not-the-real-digest-at-all==" | ||
| assert_raises(CssParser::RemoteFileError) do | ||
| cp.load_uri!("#{@uri_base}/simple.css", integrity: tampered) | ||
| end | ||
| end | ||
|
|
||
| def test_mismatched_integrity_without_io_exceptions_loads_nothing | ||
| parser = Parser.new(allow_local_network: true, io_exceptions: false) | ||
| tampered = "#{sha('sha384')[0, 15]}not-the-real-digest-at-all==" | ||
| parser.load_uri!("#{@uri_base}/simple.css", integrity: tampered) | ||
| assert_empty parser.find_by_selector('p') | ||
| end | ||
|
|
||
| def test_strongest_algorithm_wins_when_multiple_present_and_it_fails | ||
| # A correct sha256 paired with a wrong sha512 must fail -- the spec's | ||
| # "agility" rule means only the strongest present algorithm (sha512 | ||
| # here) is authoritative, so a right-but-weaker value must not mask | ||
| # a wrong-but-stronger one. | ||
| value = "#{sha('sha256')} sha512-#{Base64.strict_encode64('not the real digest')}" | ||
| assert_raises(CssParser::IntegrityError) do | ||
| cp.load_uri!("#{@uri_base}/simple.css", integrity: value) | ||
| end | ||
| end | ||
|
|
||
| def test_strongest_algorithm_wins_when_multiple_present_and_it_passes | ||
| # Mirror of the above: a wrong sha256 paired with a correct sha512 | ||
| # must still pass, since sha512 is the one actually checked. | ||
| wrong_sha256 = "sha256-#{Base64.strict_encode64('not the real digest')}" | ||
| value = "#{wrong_sha256} #{sha('sha512')}" | ||
| parser = cp | ||
| parser.load_uri!("#{@uri_base}/simple.css", integrity: value) | ||
| assert_equal 'margin: 0px;', parser.find_by_selector('p').join(' ') | ||
| end | ||
|
|
||
| def test_multiple_values_for_the_same_algorithm_accepts_any_match | ||
| # The spec allows several acceptable digests for the same algorithm | ||
| # (e.g. during a stylesheet rotation) -- any match should pass. | ||
| other_value = "sha384-#{Base64.strict_encode64('some other build of the file')}" | ||
| value = "#{other_value} #{sha('sha384')}" | ||
| parser = cp | ||
| parser.load_uri!("#{@uri_base}/simple.css", integrity: value) | ||
| assert_equal 'margin: 0px;', parser.find_by_selector('p').join(' ') | ||
| end | ||
|
|
||
| def test_unrecognized_algorithm_only_value_is_unverifiable_and_passes | ||
| # md5 is not in INTEGRITY_ALGORITHMS. A value naming only an | ||
| # unsupported algorithm can't be checked either way, so it's treated | ||
| # as unverifiable rather than failing every such fetch. | ||
| parser = cp | ||
| parser.load_uri!("#{@uri_base}/simple.css", integrity: 'md5-deadbeef==') | ||
| assert_equal 'margin: 0px;', parser.find_by_selector('p').join(' ') | ||
| end | ||
|
|
||
| def test_blank_integrity_option_is_unaffected | ||
| parser = cp | ||
| parser.load_uri!("#{@uri_base}/simple.css", integrity: '') | ||
| assert_equal 'margin: 0px;', parser.find_by_selector('p').join(' ') | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the strongest and not the first ?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the spec:
https://www.w3.org/TR/sri/#agility
Per the documentation:
https://developer.mozilla.org/en-US/docs/Web/Security/Defenses/Subresource_Integrity#how_browsers_handle_subresource_integrity
TL;DR: This is just following the specification