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.

Sign up for GitHub

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

Jump to bottom

Source Deduplication Doesn't Take Schemes into Account #317

Closed
belenko opened this issue Mar 1, 2017 · 6 comments
Closed

Source Deduplication Doesn't Take Schemes into Account #317

belenko opened this issue Mar 1, 2017 · 6 comments
Labels
bug

Comments

@belenko
Copy link

belenko commented Mar 1, 2017 •
edited
Loading

SecureHeaders excessively deduplicates sources without taking schemes into account leading to removal of sources that shouldn't be removed.

I think the problem is with dedup_source_list() which relies on filesystem-like matching.

Expected Header

Content-Security-Policy: default-src 'self' wss://ws.contoso.com *.contoso.com

Actual Header

Content-Security-Policy: default-src 'self' *.contoso.com

Config

SecureHeaders::Configuration.default do |config|
  config.csp = {
    preserve_schemes: true, # this line doesn't matter, actually
    default_src: %w('self' wss://ws.contoso.com *.contoso.com)
  }
end
@oreoshake
Copy link
Contributor

Welp. I'm assuming that policy mangling means your CSP is breaking functionality. The * should only apply to http/https schemes.

@belenko
Copy link
Author

belenko commented Mar 1, 2017

As a workaround we currently settled on %w('self' wss://*.contoso.com *.contoso.com) which works as expected, so not a deal breaker for us (though had to spend some time to figure out who eats that wss:// source :) )

@oreoshake
Copy link
Contributor

I've never been a fan of the deduplication based on * anyways. Maybe we should just rip that out.

@oreoshake
Copy link
Contributor

Like people trying to save a few bytes can optimize elsewhere.

@oreoshake oreoshake added the bug label Jul 21, 2017
srt32 added a commit to srt32/secure_headers that referenced this issue Jun 1, 2022
@srt32
Handle different schemes in dedup logic
c03fd06 
closes github#317
@srt32 srt32 mentioned this issue Jun 1, 2022
Closed
2 tasks
@srt32
Copy link
Member

srt32 commented Jun 1, 2022

This should be resolved by #478

@lgarron lgarron mentioned this issue Oct 19, 2022
Merged
lgarron added a commit that referenced this issue Oct 24, 2022
@lgarron
Remove source expression deduplication. (#499)
b6ef2ed 
This PR removes `dedup_source_list` and replaces it with a simple
`.uniq` call. This resolves
#491, which is only the
latest in a series of ongoing issues with source expression
deduplication.

`secure_headers` has had this feature [since
2015](32bb3f5)
that [deduplicates redundant URL source
expressions](https://github.com/github/secure_headers/blob/494b75ff927464ed8d1c43e98e41fe4d15ce2bdf/lib/secure_headers/headers/content_security_policy.rb#L157-L170).
For example, if `*.github.com` is listed as a source expression for a
given
[directive](https://w3c.github.io/webappsec-csp/#framework-directives),
then the addition of `example.github.com` would have no effect, and so
the latter can be safely removed by `secure_headers` to save bytes.

Unfortunately, this implementation has had various bugs due to the use
of "impedance mismatched" APIs like
[`URI`](https://docs.ruby-lang.org/en/2.1.0/URI.html)[^1] and
[`File.fnmatch`](https://apidock.com/ruby/v2_5_5/File/fnmatch/class)[^2].
For example, it made incorrect assumptions about source expression
schemes, leading to the following series of events:

[^1]: Which allows wildcards in domains but not for ports, as it is not
designed to parse URL source expressions.
[^2]: Which has general glob matching that is not designed for URL
source expressions either.

- 2017-03: A [bug was reported and
confirmed](#317)
- 2022-04: The bug was finally [fixed by `@keithamus` (a Hubber) in
2022](#478) due to our use
of web sockets.
- 2022-06: This fix in turn triggered a [new
bug](#491) with source
expressions like `data:`.
- 2022-06: An external contributor [submitted a fix for the bew
bug](#490), but this still
doesn't address some of the "fast and loose" semantic issues of the
underlying implementation.
- 2022-08: `@lgarron` [drafted a new
implementation](#498) that
semantically parses and compares source expressions based on the
specification for source expressions.
- This implementation already proved to have some value in early
testing, as its stricter validation caught an issue in `github.com`'s
CSP. However, it would take additional work to make this implementation
fully aware of CSP syntax (e.g. not allowing URL source expressions in a
source directive when only special keywords are allowed, and
vice-versa), and it relies on a new regex-based implementation of source
expression parsing that may very well lead to more subtle bugs.

In effect, this is a half feature whose maintenance cost has outweighed
its functionality:

- The relevant code has suffered from continued bugs, described as
above.
- Deduplication is purely a "nice-to-have" — it is not necessary for the
security or correct functionality of `secure_headers`.
- It was [introduced by `@oreoshake` (the then-maintainer) without
explanation in
2015](32bb3f5),
never "officially" documented. We have no concrete data on whether it
has any performance impact on any real apps — for all we know, uncached
deduplication calculations might even cost more than the saved header
bytes.
- Further, in response to the first relevant bug, `@oreoshake` himself
[said](#317 (comment)):

> I've never been a fan of the deduplication based on `*` anyways. Maybe
we should just rip that out.

> Like people trying to save a few bytes can optimize elsewhere.

So this PR completely removes the functionality. If we learn of a use
case where this was very important (and the app somehow can't preprocess
the list before passing it to `secure_headers`), we can always resume
consideration of one of:

- #490
- #498
@lgarron
Copy link
Contributor

lgarron commented Jan 3, 2023 •
edited
Loading

Source deduplication was removed in #499

Expected Header

Content-Security-Policy: default-src 'self' wss://ws.contoso.com *.contoso.com

I've confirmed that this is the case.

We also have a very similar test to prevent a regression:

secure_headers/spec/lib/secure_headers/headers/content_security_policy_spec.rb

Line 115 in 51b9845

csp = ContentSecurityPolicy.new(default_src: %w(*.example.org wss://example.example.org))

@lgarron lgarron closed this as completed Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Handle different schemes in dedup logic srt32/secure_headers
5 participants
@lgarron @oreoshake @srt32 @belenko and others