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

Inject 'strict-dynamic' into CSPRO #60

Merged
merged 4 commits into from
Aug 27, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions ChangeLogs/ChangeLog-2.0.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@
All notable changes of the SecureHeaders 2.0 release series are documented in
this file using the [Keep a CHANGELOG](http://keepachangelog.com/) principles.

## [2.0.1] - *unreleased*
### Fixed
* Ensure `strict-dynamic` is also opportunistically injected into the report
only CSP; add missing options to control this behaviour

## [2.0] - *2017-07-16*

### Added
Expand Down
30 changes: 29 additions & 1 deletion docs/complements/auto.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ SecureHeaders::AUTO_ALL = SecureHeaders::AUTO_ADD
| SecureHeaders::AUTO_COOKIE_SECURE
| SecureHeaders::AUTO_COOKIE_HTTPONLY
| SecureHeaders::AUTO_COOKIE_SAMESITE
| SecureHeaders::AUTO_STRICTDYNAMIC_ENFORCE
| SecureHeaders::AUTO_STRICTDYNAMIC_REPORT
| SecureHeaders::AUTO_STRICTDYNAMIC
```
`AUTO_ALL` will enable everything listed below.

Expand Down Expand Up @@ -59,4 +62,29 @@ SecureHeaders::AUTO_COOKIE_SAMESITE
```
`AUTO_COOKIE_SAMESITE` will ensure that cookies considered [protected](protectedCookie), will have the `SameSite` flag when they are sent to the users browser. See [`->sameSiteCookies`](sameSiteCookies) for infomation on whether this will set either `SameSite=Lax` or `SameSite=Strict`. The default is `SameSite=Lax`, though the default will change to `SameSite=Strict` if [`->strictMode`](strictMode) is enabled. Having a `SameSite` setting will ensure that the cookie is not accessible when certain cross-origin requests are made. See the [specification](https://tools.ietf.org/html/draft-west-first-party-cookies-07#section-4.1) for exactly what this means for the different settings.

**A note on forgoing this setting:** Corrections to incorrectly marking cookies as `SameSite`, or additions to the cookies that are automatically marked can be made using [`protectedCookie`](protectedCookie).
**A note on forgoing this setting:** Corrections to incorrectly marking cookies as `SameSite`, or additions to the cookies that are automatically marked can be made using [`protectedCookie`](protectedCookie).

### AUTO_STRICTDYNAMIC_ENFORCE
```php
SecureHeaders::AUTO_STRICTDYNAMIC_ENFORCE
```
`AUTO_STRICTDYNAMIC_ENFORCE` will ensure that strict dynamic is injected into
the `Content-Security-Policy` header **if** [`->strictMode`](strictMode) is
also enabled.

### AUTO_STRICTDYNAMIC_REPORT
```php
SecureHeaders::AUTO_STRICTDYNAMIC_REPORT
```
`AUTO_STRICTDYNAMIC_REPORT` will ensure that strict dynamic is injected into
the `Content-Security-Policy-Report-Only` header **if**
[`->strictMode`](strictMode) is also enabled.

### AUTO_STRICTDYNAMIC
```php
SecureHeaders::AUTO_STRICTDYNAMIC = SecureHeaders::AUTO_STRICTDYNAMIC_ENFORCE
| SecureHeaders::AUTO_STRICTDYNAMIC_REPORT
```
`AUTO_STRICTDYNAMIC` will ensure that strict dynamic is injected into
the `Content-Security-Policy` **and** `Content-Security-Policy-Report-Only`
headers **if** [`->strictMode`](strictMode) is also enabled.
30 changes: 29 additions & 1 deletion docs/v2/auto.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ SecureHeaders::AUTO_ALL = SecureHeaders::AUTO_ADD
| SecureHeaders::AUTO_COOKIE_SECURE
| SecureHeaders::AUTO_COOKIE_HTTPONLY
| SecureHeaders::AUTO_COOKIE_SAMESITE
| SecureHeaders::AUTO_STRICTDYNAMIC_ENFORCE
| SecureHeaders::AUTO_STRICTDYNAMIC_REPORT
| SecureHeaders::AUTO_STRICTDYNAMIC
```
`AUTO_ALL` will enable everything listed below.

Expand Down Expand Up @@ -73,4 +76,29 @@ SecureHeaders::AUTO_COOKIE_SAMESITE
```
`AUTO_COOKIE_SAMESITE` will ensure that cookies considered [protected](protectedCookie), will have the `SameSite` flag when they are sent to the users browser. See [`->sameSiteCookies`](sameSiteCookies) for infomation on whether this will set either `SameSite=Lax` or `SameSite=Strict`. The default is `SameSite=Lax`, though the default will change to `SameSite=Strict` if [`->strictMode`](strictMode) is enabled. Having a `SameSite` setting will ensure that the cookie is not accessible when certain cross-origin requests are made. See the [specification](https://tools.ietf.org/html/draft-west-first-party-cookies-07#section-4.1) for exactly what this means for the different settings.

**A note on forgoing this setting:** Corrections to incorrectly marking cookies as `SameSite`, or additions to the cookies that are automatically marked can be made using [`protectedCookie`](protectedCookie).
**A note on forgoing this setting:** Corrections to incorrectly marking cookies as `SameSite`, or additions to the cookies that are automatically marked can be made using [`protectedCookie`](protectedCookie).

### AUTO_STRICTDYNAMIC_ENFORCE
```php
SecureHeaders::AUTO_STRICTDYNAMIC_ENFORCE
```
`AUTO_STRICTDYNAMIC_ENFORCE` will ensure that strict dynamic is injected into
the `Content-Security-Policy` header **if** [`->strictMode`](strictMode) is
also enabled.

### AUTO_STRICTDYNAMIC_REPORT
```php
SecureHeaders::AUTO_STRICTDYNAMIC_REPORT
```
`AUTO_STRICTDYNAMIC_REPORT` will ensure that strict dynamic is injected into
the `Content-Security-Policy-Report-Only` header **if**
[`->strictMode`](strictMode) is also enabled.

### AUTO_STRICTDYNAMIC
```php
SecureHeaders::AUTO_STRICTDYNAMIC = SecureHeaders::AUTO_STRICTDYNAMIC_ENFORCE
| SecureHeaders::AUTO_STRICTDYNAMIC_REPORT
```
`AUTO_STRICTDYNAMIC` will ensure that strict dynamic is injected into
the `Content-Security-Policy` **and** `Content-Security-Policy-Report-Only`
headers **if** [`->strictMode`](strictMode) is also enabled.
6 changes: 3 additions & 3 deletions docs/v2/strictMode.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ When enabled, strict mode will:
[header proposal](header-proposals), and can thus be removed or
modified.

Don't forget to [manually submit](https://hstspreload.appspot.com/)
your domain to the HSTS preload list if you are using this option.

* The source keyword `'strict-dynamic'` will also be added to the first
of the following directives that exist: `script-src`, `default-src`;
only if that directive also contains a nonce or hash source value, and
Expand All @@ -23,9 +26,6 @@ When enabled, strict mode will:
[1]: https://research.google.com/pubs/pub45542.html "The Insecurity of
Whitelists and the Future of Content Security Policy"

Don't forget to [manually submit](https://hstspreload.appspot.com/)
your domain to the HSTS preload list if you are using this option.

* The default `SameSite` value injected into [`->protectedCookie`](protectedCookie) will
be changed from `SameSite=Lax` to `SameSite=Strict`.
See [`->auto`](auto#AUTO_COOKIE_SAMESITE) to enable/disable injection
Expand Down
38 changes: 25 additions & 13 deletions src/Operations/InjectStrictDynamic.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,49 +6,61 @@
use Aidantwoods\SecureHeaders\Header;
use Aidantwoods\SecureHeaders\HeaderBag;
use Aidantwoods\SecureHeaders\Operation;
use Aidantwoods\SecureHeaders\Util\Types;

class InjectStrictDynamic extends OperationWithErrors implements Operation, ExposesErrors
{
const ENFORCE = 0b01;
const REPORT = 0b10;

private $allowedCSPHashAlgs;
private $mode;

/**
* Create an Operation to inject `'strict-dynamic'` into an appropriate
* CSP directive, $allowedCSPHashAlgs supplies a list of allowed CSP
* hashing algorithms.
*
* @param array $hstsConfig
* @param array $allowedCSPHashAlgs
*/
public function __construct(array $allowedCSPHashAlgs)
public function __construct(array $allowedCSPHashAlgs, $mode)
{
Types::assert(['int' => [$mode]], [2]);

$this->allowedCSPHashAlgs = $allowedCSPHashAlgs;
$this->mode = $mode;
}

/**
* Transform the given set of headers
*
* @param HeaderBag $headers
* @param HeaderBag $HeaderBag
* @return void
*/
public function modify(HeaderBag &$headers)
public function modify(HeaderBag &$HeaderBag)
{
$CSPHeaders = $headers->getByName('content-security-policy');
$CSPHeaders = array_merge(
$this->mode & self::ENFORCE ?
$HeaderBag->getByName('content-security-policy') : [],
$this->mode & self::REPORT ?
$HeaderBag->getByName('content-security-policy-report-only') : []
);

if (isset($CSPHeaders[0]))
foreach ($CSPHeaders as $Header)
{
$header = $CSPHeaders[0];

$directive = $this->canInjectStrictDynamic($header);
$directive = $this->canInjectStrictDynamic($Header);

if (is_string($directive))
{
$header->setAttribute($directive, "'strict-dynamic'");
$Header->setAttribute($directive, "'strict-dynamic'");
}
elseif ($directive !== -1)
{
$this->addError(
"<b>Strict-Mode</b> is enabled, but <b>'strict-dynamic'</b>
could not be added to the Content-Security-Policy
because no hash or nonce was used.",
"<b>Strict-Mode</b> is enabled, but
<b>'strict-dynamic'</b> could not be added to <b>"
. $Header->getFriendlyName()
. '</b> because no hash or nonce was used.',
E_USER_WARNING
Copy link

Choose a reason for hiding this comment

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

please throw an exception instead, it's ugly codestyle imo.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Might be something to think about for 3.0, would be a BC break to start doing it now though.

Things to think about with errors v.s. exceptions:

  • Exceptions will generally cause headers not to reach the adapter, and so won't be sent. i.e.
    • Is no CSP better than suboptimal CSP? (bearing in mind loss of reporting header which might be stricter too).
    • Is not sending a suboptimal CSP better than HSTS, Referrer-Policy, cookie upgrades, ...?
  • It we throw exceptions, they should be genuine problems. That means not throwing an exception for advice: we cannot use exceptions to tell the dev they they should use something (learning opportunity). Learning opportunities would have to be disposed of if we converted all errors as used currently to exceptions.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Feel free to open an issue, this would be a methodology change for the library so probably isn't worth debating for this one specific case 😉

I like exceptions, I just don't think they're appropriate for every case in which errors are used in the library currently (probably a separate issue is best to discuss the decoupling).

Copy link

Choose a reason for hiding this comment

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

done.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks :D

);
}
Expand Down
32 changes: 22 additions & 10 deletions src/SecureHeaders.php
Original file line number Diff line number Diff line change
Expand Up @@ -195,12 +195,20 @@ class SecureHeaders

# auto-headers

const AUTO_ADD = 0b00001;
const AUTO_REMOVE = 0b00010;
const AUTO_COOKIE_SECURE = 0b00100;
const AUTO_COOKIE_HTTPONLY = 0b01000;
const AUTO_COOKIE_SAMESITE = 0b10000;
const AUTO_ALL = 0b11111;
const AUTO_ADD = 0b0000001;
const AUTO_REMOVE = 0b0000010;

## cookie attribute injection
const AUTO_COOKIE_SECURE = 0b0000100;
const AUTO_COOKIE_HTTPONLY = 0b0001000;
const AUTO_COOKIE_SAMESITE = 0b0010000;

## opportunistic strict-dynamic injection
const AUTO_STRICTDYNAMIC_ENFORCE = 0b0100000;
const AUTO_STRICTDYNAMIC_REPORT = 0b1000000;
const AUTO_STRICTDYNAMIC = 0b1100000;

const AUTO_ALL = 0b1111111;

# cookie upgrades

Expand Down Expand Up @@ -316,6 +324,9 @@ public function safeModeException($name)
* [header proposal](header-proposals), and can thus be removed or
* modified.
*
* Don't forget to [manually submit](https://hstspreload.appspot.com/)
* your domain to the HSTS preload list if you are using this option.
*
* * The source keyword `'strict-dynamic'` will also be added to the first
* of the following directives that exist: `script-src`, `default-src`;
* only if that directive also contains a nonce or hash source value, and
Expand All @@ -329,9 +340,6 @@ public function safeModeException($name)
* [1]: https://research.google.com/pubs/pub45542.html "The Insecurity of
* Whitelists and the Future of Content Security Policy"
*
* Don't forget to [manually submit](https://hstspreload.appspot.com/)
* your domain to the HSTS preload list if you are using this option.
*
* * The default `SameSite` value injected into {@see protectedCookie} will
* be changed from `SameSite=Lax` to `SameSite=Strict`.
* See [`->auto`](auto#AUTO_COOKIE_SAMESITE) to enable/disable injection
Expand Down Expand Up @@ -1624,7 +1632,11 @@ private function pipeline()

if ($this->strictMode)
{
$operations[] = new InjectStrictDynamic($this->allowedCSPHashAlgs);
$operations[] = new InjectStrictDynamic(
$this->allowedCSPHashAlgs,
($this->automaticHeaders & self::AUTO_STRICTDYNAMIC_ENFORCE ? InjectStrictDynamic::ENFORCE : 0)
| ($this->automaticHeaders & self::AUTO_STRICTDYNAMIC_REPORT ? InjectStrictDynamic::REPORT : 0)
);
}

if ($this->safeMode)
Expand Down
46 changes: 45 additions & 1 deletion tests/CSPTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ class CSPTest extends PHPUnit_Framework_TestCase
public function testStrictDynamicInjectableForNonInternalPolicy()
{
$headerStrings = new StringHttpAdapter([
"Content-Security-Policy: script-src 'nonce-abcdefg+123456'"
"Content-Security-Policy: script-src 'nonce-abcdefg+123456'",
"Content-Security-Policy-Report-Only: script-src 'nonce-abcdefg+123456'"
]);

$headers = new SecureHeaders;
Expand All @@ -24,6 +25,49 @@ public function testStrictDynamicInjectableForNonInternalPolicy()
$headersString = $headerStrings->getSentHeaders();

$this->assertContains("Content-Security-Policy: script-src 'nonce-abcdefg+123456' 'strict-dynamic'", $headersString);
$this->assertContains("Content-Security-Policy-Report-Only: script-src 'nonce-abcdefg+123456' 'strict-dynamic'", $headersString);
}

public function testStrictDynamicInjectOnlyEnforced()
{
$headerStrings = new StringHttpAdapter([
"Content-Security-Policy: script-src 'nonce-abcdefg+123456'",
"Content-Security-Policy-Report-Only: script-src 'nonce-abcdefg+123456'"
]);

$headers = new SecureHeaders;
$headers->errorReporting(false);
$headers->auto(SecureHeaders::AUTO_STRICTDYNAMIC_ENFORCE);

$headers->strictMode();

$headers->apply($headerStrings);

$headersString = $headerStrings->getSentHeaders();

$this->assertContains("Content-Security-Policy: script-src 'nonce-abcdefg+123456' 'strict-dynamic'", $headersString);
$this->assertNotContains("Content-Security-Policy-Report-Only: script-src 'nonce-abcdefg+123456' 'strict-dynamic'", $headersString);
}

public function testStrictDynamicInjectOnlyReport()
{
$headerStrings = new StringHttpAdapter([
"Content-Security-Policy: script-src 'nonce-abcdefg+123456'",
"Content-Security-Policy-Report-Only: script-src 'nonce-abcdefg+123456'"
]);

$headers = new SecureHeaders;
$headers->errorReporting(false);
$headers->auto(SecureHeaders::AUTO_STRICTDYNAMIC_REPORT);

$headers->strictMode();

$headers->apply($headerStrings);

$headersString = $headerStrings->getSentHeaders();

$this->assertNotContains("Content-Security-Policy: script-src 'nonce-abcdefg+123456' 'strict-dynamic'", $headersString);
$this->assertContains("Content-Security-Policy-Report-Only: script-src 'nonce-abcdefg+123456' 'strict-dynamic'", $headersString);
}

public function testCSPHeaderMerge()
Expand Down